Re: [PATCH net 2/3] net/mlx5: HWS, Fix heap buffer overflow in insert header action

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern: the commit message calls this a 'heap buffer overflow'
  and carries a Fixes: tag, but the overflow is not reachable today.

Pre-existing issues:
- [Medium] mlx5hws_action_create_insert_header()
  (drivers/net/ethernet/mellanox/mlx5/core/steering/hws/action.c:1464)…

--

net/mlx5: HWS, Fix heap buffer overflow in insert header action

This commit changes mlx5hws_action_create_insert_header() to use
hws_action_create_generic_bulk() instead of hws_action_create_generic(),
so it allocates num_of_hdrs action structs instead of one. It also
returns NULL early when num_of_hdrs is zero.

> hws_action_create_generic() allocates only a single action struct,
> but the loop writes to action[i] for each of the num_of_hdrs entries.
> When num_of_hdrs > 1 this overflows the allocated buffer.

[Severity: Low]
This isn't a bug, but could the commit message say that the overflow
can't be hit today?

The only in-tree caller, mlx5_fs_insert_hdr_bulk_action_create() in
fs_hws_pools.c, always passes num_of_hdrs = 1:

	return mlx5hws_action_create_insert_header(ctx, 1, &insert_hdr,
						   log_bulk_size, flags);

The HWS symbols are not exported, so nothing else can reach the
num_of_hdrs > 1 case. The description above is accurate as written. A
note that the overflow can't currently be triggered would help when
reviewing the Fixes: tag for stable.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/action.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/action.c
> index 23e86f2920228..9caaa34074dbb 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/action.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/action.c
> @@ -1473,7 +1473,12 @@ mlx5hws_action_create_insert_header(struct mlx5hws_context *ctx,
>  	int ret;
>  	int i;
>  
> -	action = hws_action_create_generic(ctx, flags, MLX5HWS_ACTION_TYP_INSERT_HEADER);
> +	if (!num_of_hdrs)
> +		return NULL;
> +
> +	action = hws_action_create_generic_bulk(ctx, flags,
> +						MLX5HWS_ACTION_TYP_INSERT_HEADER,
> +						num_of_hdrs);
>  	if (!action)
>  		return NULL;
>  

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. The
zero check and bulk allocation here now match
mlx5hws_action_create_reformat(). Should that function's SHARED flag
check be copied here as well?

mlx5hws_action_create_reformat() rejects this combination:

	if ((flags & MLX5HWS_ACTION_FLAG_SHARED) && (log_bulk_size || num_of_hdrs > 1)) {
		mlx5hws_err(ctx, "Reformat flags don't fit HWS (flags: 0x%x)\n", flags);
		goto free_action;
	}

mlx5hws_action_create_insert_header() has no such check.

Both constructors call hws_action_handle_insert_with_ptr(). That function
sets max_sz to the largest hdrs[i].sz, but passes only the first header's
buffer:

	ret = mlx5hws_arg_create(action->ctx,
				 hdrs->data,
				 max_sz,
				 log_bulk_sz,
				 action->flags & MLX5HWS_ACTION_FLAG_SHARED,
				 &arg_id);

With MLX5HWS_ACTION_FLAG_SHARED set, max_sz bytes are copied from that
single pointer into the send queue WQEs:

  mlx5hws_arg_create()
    mlx5hws_arg_write_inline_arg_data()
      mlx5hws_arg_write()
        memcpy(wqe_arg, arg_data, MLX5HWS_ARG_DATA_SIZE);

Take a caller that passes MLX5HWS_ACTION_FLAG_SHARED, num_of_hdrs > 1,
and a hdrs[0].hdr.sz smaller than a later header's size. Would this read
past the end of hdrs[0].hdr.data and pass those heap bytes to the device?
Headers 1..n would also silently get header 0's data.

The only caller today passes num_of_hdrs = 1 and does not set
MLX5HWS_ACTION_FLAG_SHARED, so this can't be triggered yet. Before this
patch, the same over-read sat behind the action[] overflow.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021074949.3471704-1-tariqt%40nvidia.com