[PATCH net 0/1] net/mlx5e: fix NULL pointer dereference in arfs_create_groups()

[PATCH net 0/1] net/mlx5e: fix NULL pointer dereference in arfs_create_groups()

[Jinvas]

The following patch fixes a NULL pointer dereference in the mlx5e aRFS
code, which oopses the kernel during NIC bring-up.

The bug was introduced by commit 3c6d5189246f ("net/mlx5e: fix a
double-free in arfs_create_groups", in v6.8) and is still present in
v7.4-rc2.

1. What happens

arfs_create_groups() creates up to two flow groups in ft->g[], and
increments ft->num_groups for each group that was created successfully.
If creating the second group fails, the error path frees ft->g and sets
it to NULL, but leaves ft->num_groups at 1.

The only caller, arfs_create_table(), calls mlx5e_destroy_flow_table() on
the error path, which calls mlx5e_destroy_groups():

	for (i = ft->num_groups - 1; i >= 0; i--) {
		if (!IS_ERR_OR_NULL(ft->g[i]))
			mlx5_destroy_flow_group(ft->g[i]);
		ft->g[i] = NULL;
	}

ft->g is NULL at this point, so ft->g[0] is a NULL pointer dereference.
Note that only a failure of the *second* group triggers this; if the
first group fails, num_groups is still 0 and the loop body never runs.

The same error path also leaks the flow group that was created
successfully before the failure, since ft->g is freed without the group
being destroyed.

2. How this was introduced

Commit 3c6d5189246f fixed a double free of ft->g between
arfs_create_groups() and its caller. Before that commit, the group
failure path did not touch ft->g at all:

	err:
		err = PTR_ERR(ft->g[ft->num_groups]);
		ft->g[ft->num_groups] = NULL;
	out:
		kvfree(in);
		return err;

so the caller destroyed the flow groups that had been created and then
freed the array. That commit fixed the double free in the allocation
failure path by freeing ft->g here as well, and in doing so changed the
group failure path too:

	err_clean_group:
		err = PTR_ERR(ft->g[ft->num_groups]);
		ft->g[ft->num_groups] = NULL;
	err_free_in:
		kvfree(in);
	err_free_g:
		kfree(ft->g);
		ft->g = NULL;
		return err;

The reset of ft->num_groups that would have prevented this was
suggested during review, but deferred ("it is a better idea to leave it
to a refector patch one day"), and that patch never landed.

3. The oops

Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
Oops [#1]
epc : mlx5e_destroy_groups+0x38/0x6c
 ra : mlx5e_destroy_flow_table+0x10/0x30
status: 0000000200000120 badaddr: 0000000000000000 cause: 000000000000000d
[<ffffffff80843604>] mlx5e_arfs_create_tables+0x44c/0x5d8
[<ffffffff80815bd6>] mlx5e_create_flow_steering+0x42/0x6bc
[<ffffffff808043e4>] mlx5e_init_nic_rx+0x84/0x14c
[<ffffffff80808a4c>] mlx5e_attach_netdev+0xb8/0x2e8
[<ffffffff8080f36e>] mlx5e_probe+0x48e/0x81c
Code: 89b3 40f9 e052 098e 7a7d a011 84be 3783 0109 97a6 (6398) 853a
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

The register dump shows a5 = 0, i.e. ft->g was read as NULL. Offset 16
of struct mlx5e_flow_table is ft->g, and the faulting instruction loads
from the address in a5, so the faulting access is the ft->g[0] read.

Note that the bug itself is not architecture specific. It is a plain
error path in C, and it is reachable on any architecture; the register
dump above is only from the machine on which it was hit.

4. How this was hit

This was found while testing a virtual mlx5 device whose firmware
answers host mailbox commands. The device took longer than the
firmware's own timeout to answer one CREATE_FLOW_GROUP command, so the
firmware reported the command as failed even though the device had
completed it. The host then took its error path, and the bug above
turned a command failure into a kernel panic.

That is only the trigger though, not the cause: the host oopses on any
error returned by CREATE_FLOW_GROUP, no matter where the error comes
from (a device that rejects the command, a resource shortage, a command
timeout, ...). A firmware that returns an error for the second
CREATE_FLOW_GROUP is enough to reproduce it.

A minimal reproduction is therefore: enable aRFS, and make the second
CREATE_FLOW_GROUP of one of the aRFS flow tables fail. The kernel
then calls mlx5e_destroy_flow_table() with ft->num_groups == 1 and
ft->g == NULL, and dereferences the NULL array.

5. The fix

The patch leaves the free of ft->g to the caller:
mlx5e_destroy_flow_table() frees ft->g exactly once, and also destroys
the flow groups that were created. This fixes the NULL pointer
dereference and the leak, while keeping the double free fixed.

For the three error paths of arfs_create_groups():

  ft->g     num_groups  what the caller does
  --------  ----------  ----------------------------------------
  NULL      0           kfree(NULL), no group to destroy
  non-NULL  0           kfree(ft->g), no group to destroy
  non-NULL  1           destroy the created group, then kfree(ft->g)

ft->g is therefore freed exactly once in all three cases, so the double
free that 3c6d5189246f fixed stays fixed.

The patch also makes the error handling of this function consistent
with the rest of the driver, e.g. mlx5e_create_l2_table().

6. Scope

The patch was reviewed against v7.4-rc2 and applies cleanly. The crash
itself was reproduced on a virtual mlx5 device; the patched kernel was
not booted, so this patch is not marked as tested.

--

Jinvas (1):
  net/mlx5e: fix NULL pointer dereference in arfs_create_groups()

 drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

base-commit: 97d73c8251226cf27cc6656b4c542869bb8c69fa
-- 
2.28.0.windows.1

[PATCH net 1/1] net/mlx5e: fix NULL pointer dereference in arfs_create_groups()

[Jinvas]

arfs_create_groups() creates up to two flow groups in ft->g[], and
increments ft->num_groups for each group that was created successfully.
If creating the second group fails, the error path frees ft->g and sets
it to NULL, but leaves ft->num_groups at 1.

The only caller, arfs_create_table(), calls mlx5e_destroy_flow_table() on
the error path, which calls mlx5e_destroy_groups(). The latter walks
ft->g[] from ft->num_groups - 1 down to 0 and dereferences ft->g[0].
Since ft->g was freed and set to NULL, this is a NULL pointer
dereference, and the kernel oopses.

The same error path also leaks the flow group that was created
successfully before the failure, since ft->g is freed without the group
being destroyed.

Freeing ft->g here was added by commit 3c6d5189246f ("net/mlx5e: fix a
double-free in arfs_create_groups"). Before that commit, the group
failure path did not touch ft->g at all, so the caller destroyed the
flow groups that had been created and then freed the array. That
commit fixed a double free in the allocation failure path by freeing
ft->g here as well, and in doing so changed the group failure path
too.

Rather than also resetting ft->num_groups, leave the free of ft->g to
the caller: mlx5e_destroy_flow_table() frees ft->g exactly once, and
also destroys the flow groups that were created. This fixes the NULL
pointer dereference and the leak, while keeping the double free fixed.

This also makes the error handling of this function consistent with the
rest of the driver, e.g. mlx5e_create_l2_table().

Fixes: 3c6d5189246f ("net/mlx5e: fix a double-free in arfs_create_groups")
Signed-off-by: Jinvas <jinvas@126.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c
index c5e62d7..7888d38 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c
@@ -267,7 +267,7 @@ static int arfs_create_groups(struct mlx5e_flow_table *ft,
 	in = kvzalloc(inlen, GFP_KERNEL);
 	if (!in) {
 		err = -ENOMEM;
-		goto err_free_g;
+		goto err_free_in;
 	}
 
 	mc = MLX5_ADDR_OF(create_flow_group_in, in, match_criteria);
@@ -338,9 +338,6 @@ err_clean_group:
 	ft->g[ft->num_groups] = NULL;
 err_free_in:
 	kvfree(in);
-err_free_g:
-	kfree(ft->g);
-	ft->g = NULL;
 	return err;
 }
 
-- 
2.28.0.windows.1