[PATCH net-next] bnxt_en: fix kernel panic in queue api functions

[PATCH net-next] bnxt_en: fix kernel panic in queue api functions

[Taehee Yoo]

bnxt_queue_{mem_alloc,start,stop} access bp->rx_ring array and this is
initialized while an interface is being up.
The rings are initialized as a number of channels.

The queue API functions access rx_ring without checking both null and
ring size.
So, if the queue API functions are called when interface status is down,
they access an uninitialized rx_ring array.
Also if the queue index parameter value is larger than a ring, it
would also access an uninitialized rx_ring.

 BUG: kernel NULL pointer dereference, address: 0000000000000000
 #PF: supervisor read access in kernel mode
 #PF: error_code(0x0000) - not-present page
 PGD 0 P4D 0
 Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
 CPU: 1 PID: 1697 Comm: ncdevmem Not tainted 6.10.0-rc5+ #34
 RIP: 0010:bnxt_queue_mem_alloc+0x38/0x410 [bnxt_en]
 Code: 49 89 f5 41 54 4d 89 c4 4d 69 c0 c0 05 00 00 55 48 8d af 40 0a 00 00 53 48 89 fb 48 83 ec 05
 RSP: 0018:ffffa1ad0449ba48 EFLAGS: 00010246
 RAX: ffffffffc04c7710 RBX: ffff9b88aee48000 RCX: 0000000000000000
 RDX: 0000000000000000 RSI: ffff9b8884ba0000 RDI: ffff9b8884ba0008
 RBP: ffff9b88aee48a40 R08: 0000000000000000 R09: ffff9b8884ba6000
 R10: ffffa1ad0449ba88 R11: ffff9b8884ba6000 R12: 0000000000000000
 R13: ffff9b8884ba0000 R14: ffff9b8884ba0000 R15: ffff9b8884ba6000
 FS:  00007f7b2a094740(0000) GS:ffff9b8f9f680000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 0000000000000000 CR3: 000000015f394000 CR4: 00000000007506f0
 PKRU: 55555554
 Call Trace:
  <TASK>
  ? __die+0x20/0x70
  ? page_fault_oops+0x15a/0x460
  ? __vmalloc_node_range_noprof+0x4f7/0x8e0
  ? exc_page_fault+0x6e/0x180
  ? asm_exc_page_fault+0x22/0x30
  ? __pfx_bnxt_queue_mem_alloc+0x10/0x10 [bnxt_en 2b2843e995211f081639d5c0e74fe1cce7fed534]
  ? bnxt_queue_mem_alloc+0x38/0x410 [bnxt_en 2b2843e995211f081639d5c0e74fe1cce7fed534]
  netdev_rx_queue_restart+0xa9/0x1c0
  net_devmem_bind_dmabuf_to_queue+0xcb/0x100
  netdev_nl_bind_rx_doit+0x2f6/0x350
  genl_family_rcv_msg_doit+0xd9/0x130
  genl_rcv_msg+0x184/0x2b0
  ? __pfx_netdev_nl_bind_rx_doit+0x10/0x10
  ? __pfx_genl_rcv_msg+0x10/0x10
  netlink_rcv_skb+0x54/0x100
  genl_rcv+0x24/0x40
  netlink_unicast+0x243/0x370
  netlink_sendmsg+0x1bb/0x3e0

Fixes: 2d694c27d32e ("bnxt_en: implement netdev_queue_mgmt_ops")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

The branch is not net because the commit 2d694c27d32e is not
yet merged into net branch.

devmem TCP causes this problem, but it is not yet merged.
So, to test this patch, please patch the current devmem TCP.
The /tools/testing/selftests/net/ncdevmem will immediately reproduce 
this problem.

 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 6fc34ccb86e3..e270fb6b2866 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -15022,6 +15022,9 @@ static int bnxt_queue_mem_alloc(struct net_device *dev, void *qmem, int idx)
 	struct bnxt_ring_struct *ring;
 	int rc;
 
+	if (!bp->rx_ring || idx >= bp->rx_nr_rings)
+		return -EINVAL;
+
 	rxr = &bp->rx_ring[idx];
 	clone = qmem;
 	memcpy(clone, rxr, sizeof(*rxr));
@@ -15156,6 +15159,9 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
 	struct bnxt_cp_ring_info *cpr;
 	int rc;
 
+	if (!bp->rx_ring || idx >= bp->rx_nr_rings)
+		return -EINVAL;
+
 	rxr = &bp->rx_ring[idx];
 	clone = qmem;
 
@@ -15195,6 +15201,9 @@ static int bnxt_queue_stop(struct net_device *dev, void *qmem, int idx)
 	struct bnxt *bp = netdev_priv(dev);
 	struct bnxt_rx_ring_info *rxr;
 
+	if (!bp->rx_ring || idx >= bp->rx_nr_rings)
+		return -EINVAL;
+
 	rxr = &bp->rx_ring[idx];
 	napi_disable(&rxr->bnapi->napi);
 	bnxt_hwrm_rx_ring_free(bp, rxr, false);
-- 
2.34.1

Re: [PATCH net-next] bnxt_en: fix kernel panic in queue api functions

[Somnath Kotur]

[-- Attachment #1: Type: text/plain, Size: 4527 bytes --]

On Thu, Jul 4, 2024 at 1:12 PM Taehee Yoo <ap420073@gmail.com> wrote:
>
> bnxt_queue_{mem_alloc,start,stop} access bp->rx_ring array and this is
> initialized while an interface is being up.
> The rings are initialized as a number of channels.
>
> The queue API functions access rx_ring without checking both null and
> ring size.
> So, if the queue API functions are called when interface status is down,
> they access an uninitialized rx_ring array.
> Also if the queue index parameter value is larger than a ring, it
> would also access an uninitialized rx_ring.
You might want to rephrase this as  'if the queue index parameter is
greater than the no: of rings created'
>
>  BUG: kernel NULL pointer dereference, address: 0000000000000000
>  #PF: supervisor read access in kernel mode
>  #PF: error_code(0x0000) - not-present page
>  PGD 0 P4D 0
>  Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
>  CPU: 1 PID: 1697 Comm: ncdevmem Not tainted 6.10.0-rc5+ #34
>  RIP: 0010:bnxt_queue_mem_alloc+0x38/0x410 [bnxt_en]
>  Code: 49 89 f5 41 54 4d 89 c4 4d 69 c0 c0 05 00 00 55 48 8d af 40 0a 00 00 53 48 89 fb 48 83 ec 05
>  RSP: 0018:ffffa1ad0449ba48 EFLAGS: 00010246
>  RAX: ffffffffc04c7710 RBX: ffff9b88aee48000 RCX: 0000000000000000
>  RDX: 0000000000000000 RSI: ffff9b8884ba0000 RDI: ffff9b8884ba0008
>  RBP: ffff9b88aee48a40 R08: 0000000000000000 R09: ffff9b8884ba6000
>  R10: ffffa1ad0449ba88 R11: ffff9b8884ba6000 R12: 0000000000000000
>  R13: ffff9b8884ba0000 R14: ffff9b8884ba0000 R15: ffff9b8884ba6000
>  FS:  00007f7b2a094740(0000) GS:ffff9b8f9f680000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 0000000000000000 CR3: 000000015f394000 CR4: 00000000007506f0
>  PKRU: 55555554
>  Call Trace:
>   <TASK>
>   ? __die+0x20/0x70
>   ? page_fault_oops+0x15a/0x460
>   ? __vmalloc_node_range_noprof+0x4f7/0x8e0
>   ? exc_page_fault+0x6e/0x180
>   ? asm_exc_page_fault+0x22/0x30
>   ? __pfx_bnxt_queue_mem_alloc+0x10/0x10 [bnxt_en 2b2843e995211f081639d5c0e74fe1cce7fed534]
>   ? bnxt_queue_mem_alloc+0x38/0x410 [bnxt_en 2b2843e995211f081639d5c0e74fe1cce7fed534]
>   netdev_rx_queue_restart+0xa9/0x1c0
>   net_devmem_bind_dmabuf_to_queue+0xcb/0x100
>   netdev_nl_bind_rx_doit+0x2f6/0x350
>   genl_family_rcv_msg_doit+0xd9/0x130
>   genl_rcv_msg+0x184/0x2b0
>   ? __pfx_netdev_nl_bind_rx_doit+0x10/0x10
>   ? __pfx_genl_rcv_msg+0x10/0x10
>   netlink_rcv_skb+0x54/0x100
>   genl_rcv+0x24/0x40
>   netlink_unicast+0x243/0x370
>   netlink_sendmsg+0x1bb/0x3e0
>
> Fixes: 2d694c27d32e ("bnxt_en: implement netdev_queue_mgmt_ops")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---
>
> The branch is not net because the commit 2d694c27d32e is not
> yet merged into net branch.
>
> devmem TCP causes this problem, but it is not yet merged.
> So, to test this patch, please patch the current devmem TCP.
> The /tools/testing/selftests/net/ncdevmem will immediately reproduce
> this problem.
>
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 6fc34ccb86e3..e270fb6b2866 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -15022,6 +15022,9 @@ static int bnxt_queue_mem_alloc(struct net_device *dev, void *qmem, int idx)
>         struct bnxt_ring_struct *ring;
>         int rc;
>
> +       if (!bp->rx_ring || idx >= bp->rx_nr_rings)
> +               return -EINVAL;
> +
>         rxr = &bp->rx_ring[idx];
>         clone = qmem;
>         memcpy(clone, rxr, sizeof(*rxr));
> @@ -15156,6 +15159,9 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
>         struct bnxt_cp_ring_info *cpr;
>         int rc;
>
> +       if (!bp->rx_ring || idx >= bp->rx_nr_rings)
> +               return -EINVAL;
> +
>         rxr = &bp->rx_ring[idx];
>         clone = qmem;
>
> @@ -15195,6 +15201,9 @@ static int bnxt_queue_stop(struct net_device *dev, void *qmem, int idx)
>         struct bnxt *bp = netdev_priv(dev);
>         struct bnxt_rx_ring_info *rxr;
>
> +       if (!bp->rx_ring || idx >= bp->rx_nr_rings)
> +               return -EINVAL;
> +
>         rxr = &bp->rx_ring[idx];
>         napi_disable(&rxr->bnapi->napi);
>         bnxt_hwrm_rx_ring_free(bp, rxr, false);
> --
> 2.34.1
>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH net-next] bnxt_en: fix kernel panic in queue api functions

[Jakub Kicinski]

On Thu,  4 Jul 2024 07:41:53 +0000 Taehee Yoo wrote:
> bnxt_queue_{mem_alloc,start,stop} access bp->rx_ring array and this is
> initialized while an interface is being up.
> The rings are initialized as a number of channels.
> 
> The queue API functions access rx_ring without checking both null and
> ring size.
> So, if the queue API functions are called when interface status is down,
> they access an uninitialized rx_ring array.
> Also if the queue index parameter value is larger than a ring, it
> would also access an uninitialized rx_ring.

Shouldn't the core be checking against dev->real_num_rx_queues instead ?

Re: [PATCH net-next] bnxt_en: fix kernel panic in queue api functions

[Taehee Yoo]

On Thu, Jul 4, 2024 at 10:33 PM Jakub Kicinski <kuba@kernel.org> wrote:
>

Hi Jakub,
Thanks a lot for review!


> On Thu, 4 Jul 2024 07:41:53 +0000 Taehee Yoo wrote:
> > bnxt_queue_{mem_alloc,start,stop} access bp->rx_ring array and this is
> > initialized while an interface is being up.
> > The rings are initialized as a number of channels.
> >
> > The queue API functions access rx_ring without checking both null and
> > ring size.
> > So, if the queue API functions are called when interface status is down,
> > they access an uninitialized rx_ring array.
> > Also if the queue index parameter value is larger than a ring, it
> > would also access an uninitialized rx_ring.
>
> Shouldn't the core be checking against dev->real_num_rx_queues instead ?

Oh, I missed it.
I agree the core should check dev->real_num_rx_queues.
But the current devmem TCP code checks dev->num_rx_queues instead of
dev->real_num_rx_queues.
I tested the below change, and it works well.
So, I will comment on it in Mina's patch.

diff --git a/net/core/devmem.c b/net/core/devmem.c
index 7afaf17801ef..da27778c2421 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -146,7 +146,7 @@ int net_devmem_bind_dmabuf_to_queue(struct
net_device *dev, u32 rxq_idx,
u32 xa_idx;
int err;

- if (rxq_idx >= dev->num_rx_queues)
+ if (rxq_idx >= dev->real_num_rx_queues)
return -ERANGE;

rxq = __netif_get_rx_queue(dev, rxq_idx);


Thanks a lot!
Taehee Yoo

Re: [PATCH net-next] bnxt_en: fix kernel panic in queue api functions

[David Wei]

On 2024-07-04 00:41, Taehee Yoo wrote:
> bnxt_queue_{mem_alloc,start,stop} access bp->rx_ring array and this is
> initialized while an interface is being up.
> The rings are initialized as a number of channels.
> 
> The queue API functions access rx_ring without checking both null and
> ring size.
> So, if the queue API functions are called when interface status is down,
> they access an uninitialized rx_ring array.
> Also if the queue index parameter value is larger than a ring, it
> would also access an uninitialized rx_ring.

Hi Taehee, I deliberately didn't do a check as it is too defensive. The
caller of the queue API e.g. netdev_rx_queue_restart() should be the one
doing the check instead.

> 
>  BUG: kernel NULL pointer dereference, address: 0000000000000000
>  #PF: supervisor read access in kernel mode
>  #PF: error_code(0x0000) - not-present page
>  PGD 0 P4D 0
>  Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
>  CPU: 1 PID: 1697 Comm: ncdevmem Not tainted 6.10.0-rc5+ #34
>  RIP: 0010:bnxt_queue_mem_alloc+0x38/0x410 [bnxt_en]
>  Code: 49 89 f5 41 54 4d 89 c4 4d 69 c0 c0 05 00 00 55 48 8d af 40 0a 00 00 53 48 89 fb 48 83 ec 05
>  RSP: 0018:ffffa1ad0449ba48 EFLAGS: 00010246
>  RAX: ffffffffc04c7710 RBX: ffff9b88aee48000 RCX: 0000000000000000
>  RDX: 0000000000000000 RSI: ffff9b8884ba0000 RDI: ffff9b8884ba0008
>  RBP: ffff9b88aee48a40 R08: 0000000000000000 R09: ffff9b8884ba6000
>  R10: ffffa1ad0449ba88 R11: ffff9b8884ba6000 R12: 0000000000000000
>  R13: ffff9b8884ba0000 R14: ffff9b8884ba0000 R15: ffff9b8884ba6000
>  FS:  00007f7b2a094740(0000) GS:ffff9b8f9f680000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 0000000000000000 CR3: 000000015f394000 CR4: 00000000007506f0
>  PKRU: 55555554
>  Call Trace:
>   <TASK>
>   ? __die+0x20/0x70
>   ? page_fault_oops+0x15a/0x460
>   ? __vmalloc_node_range_noprof+0x4f7/0x8e0
>   ? exc_page_fault+0x6e/0x180
>   ? asm_exc_page_fault+0x22/0x30
>   ? __pfx_bnxt_queue_mem_alloc+0x10/0x10 [bnxt_en 2b2843e995211f081639d5c0e74fe1cce7fed534]
>   ? bnxt_queue_mem_alloc+0x38/0x410 [bnxt_en 2b2843e995211f081639d5c0e74fe1cce7fed534]
>   netdev_rx_queue_restart+0xa9/0x1c0
>   net_devmem_bind_dmabuf_to_queue+0xcb/0x100
>   netdev_nl_bind_rx_doit+0x2f6/0x350
>   genl_family_rcv_msg_doit+0xd9/0x130
>   genl_rcv_msg+0x184/0x2b0
>   ? __pfx_netdev_nl_bind_rx_doit+0x10/0x10
>   ? __pfx_genl_rcv_msg+0x10/0x10
>   netlink_rcv_skb+0x54/0x100
>   genl_rcv+0x24/0x40
>   netlink_unicast+0x243/0x370
>   netlink_sendmsg+0x1bb/0x3e0
> 
> Fixes: 2d694c27d32e ("bnxt_en: implement netdev_queue_mgmt_ops")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---
> 
> The branch is not net because the commit 2d694c27d32e is not
> yet merged into net branch.
> 
> devmem TCP causes this problem, but it is not yet merged.
> So, to test this patch, please patch the current devmem TCP.
> The /tools/testing/selftests/net/ncdevmem will immediately reproduce 
> this problem.
> 
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 6fc34ccb86e3..e270fb6b2866 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -15022,6 +15022,9 @@ static int bnxt_queue_mem_alloc(struct net_device *dev, void *qmem, int idx)
>  	struct bnxt_ring_struct *ring;
>  	int rc;
>  
> +	if (!bp->rx_ring || idx >= bp->rx_nr_rings)
> +		return -EINVAL;
> +
>  	rxr = &bp->rx_ring[idx];
>  	clone = qmem;
>  	memcpy(clone, rxr, sizeof(*rxr));
> @@ -15156,6 +15159,9 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
>  	struct bnxt_cp_ring_info *cpr;
>  	int rc;
>  
> +	if (!bp->rx_ring || idx >= bp->rx_nr_rings)
> +		return -EINVAL;
> +
>  	rxr = &bp->rx_ring[idx];
>  	clone = qmem;
>  
> @@ -15195,6 +15201,9 @@ static int bnxt_queue_stop(struct net_device *dev, void *qmem, int idx)
>  	struct bnxt *bp = netdev_priv(dev);
>  	struct bnxt_rx_ring_info *rxr;
>  
> +	if (!bp->rx_ring || idx >= bp->rx_nr_rings)
> +		return -EINVAL;
> +
>  	rxr = &bp->rx_ring[idx];
>  	napi_disable(&rxr->bnapi->napi);
>  	bnxt_hwrm_rx_ring_free(bp, rxr, false);