Re: [PATCH] net/mlx5e: Avoid field-overflowing memcpy()

On Fri, 2021-08-06 at 15:45 -0700, Kees Cook wrote:

On Fri, Aug 06, 2021 at 03:17:56PM -0700, Saeed Mahameed wrote:

quoted

On Fri, 2021-08-06 at 14:50 -0700, Kees Cook wrote:

quoted

[...]
So, split the memcpy() so the compiler can reason about the buffer
sizes.

"pahole" shows no size nor member offset changes to struct > >
mlx5e_tx_wqe
nor struct mlx5e_umr_wqe. "objdump -d" shows no meaningful object
code changes (i.e. only source line number induced differences and
optimizations).

spiting the memcpy doesn't induce any performance degradation ? extra
instruction to copy the 1st 2 bytes ? 

Not meaningfully, but strictly speaking, yes, it's a different series
of
instructions.

quoted

[...]

quoted

--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c

why only here ? mlx5 has at least 3 other places where we use this
unbound memcpy .. 

Can you point them out? I've been fixing only the ones I've been able
to
find through instrumentation (generally speaking, those with fixed
sizes).

we will need to examine each change carefully to look for performance
degradation and maybe run some micro-benchmark tests in house before i
can ack this patch. 

$ git grep -n "eseg->inline_hdr.start"
drivers/infiniband/hw/mlx5/wr.c:129:            copysz = min_t(u64,
*cur_edge - (void *)eseg->inline_hdr.start,
drivers/infiniband/hw/mlx5/wr.c:131:            memcpy(eseg-

inline_hdr.start, pdata, copysz);

drivers/infiniband/hw/mlx5/wr.c:133:                          
sizeof(eseg->inline_hdr.start) + copysz, 16);
drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c:344:          
memcpy(eseg->inline_hdr.start, xdptxd->data, MLX5E_XDP_MIN_INLINE);
drivers/net/ethernet/mellanox/mlx5/core/en_tx.c:510:                  
mlx5e_insert_vlan(eseg->inline_hdr.start, skb, attr->ihs);
drivers/net/ethernet/mellanox/mlx5/core/en_tx.c:514:                  
memcpy(eseg->inline_hdr.start, skb->data, attr->ihs);
drivers/net/ethernet/mellanox/mlx5/core/en_tx.c:1033:          
memcpy(eseg->inline_hdr.start, skb->data, attr.ihs);