netdev - Re: [PATCH net] eth: bnxt: always recalculate features after XDP clearing, fix null-deref

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACKFLi=y1Geua16t+u5E-A9iL5mgPNEE_yLTHEeFk0ZkVdBoAw@mail.gmail.com>
Date: Wed, 8 Jan 2025 21:38:31 -0800
From: Michael Chan <michael.chan@...adcom.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com, 
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org, 
	daniel@...earbox.net, hawk@...nel.org, john.fastabend@...il.com, 
	andrew.gospodarek@...adcom.com, pavan.chebbi@...adcom.com
Subject: Re: [PATCH net] eth: bnxt: always recalculate features after XDP
 clearing, fix null-deref

On Wed, Jan 8, 2025 at 8:31 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> Recalculate features when XDP is detached.
>
> Before:
>   # ip li set dev eth0 xdp obj xdp_dummy.bpf.o sec xdp
>   # ip li set dev eth0 xdp off
>   # ethtool -k eth0 | grep gro
>   rx-gro-hw: off [requested on]
>
> After:
>   # ip li set dev eth0 xdp obj xdp_dummy.bpf.o sec xdp
>   # ip li set dev eth0 xdp off
>   # ethtool -k eth0 | grep gro
>   rx-gro-hw: on
>
> The fact that HW-GRO doesn't get re-enabled automatically is just
> a minor annoyance.

I knew about the annoyance, but didn't know about this possible crash.

> The real issue is that the features will randomly
> come back during another reconfiguration which just happens to invoke
> netdev_update_features(). The driver doesn't handle reconfiguring
> two things at a time very robustly.
>
> Starting with commit 98ba1d931f61 ("bnxt_en: Fix RSS logic in
> __bnxt_reserve_rings()") we only reconfigure the RSS hash table
> if the "effective" number of Rx rings has changed. If HW-GRO is
> enabled "effective" number of rings is 2x what user sees.
> So if we are in the bad state, with HW-GRO re-enablement "pending"
> after XDP off, and we lower the rings by / 2 - the HW-GRO rings
> doing 2x and the ethtool -L doing / 2 may cancel each other out,
> and the:
>
>   if (old_rx_rings != bp->hw_resc.resv_rx_rings &&
>
> condition in __bnxt_reserve_rings() will be false.
> The RSS map won't get updated, and we'll crash with:
>
>   BUG: kernel NULL pointer dereference, address: 0000000000000168
>   RIP: 0010:__bnxt_hwrm_vnic_set_rss+0x13a/0x1a0
>     bnxt_hwrm_vnic_rss_cfg_p5+0x47/0x180
>     __bnxt_setup_vnic_p5+0x58/0x110
>     bnxt_init_nic+0xb72/0xf50
>     __bnxt_open_nic+0x40d/0xab0
>     bnxt_open_nic+0x2b/0x60
>     ethtool_set_channels+0x18c/0x1d0
>
> As we try to access a freed ring.
>
> The issue is present since XDP support was added, really, but
> prior to commit 98ba1d931f61 ("bnxt_en: Fix RSS logic in
> __bnxt_reserve_rings()") it wasn't causing major issues.
>
> Fixes: 1054aee82321 ("bnxt_en: Use NETIF_F_GRO_HW.")
> Fixes: 98ba1d931f61 ("bnxt_en: Fix RSS logic in __bnxt_reserve_rings()")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

Thanks for the patch.
Reviewed-by: Michael Chan <michael.chan@...adcom.com>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)