[PATCH net 0/3] net/mlx5: Fix HWS and DR steering memory safety bugs

[PATCH net 0/3] net/mlx5: Fix HWS and DR steering memory safety bugs

[Tariq Toukan]

Hi,

This series by Yevgeny fixes three memory safety bugs in the mlx5
software and hardware steering code paths.

All three are missing-check or wrong-allocation problems that lead to a
NULL pointer being inserted into an xarray, a heap buffer overflow, or
an out-of-bounds write that corrupts the kernel heap.

Regards,
Tariq

Yevgeny Kliteynik (3):
  net/mlx5: HWS, Handle NULL in vport destination action creation
  net/mlx5: HWS, Fix heap buffer overflow in insert header action
  net/mlx5: DR, Fix out-of-bounds write when sync to HW fails

 .../mellanox/mlx5/core/steering/hws/action.c  |  7 +++++-
 .../mellanox/mlx5/core/steering/hws/fs_hws.c  |  3 +++
 .../mlx5/core/steering/sws/dr_icm_pool.c      | 23 +++++++++++++++++--
 3 files changed, 30 insertions(+), 3 deletions(-)


base-commit: b6ef0d965fe8ce0ee4b4dcb58b3511f83003e42d
-- 
2.44.0

[PATCH net 1/3] net/mlx5: HWS, Handle NULL in vport destination action creation

[Tariq Toukan]

From: Yevgeny Kliteynik <kliteyn@nvidia.com>

mlx5hws_action_create_dest_vport can return NULL - handle
this case before trying to insert NULL into xarray.

Fixes: 8e2e08a6d1e0 ("net/mlx5: fs, add support for dest vport HWS action")
Signed-off-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
Signed-off-by: Alex Vesker <valex@nvidia.com>
Reviewed-by: Erez Shitrit <erezsh@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c
index 54761754e657..c4574c5f4ded 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c
@@ -440,6 +440,9 @@ mlx5_fs_get_dest_action_vport(struct mlx5_fs_hws_context *fs_ctx,
 	dest = mlx5hws_action_create_dest_vport(ctx, vport_num,	vhca_id_valid,
 						dest_attr->vport.vhca_id, flags);
 
+	if (!dest)
+		return NULL;
+
 	err = xa_insert(dests_xa, idx, dest, GFP_KERNEL);
 	if (err) {
 		mlx5hws_action_destroy(dest);
-- 
2.44.0

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

[Tariq Toukan]

From: Yevgeny Kliteynik <kliteyn@nvidia.com>

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.

Use hws_action_create_generic_bulk() to allocate the right number
of actions.

Fixes: 504e536d9010 ("net/mlx5: HWS, added actions handling")
Signed-off-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
Reviewed-by: Itamar Gozlan <igozlan@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/steering/hws/action.c  | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

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 23e86f292022..9caaa34074db 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;
 
-- 
2.44.0

[PATCH net 3/3] net/mlx5: DR, Fix out-of-bounds write when sync to HW fails

[Tariq Toukan]

From: Yevgeny Kliteynik <kliteyn@nvidia.com>

mlx5dr_icm_free_chunk() moves a freed chunk to the "hot" chunks array by
writing it to hot_chunks_arr and, once the accumulated hot memory
crosses a threshold, triggers a sync to HW that flushes the array.  The
array is large enough to hold the chunks of a single accumulation cycle,
and relies on the sync path to flush it.

However, if sync to HW fails, hot chunks array is not cleared, and every
subsequent mlx5dr_icm_free_chunk will write past the end of
hot_chunks_arr, resulting in an out-of-bounds write and kernel heap
corruption.

A failed sync to HW also means it is no longer safe to recycle ICM: a
chunk's memory may only be returned to the buddy allocator (and thus be
reused) after a sync has confirmed that HW no longer references it.

Fix both problems by putting the pool into an error state when a sync to
HW fails - mlx5dr_icm_free_chunk() stops queuing freed chunks, so it
never writes past hot_chunks_arr. The chunk's ICM memory is not returned
to the buddy allocator (HW may still reference it); the segment is left
reserved and is reclaimed when the pool is destroyed.

Fixes: 4519fc45beeb ("net/mlx5: DR, Keep track of hot ICM chunks in an array instead of list")
Signed-off-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
Reviewed-by: Alex Vesker <valex@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../mlx5/core/steering/sws/dr_icm_pool.c      | 23 +++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_icm_pool.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_icm_pool.c
index 1b928b40686e..375ab8c2043f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_icm_pool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_icm_pool.c
@@ -33,6 +33,10 @@ struct mlx5dr_icm_pool {
 	u64 hot_memory_size;
 	/* hot memory size threshold for triggering sync */
 	u64 th;
+	/* Set when a sync to HW fails - no new chunks are served,
+	 * and freed chunks are no longer queued.
+	 */
+	bool sync_failed;
 };
 
 struct mlx5dr_icm_dm {
@@ -369,6 +373,12 @@ static int dr_icm_pool_sync_all_buddy_pools(struct mlx5dr_icm_pool *pool)
 	err = mlx5dr_cmd_sync_steering(pool->dmn->mdev);
 	if (err) {
 		mlx5dr_err(pool->dmn, "Failed to sync to HW (err: %d)\n", err);
+		/* The hot chunks could not be flushed. Put the pool in an error
+		 * state so that no further chunks are queued (which would
+		 * overflow hot_chunks_arr) or served (which could reuse ICM
+		 * that HW may still reference).
+		 */
+		pool->sync_failed = true;
 		return err;
 	}
 
@@ -475,6 +485,14 @@ void mlx5dr_icm_free_chunk(struct mlx5dr_icm_chunk *chunk)
 	/* move the chunk to the waiting chunks array, AKA "hot" memory */
 	mutex_lock(&pool->mutex);
 
+	/* If a previous sync to HW failed, the pool is in an error state.
+	 * The chunk must not be queued. ICM memory is not returned to the
+	 * buddy allocator - it is reclaimed only when the pool is destroyed.
+	 * Just free the chunk's bookkeeping struct.
+	 */
+	if (pool->sync_failed)
+		goto out;
+
 	pool->hot_memory_size += mlx5dr_icm_pool_get_chunk_byte_size(chunk);
 
 	hot_chunk = &pool->hot_chunks_arr[pool->hot_chunks_num++];
@@ -482,12 +500,13 @@ void mlx5dr_icm_free_chunk(struct mlx5dr_icm_chunk *chunk)
 	hot_chunk->seg = chunk->seg;
 	hot_chunk->size = chunk->size;
 
-	kmem_cache_free(chunks_cache, chunk);
-
 	/* Check if we have chunks that are waiting for sync-ste */
 	if (dr_icm_pool_is_sync_required(pool))
 		dr_icm_pool_sync_all_buddy_pools(pool);
 
+out:
+	kmem_cache_free(chunks_cache, chunk);
+
 	mutex_unlock(&pool->mutex);
 }
 
-- 
2.44.0

Re: [PATCH net 0/3] net/mlx5: Fix HWS and DR steering memory safety bugs

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

Re: [PATCH net 3/3] net/mlx5: DR, Fix out-of-bounds write when sync to HW fails

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new comments promise that a failed pool stops serving
  chunks, but nothing enforces this.
- [Medium] After a single SYNC_STEERING failure the pool latches into a
  permanent error state while allocation stays open, so device SW ICM…

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the patch now
  relies on it.

--

net/mlx5: DR, Fix out-of-bounds write when sync to HW fails

If the sync to HW fails, mlx5dr_icm_free_chunk() keeps appending to
hot_chunks_arr past its end. This patch adds a sync_failed flag to the
ICM pool. A failed sync sets it, and once it is set, freed chunks are
no longer queued and their ICM stays reserved until the pool is
destroyed.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_icm_pool.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_icm_pool.c
> index 1b928b40686e5..375ab8c2043f9 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_icm_pool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_icm_pool.c
> @@ -33,6 +33,10 @@ struct mlx5dr_icm_pool {
>  	u64 hot_memory_size;
>  	/* hot memory size threshold for triggering sync */
>  	u64 th;
> +	/* Set when a sync to HW fails - no new chunks are served,
> +	 * and freed chunks are no longer queued.
> +	 */
> +	bool sync_failed;
>  };
>  
>  struct mlx5dr_icm_dm {
> @@ -369,6 +373,12 @@ static int dr_icm_pool_sync_all_buddy_pools(struct mlx5dr_icm_pool *pool)
>  	err = mlx5dr_cmd_sync_steering(pool->dmn->mdev);
>  	if (err) {
>  		mlx5dr_err(pool->dmn, "Failed to sync to HW (err: %d)\n", err);
> +		/* The hot chunks could not be flushed. Put the pool in an error
> +		 * state so that no further chunks are queued (which would
> +		 * overflow hot_chunks_arr) or served (which could reuse ICM
> +		 * that HW may still reference).
> +		 */
> +		pool->sync_failed = true;
>  		return err;
>  	}

[Severity: Medium]

This comment and the comment on the new sync_failed field both say that
no new chunks are served once the flag is set. What enforces that?

mlx5dr_icm_alloc_chunk() never reads pool->sync_failed:

	mutex_lock(&pool->mutex);
	/* find mem, get back the relevant buddy pool and seg in that mem */
	ret = dr_icm_handle_buddies_get_mem(pool, chunk_size, &buddy, &seg);

When the existing buddies are full, dr_icm_handle_buddies_get_mem() goes
on to call dr_icm_buddy_create(). The only reader of the flag is
mlx5dr_icm_free_chunk().

The reason given for not serving chunks also looks inaccurate. The
segments of the hot chunks, and any segments freed after the failure,
stay allocated in the buddy bitmap. For freed chunks, mlx5dr_buddy_free_mem()
is only reached through dr_icm_pool_clear_hot_chunks_arr(), so the
buddy cannot hand that ICM out again anyway.

Should these comments be changed, or is mlx5dr_icm_alloc_chunk() missing
a sync_failed check?

[ ... ]

> @@ -475,6 +485,14 @@ void mlx5dr_icm_free_chunk(struct mlx5dr_icm_chunk *chunk)
>  	/* move the chunk to the waiting chunks array, AKA "hot" memory */
>  	mutex_lock(&pool->mutex);
>  
> +	/* If a previous sync to HW failed, the pool is in an error state.
> +	 * The chunk must not be queued. ICM memory is not returned to the
> +	 * buddy allocator - it is reclaimed only when the pool is destroyed.
> +	 * Just free the chunk's bookkeeping struct.
> +	 */

[Severity: Low]

This is a pre-existing issue and was not introduced by this patch, but
the new error handling now depends on it.

The commit message says ICM may only be returned after a sync has
confirmed that HW no longer references it. At teardown,
mlx5dr_domain_destroy() ignores the result of its own sync:

	/* make sure resources are not used by the hardware */
	mlx5dr_cmd_sync_steering(dmn->mdev);
	...
	dr_domain_uninit_resources(dmn);

mlx5dr_icm_pool_destroy() then calls dr_icm_pool_clear_hot_chunks_arr()
unconditionally. After that, dr_icm_buddy_destroy()->dr_icm_pool_mr_destroy()
gives the SW ICM back to the device through mlx5_dm_sw_icm_dealloc().

So when the patch says the quarantined ICM is reclaimed "when the pool
is destroyed", that ICM can go back to the device even though no sync
has ever succeeded.

Teardown behaved the same way before this patch. Also,
mlx5dr_domain_destroy() returns -EBUSY unless dmn->refcount is 1, so
the tables and matchers that referenced this ICM are already gone by
that point.

Is it acceptable to rely on teardown here, given that it doesn't check
whether the final sync succeeded?

> +	if (pool->sync_failed)
> +		goto out;
> +

[Severity: Medium]

Once sync_failed is set, can this pool keep growing device SW ICM and
host memory with no limit?

Nothing clears the flag. mlx5dr_icm_free_chunk() is the only runtime
caller of dr_icm_pool_sync_all_buddy_pools(), and it now jumps to out
before reaching the sync. A transient SYNC_STEERING failure, such as a
FW timeout or an error status, is therefore never retried.

mlx5dr_cmd_sync_steering() returns 0 when the device is in
MLX5_DEVICE_STATE_INTERNAL_ERROR. A non-zero return therefore means the
command failed on a device that is still running, and the pool can stay
in service for a long time.

In this state, every mlx5dr_icm_free_chunk() frees only the kmem_cache
object. The segment never reaches mlx5dr_buddy_free_mem(), and
buddy->used_memory is never decremented. This also applies to chunks
that were allocated after the failure.

Meanwhile mlx5dr_icm_alloc_chunk() keeps serving chunks. When the
buddies fill up, it takes this path:

mlx5dr_icm_alloc_chunk()
  dr_icm_handle_buddies_get_mem()
    dr_icm_buddy_create()
      dr_icm_pool_mr_create()
        mlx5_dm_sw_icm_alloc()

That path also allocates a new mkey, buddy bitmaps and, for STE pools,
the kvzalloc'd ste_arr/hw_ste_arr/miss_list caches.

With ongoing rule add/delete or hash table rehash, each alloc/free cycle
permanently uses up more ICM. Eventually mlx5_dm_sw_icm_alloc() fails
and the domain can no longer insert rules until it is destroyed.

The commit message covers reclaim being deferred to pool destruction,
but it doesn't mention that allocation keeps going in the meantime.

Would it make sense to either fail allocation once sync_failed is set,
or retry the sync on later frees and clear the flag when it succeeds?

>  	pool->hot_memory_size += mlx5dr_icm_pool_get_chunk_byte_size(chunk);
>  
>  	hot_chunk = &pool->hot_chunks_arr[pool->hot_chunks_num++];

[ ... ]

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