diff mbox series

[net-next] bnxt_en: fix accessing vnic_info before allocating it

Message ID 20240226144911.1297336-1-aleksander.lobakin@intel.com (mailing list archive)
State Accepted
Commit c4b04a802d8e3996e588cbbb47756b2f9d239d78
Delegated to: Netdev Maintainers
Headers show
Series [net-next] bnxt_en: fix accessing vnic_info before allocating it | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 940 this patch: 940
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 957 this patch: 957
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 959 this patch: 959
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 18 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-27--03-00 (tests: 1456)

Commit Message

Alexander Lobakin Feb. 26, 2024, 2:49 p.m. UTC 
bnxt_alloc_mem() dereferences ::vnic_info in the variable declaration
block, but allocates it much later. As a result, the following crash
happens on my setup:

 BUG: kernel NULL pointer dereference, address: 0000000000000090
 fbcon: Taking over console
 #PF: supervisor write access in kernel mode
 #PF: error_code (0x0002) - not-present page
 PGD 12f382067 P4D 0
 Oops: 8002 [#1] PREEMPT SMP NOPTI
 CPU: 47 PID: 2516 Comm: NetworkManager Not tainted 6.8.0-rc5-libeth+ #49
 Hardware name: Intel Corporation M50CYP2SBSTD/M58CYP2SBSTD, BIOS SE5C620.86B.01.01.0088.2305172341 05/17/2023
 RIP: 0010:bnxt_alloc_mem+0x1609/0x1910 [bnxt_en]
 Code: 81 c8 48 83 c8 08 31 c9 e9 d7 fe ff ff c7 44 24 Oc 00 00 00 00 49 89 d5 e9 2d fe ff ff 41 89 c6 e9 88 00 00 00 48 8b 44 24 50 <80> 88 90 00 00 00 Od 8b 43 74 a8 02 75 1e f6 83 14 02 00 00 80 74
 RSP: 0018:ff3f25580f3432c8 EFLAGS: 00010246
 RAX: 0000000000000000 RBX: ff15a5cfc45249e0 RCX: 0000002079777000
 RDX: ff15a5dfb9767000 RSI: 0000000000000000 RDI: 0000000000000000
 RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
 R10: ff15a5dfb9777000 R11: ffffff8000000000 R12: 0000000000000000
 R13: 0000000000000000 R14: 0000000000000020 R15: ff15a5cfce34f540
 FS:  000007fb9a160500(0000) GS:ff15a5dfbefc0000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CRO: 0000000080050033
 CR2: 0000000000000090 CR3: 0000000109efc00Z CR4: 0000000000771ef0
 DR0: 0000000000000000 DR1: 0000000000000000 DRZ: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 PKRU: 55555554

 Call Trace:
 <TASK>
 ? __die_body+0x68/0xb0
 ? page_fault_oops+0x3a6/0x400
 ? exc_page_fault+0x7a/0x1b0
 ? asm_exc_page_fault+0x26/8x30
 ? bnxt_alloc_mem+0x1609/0x1910 [bnxt_en]
 ? bnxt_alloc_mem+0x1389/8x1918 [bnxt_en]
 _bnxt_open_nic+0x198/0xa50 [bnxt_en]
 ? bnxt_hurm_if_change+0x287/0x3d0 [bnxt_en]
 bnxt_open+0xeb/0x1b0 [bnxt_en]
 _dev_open+0x12e/0x1f0
 _dev_change_flags+0xb0/0x200
 dev_change_flags+0x25/0x60
 do_setlink+0x463/0x1260
 ? sock_def_readable+0x14/0xc0
 ? rtnl_getlink+0x4b9/0x590
 ? _nla_validate_parse+0x91/0xfa0
 rtnl_newlink+0xbac/0xe40
 <...>

Don't create a variable and dereference the first array member directly
since it's used only once in the code.

Fixes: ef4ee64e9990 ("bnxt_en: Define BNXT_VNIC_DEFAULT for the default vnic index")
Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Jiri Pirko Feb. 26, 2024, 3:43 p.m. UTC | #1 
Mon, Feb 26, 2024 at 03:49:11PM CET, aleksander.lobakin@intel.com wrote:
>bnxt_alloc_mem() dereferences ::vnic_info in the variable declaration
>block, but allocates it much later. As a result, the following crash
>happens on my setup:
>
> BUG: kernel NULL pointer dereference, address: 0000000000000090
> fbcon: Taking over console
> #PF: supervisor write access in kernel mode
> #PF: error_code (0x0002) - not-present page
> PGD 12f382067 P4D 0
> Oops: 8002 [#1] PREEMPT SMP NOPTI
> CPU: 47 PID: 2516 Comm: NetworkManager Not tainted 6.8.0-rc5-libeth+ #49
> Hardware name: Intel Corporation M50CYP2SBSTD/M58CYP2SBSTD, BIOS SE5C620.86B.01.01.0088.2305172341 05/17/2023
> RIP: 0010:bnxt_alloc_mem+0x1609/0x1910 [bnxt_en]
> Code: 81 c8 48 83 c8 08 31 c9 e9 d7 fe ff ff c7 44 24 Oc 00 00 00 00 49 89 d5 e9 2d fe ff ff 41 89 c6 e9 88 00 00 00 48 8b 44 24 50 <80> 88 90 00 00 00 Od 8b 43 74 a8 02 75 1e f6 83 14 02 00 00 80 74
> RSP: 0018:ff3f25580f3432c8 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ff15a5cfc45249e0 RCX: 0000002079777000
> RDX: ff15a5dfb9767000 RSI: 0000000000000000 RDI: 0000000000000000
> RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> R10: ff15a5dfb9777000 R11: ffffff8000000000 R12: 0000000000000000
> R13: 0000000000000000 R14: 0000000000000020 R15: ff15a5cfce34f540
> FS:  000007fb9a160500(0000) GS:ff15a5dfbefc0000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CRO: 0000000080050033
> CR2: 0000000000000090 CR3: 0000000109efc00Z CR4: 0000000000771ef0
> DR0: 0000000000000000 DR1: 0000000000000000 DRZ: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> PKRU: 55555554
>
> Call Trace:
> <TASK>
> ? __die_body+0x68/0xb0
> ? page_fault_oops+0x3a6/0x400
> ? exc_page_fault+0x7a/0x1b0
> ? asm_exc_page_fault+0x26/8x30
> ? bnxt_alloc_mem+0x1609/0x1910 [bnxt_en]
> ? bnxt_alloc_mem+0x1389/8x1918 [bnxt_en]
> _bnxt_open_nic+0x198/0xa50 [bnxt_en]
> ? bnxt_hurm_if_change+0x287/0x3d0 [bnxt_en]
> bnxt_open+0xeb/0x1b0 [bnxt_en]
> _dev_open+0x12e/0x1f0
> _dev_change_flags+0xb0/0x200
> dev_change_flags+0x25/0x60
> do_setlink+0x463/0x1260
> ? sock_def_readable+0x14/0xc0
> ? rtnl_getlink+0x4b9/0x590
> ? _nla_validate_parse+0x91/0xfa0
> rtnl_newlink+0xbac/0xe40
> <...>
>
>Don't create a variable and dereference the first array member directly
>since it's used only once in the code.
>
>Fixes: ef4ee64e9990 ("bnxt_en: Define BNXT_VNIC_DEFAULT for the default vnic index")

Nice example how things may go off when patch is doing multiple things
at once :)


>Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Michael Chan Feb. 26, 2024, 4:26 p.m. UTC | #2 
On Mon, Feb 26, 2024 at 6:51 AM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> bnxt_alloc_mem() dereferences ::vnic_info in the variable declaration
> block, but allocates it much later. As a result, the following crash
> happens on my setup:
>
>  BUG: kernel NULL pointer dereference, address: 0000000000000090
>  fbcon: Taking over console
>  #PF: supervisor write access in kernel mode
>  #PF: error_code (0x0002) - not-present page
>  PGD 12f382067 P4D 0
>  Oops: 8002 [#1] PREEMPT SMP NOPTI
>  CPU: 47 PID: 2516 Comm: NetworkManager Not tainted 6.8.0-rc5-libeth+ #49
>  Hardware name: Intel Corporation M50CYP2SBSTD/M58CYP2SBSTD, BIOS SE5C620.86B.01.01.0088.2305172341 05/17/2023
>  RIP: 0010:bnxt_alloc_mem+0x1609/0x1910 [bnxt_en]
>  Code: 81 c8 48 83 c8 08 31 c9 e9 d7 fe ff ff c7 44 24 Oc 00 00 00 00 49 89 d5 e9 2d fe ff ff 41 89 c6 e9 88 00 00 00 48 8b 44 24 50 <80> 88 90 00 00 00 Od 8b 43 74 a8 02 75 1e f6 83 14 02 00 00 80 74
>  RSP: 0018:ff3f25580f3432c8 EFLAGS: 00010246
>  RAX: 0000000000000000 RBX: ff15a5cfc45249e0 RCX: 0000002079777000
>  RDX: ff15a5dfb9767000 RSI: 0000000000000000 RDI: 0000000000000000
>  RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
>  R10: ff15a5dfb9777000 R11: ffffff8000000000 R12: 0000000000000000
>  R13: 0000000000000000 R14: 0000000000000020 R15: ff15a5cfce34f540
>  FS:  000007fb9a160500(0000) GS:ff15a5dfbefc0000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CRO: 0000000080050033
>  CR2: 0000000000000090 CR3: 0000000109efc00Z CR4: 0000000000771ef0
>  DR0: 0000000000000000 DR1: 0000000000000000 DRZ: 0000000000000000
>  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>  PKRU: 55555554
>
>  Call Trace:
>  <TASK>
>  ? __die_body+0x68/0xb0
>  ? page_fault_oops+0x3a6/0x400
>  ? exc_page_fault+0x7a/0x1b0
>  ? asm_exc_page_fault+0x26/8x30
>  ? bnxt_alloc_mem+0x1609/0x1910 [bnxt_en]
>  ? bnxt_alloc_mem+0x1389/8x1918 [bnxt_en]
>  _bnxt_open_nic+0x198/0xa50 [bnxt_en]
>  ? bnxt_hurm_if_change+0x287/0x3d0 [bnxt_en]
>  bnxt_open+0xeb/0x1b0 [bnxt_en]
>  _dev_open+0x12e/0x1f0
>  _dev_change_flags+0xb0/0x200
>  dev_change_flags+0x25/0x60
>  do_setlink+0x463/0x1260
>  ? sock_def_readable+0x14/0xc0
>  ? rtnl_getlink+0x4b9/0x590
>  ? _nla_validate_parse+0x91/0xfa0
>  rtnl_newlink+0xbac/0xe40
>  <...>
>
> Don't create a variable and dereference the first array member directly
> since it's used only once in the code.
>
> Fixes: ef4ee64e9990 ("bnxt_en: Define BNXT_VNIC_DEFAULT for the default vnic index")
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>

Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
patchwork-bot+netdevbpf@kernel.org Feb. 27, 2024, 4:40 p.m. UTC | #3 
Hello:

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

On Mon, 26 Feb 2024 15:49:11 +0100 you wrote:
> bnxt_alloc_mem() dereferences ::vnic_info in the variable declaration
> block, but allocates it much later. As a result, the following crash
> happens on my setup:
> 
>  BUG: kernel NULL pointer dereference, address: 0000000000000090
>  fbcon: Taking over console
>  #PF: supervisor write access in kernel mode
>  #PF: error_code (0x0002) - not-present page
>  PGD 12f382067 P4D 0
>  Oops: 8002 [#1] PREEMPT SMP NOPTI
>  CPU: 47 PID: 2516 Comm: NetworkManager Not tainted 6.8.0-rc5-libeth+ #49
>  Hardware name: Intel Corporation M50CYP2SBSTD/M58CYP2SBSTD, BIOS SE5C620.86B.01.01.0088.2305172341 05/17/2023
>  RIP: 0010:bnxt_alloc_mem+0x1609/0x1910 [bnxt_en]
>  Code: 81 c8 48 83 c8 08 31 c9 e9 d7 fe ff ff c7 44 24 Oc 00 00 00 00 49 89 d5 e9 2d fe ff ff 41 89 c6 e9 88 00 00 00 48 8b 44 24 50 <80> 88 90 00 00 00 Od 8b 43 74 a8 02 75 1e f6 83 14 02 00 00 80 74
>  RSP: 0018:ff3f25580f3432c8 EFLAGS: 00010246
>  RAX: 0000000000000000 RBX: ff15a5cfc45249e0 RCX: 0000002079777000
>  RDX: ff15a5dfb9767000 RSI: 0000000000000000 RDI: 0000000000000000
>  RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
>  R10: ff15a5dfb9777000 R11: ffffff8000000000 R12: 0000000000000000
>  R13: 0000000000000000 R14: 0000000000000020 R15: ff15a5cfce34f540
>  FS:  000007fb9a160500(0000) GS:ff15a5dfbefc0000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CRO: 0000000080050033
>  CR2: 0000000000000090 CR3: 0000000109efc00Z CR4: 0000000000771ef0
>  DR0: 0000000000000000 DR1: 0000000000000000 DRZ: 0000000000000000
>  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>  PKRU: 55555554
> 
> [...]

Here is the summary with links:
  - [net-next] bnxt_en: fix accessing vnic_info before allocating it
    https://git.kernel.org/netdev/net-next/c/c4b04a802d8e

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 9968d67e6c77..a15e6d31fc22 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -5004,7 +5004,6 @@  static void bnxt_free_mem(struct bnxt *bp, bool irq_re_init)
 
 static int bnxt_alloc_mem(struct bnxt *bp, bool irq_re_init)
 {
-	struct bnxt_vnic_info *vnic0 = &bp->vnic_info[BNXT_VNIC_DEFAULT];
 	int i, j, rc, size, arr_size;
 	void *bnapi;
 
@@ -5133,8 +5132,9 @@  static int bnxt_alloc_mem(struct bnxt *bp, bool irq_re_init)
 	if (rc)
 		goto alloc_mem_err;
 
-	vnic0->flags |= BNXT_VNIC_RSS_FLAG | BNXT_VNIC_MCAST_FLAG |
-			BNXT_VNIC_UCAST_FLAG;
+	bp->vnic_info[BNXT_VNIC_DEFAULT].flags |= BNXT_VNIC_RSS_FLAG |
+						  BNXT_VNIC_MCAST_FLAG |
+						  BNXT_VNIC_UCAST_FLAG;
 	if (BNXT_SUPPORTS_NTUPLE_VNIC(bp) && (bp->flags & BNXT_FLAG_RFS))
 		bp->vnic_info[BNXT_VNIC_NTUPLE].flags |=
 			BNXT_VNIC_RSS_FLAG | BNXT_VNIC_NTUPLE_FLAG;