diff mbox series

net/mlx4_en: Fix an use-after-free bug in mlx4_en_try_alloc_resources()

Message ID 20211130164438.190591-1-zhou1615@umn.edu (mailing list archive)
State Accepted
Commit addad7643142f500080417dd7272f49b7a185570
Delegated to: Netdev Maintainers
Headers show
Series net/mlx4_en: Fix an use-after-free bug in mlx4_en_try_alloc_resources() | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 16 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Zhou Qingyang Nov. 30, 2021, 4:44 p.m. UTC
In mlx4_en_try_alloc_resources(), mlx4_en_copy_priv() is called and
tmp->tx_cq will be freed on the error path of mlx4_en_copy_priv().
After that mlx4_en_alloc_resources() is called and there is a dereference
of &tmp->tx_cq[t][i] in mlx4_en_alloc_resources(), which could lead to
a use after free problem on failure of mlx4_en_copy_priv().

Fix this bug by adding a check of mlx4_en_copy_priv()

This bug was found by a static analyzer. The analysis employs
differential checking to identify inconsistent security operations
(e.g., checks or kfrees) between two code paths and confirms that the
inconsistent operations are not recovered in the current function or
the callers, so they constitute bugs.

Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger. Multiple researchers have cross-reviewed
the bug.

Builds with CONFIG_MLX4_EN=m show no new warnings,
and our static analyzer no longer warns about this code.

Fixes: ec25bc04ed8e ("net/mlx4_en: Add resilience in low memory systems")
Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Leon Romanovsky Dec. 1, 2021, 9:27 a.m. UTC | #1
On Wed, Dec 01, 2021 at 12:44:38AM +0800, Zhou Qingyang wrote:
> In mlx4_en_try_alloc_resources(), mlx4_en_copy_priv() is called and
> tmp->tx_cq will be freed on the error path of mlx4_en_copy_priv().
> After that mlx4_en_alloc_resources() is called and there is a dereference
> of &tmp->tx_cq[t][i] in mlx4_en_alloc_resources(), which could lead to
> a use after free problem on failure of mlx4_en_copy_priv().
> 
> Fix this bug by adding a check of mlx4_en_copy_priv()
> 
> This bug was found by a static analyzer. The analysis employs
> differential checking to identify inconsistent security operations
> (e.g., checks or kfrees) between two code paths and confirms that the
> inconsistent operations are not recovered in the current function or
> the callers, so they constitute bugs.
> 
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
> 
> Builds with CONFIG_MLX4_EN=m show no new warnings,
> and our static analyzer no longer warns about this code.
> 
> Fixes: ec25bc04ed8e ("net/mlx4_en: Add resilience in low memory systems")
> Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> ---
>  drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
patchwork-bot+netdevbpf@kernel.org Dec. 2, 2021, 3:10 a.m. UTC | #2
Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  1 Dec 2021 00:44:38 +0800 you wrote:
> In mlx4_en_try_alloc_resources(), mlx4_en_copy_priv() is called and
> tmp->tx_cq will be freed on the error path of mlx4_en_copy_priv().
> After that mlx4_en_alloc_resources() is called and there is a dereference
> of &tmp->tx_cq[t][i] in mlx4_en_alloc_resources(), which could lead to
> a use after free problem on failure of mlx4_en_copy_priv().
> 
> Fix this bug by adding a check of mlx4_en_copy_priv()
> 
> [...]

Here is the summary with links:
  - net/mlx4_en: Fix an use-after-free bug in mlx4_en_try_alloc_resources()
    https://git.kernel.org/netdev/net/c/addad7643142

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 3f6d5c384637..f1c10f2bda78 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2286,9 +2286,14 @@  int mlx4_en_try_alloc_resources(struct mlx4_en_priv *priv,
 				bool carry_xdp_prog)
 {
 	struct bpf_prog *xdp_prog;
-	int i, t;
+	int i, t, ret;
 
-	mlx4_en_copy_priv(tmp, priv, prof);
+	ret = mlx4_en_copy_priv(tmp, priv, prof);
+	if (ret) {
+		en_warn(priv, "%s: mlx4_en_copy_priv() failed, return\n",
+			__func__);
+		return ret;
+	}
 
 	if (mlx4_en_alloc_resources(tmp)) {
 		en_warn(priv,