[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.