| Message ID | 20240907190341.162289-1-aha310510@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a7789fd4caaf96ecfed5e28c4cddb927e6bebadb |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: hsr: prevent NULL pointer dereference in hsr_proxy_announce() | expand |
On Sun, Sep 08, 2024 at 04:03:41AM +0900, Jeongjun Park wrote: > In the function hsr_proxy_annouance() added in the previous commit > 5f703ce5c981 ("net: hsr: Send supervisory frames to HSR network > with ProxyNodeTable data"), the return value of the hsr_port_get_hsr() > function is not checked to be a NULL pointer, which causes a NULL > pointer dereference. > > To solve this, we need to add code to check whether the return value > of hsr_port_get_hsr() is NULL. > > Reported-by: syzbot+02a42d9b1bd395cbcab4@syzkaller.appspotmail.com > Fixes: 5f703ce5c981 ("net: hsr: Send supervisory frames to HSR network with ProxyNodeTable data") > Signed-off-by: Jeongjun Park <aha310510@gmail.com> Thanks, I agree with your analysis; that the cited commit introduced this problem; and that this is an appropriate solution. Reviewed-by: Simon Horman <horms@kernel.org>
Hi Jeongjun, > In the function hsr_proxy_annouance() added in the previous commit > 5f703ce5c981 ("net: hsr: Send supervisory frames to HSR network > with ProxyNodeTable data"), the return value of the > hsr_port_get_hsr() function is not checked to be a NULL pointer, > which causes a NULL pointer dereference. Thank you for your patch. The code in hsr_proxy_announcement() is _only_ executed (the timer is configured to trigger this function) when hsr->redbox is set, which means that somebody has called earlier iproute2 command: ip link add name hsr1 type hsr slave1 lan4 slave2 lan5 interlink lan3 supervision 45 version 1 > > To solve this, we need to add code to check whether the return value > of hsr_port_get_hsr() is NULL. > > Reported-by: syzbot+02a42d9b1bd395cbcab4@syzkaller.appspotmail.com > Fixes: 5f703ce5c981 ("net: hsr: Send supervisory frames to HSR > network with ProxyNodeTable data") Signed-off-by: Jeongjun Park > <aha310510@gmail.com> --- > net/hsr/hsr_device.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c > index e4cc6b78dcfc..b3191968e53a 100644 > --- a/net/hsr/hsr_device.c > +++ b/net/hsr/hsr_device.c > @@ -427,6 +427,9 @@ static void hsr_proxy_announce(struct timer_list > *t) > * of SAN nodes stored in ProxyNodeTable. > */ > interlink = hsr_port_get_hsr(hsr, HSR_PT_INTERLINK); > + if (!interlink) > + goto done; > + > list_for_each_entry_rcu(node, &hsr->proxy_node_db, mac_list) > { if (hsr_addr_is_redbox(hsr, node->macaddress_A)) > continue; > @@ -441,6 +444,7 @@ static void hsr_proxy_announce(struct timer_list > *t) mod_timer(&hsr->announce_proxy_timer, jiffies + interval); > } > > +done: > rcu_read_unlock(); > } > > -- Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Mon, 9 Sep 2024 10:58:22 +0200 Lukasz Majewski wrote: > > In the function hsr_proxy_annouance() added in the previous commit > > 5f703ce5c981 ("net: hsr: Send supervisory frames to HSR network > > with ProxyNodeTable data"), the return value of the > > hsr_port_get_hsr() function is not checked to be a NULL pointer, > > which causes a NULL pointer dereference. > > Thank you for your patch. > > The code in hsr_proxy_announcement() is _only_ executed (the timer is > configured to trigger this function) when hsr->redbox is set, which > means that somebody has called earlier iproute2 command: > > ip link add name hsr1 type hsr slave1 lan4 slave2 lan5 interlink lan3 > supervision 45 version 1 Are you trying to say the patch is correct or incorrect? The structs have no refcounting - should the timers be deleted with _sync() inside hsr_check_announce()?
Hi Jakub, > On Mon, 9 Sep 2024 10:58:22 +0200 Lukasz Majewski wrote: > > > In the function hsr_proxy_annouance() added in the previous > > > commit 5f703ce5c981 ("net: hsr: Send supervisory frames to HSR > > > network with ProxyNodeTable data"), the return value of the > > > hsr_port_get_hsr() function is not checked to be a NULL pointer, > > > which causes a NULL pointer dereference. > > > > Thank you for your patch. > > > > The code in hsr_proxy_announcement() is _only_ executed (the timer > > is configured to trigger this function) when hsr->redbox is set, > > which means that somebody has called earlier iproute2 command: > > > > ip link add name hsr1 type hsr slave1 lan4 slave2 lan5 interlink > > lan3 supervision 45 version 1 > > Are you trying to say the patch is correct or incorrect? I'm just trying to explain that this code (i.e. hsr_proxy_announcement()) shall NOT be trigger if the interlink port is not configured. Nonetheless the patch is correct - as it was pointed out that the return value is not checked. > The structs have no refcounting - should the timers be deleted with > _sync() inside hsr_check_announce()? The timers don't need to be conditionally enabled (and removed) as we discussed it previously (as they only do useful work when they are configured and almost take no resources when declared during the driver probe). Anyway: Acked-by: Lukasz Majewski <lukma@denx.de> Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
+ Edward Adam Davis, syzbot+c229849f5b6c82eba3c2 On Wed, Sep 11, 2024 at 10:00:07AM +0200, Lukasz Majewski wrote: > Hi Jakub, > > > On Mon, 9 Sep 2024 10:58:22 +0200 Lukasz Majewski wrote: > > > > In the function hsr_proxy_annouance() added in the previous > > > > commit 5f703ce5c981 ("net: hsr: Send supervisory frames to HSR > > > > network with ProxyNodeTable data"), the return value of the > > > > hsr_port_get_hsr() function is not checked to be a NULL pointer, > > > > which causes a NULL pointer dereference. > > > > > > Thank you for your patch. > > > > > > The code in hsr_proxy_announcement() is _only_ executed (the timer > > > is configured to trigger this function) when hsr->redbox is set, > > > which means that somebody has called earlier iproute2 command: > > > > > > ip link add name hsr1 type hsr slave1 lan4 slave2 lan5 interlink > > > lan3 supervision 45 version 1 > > > > Are you trying to say the patch is correct or incorrect? > > I'm just trying to explain that this code (i.e. > hsr_proxy_announcement()) shall NOT be trigger if the interlink port is > not configured. > > Nonetheless the patch is correct - as it was pointed out that the return > value is not checked. > > > The structs have no refcounting - should the timers be deleted with > > _sync() inside hsr_check_announce()? > > The timers don't need to be conditionally enabled (and removed) as we > discussed it previously (as they only do useful work when they are > configured and almost take no resources when declared during the > driver probe). > > Anyway: > > Acked-by: Lukasz Majewski <lukma@denx.de> Thanks, Like Jakub I was a little confused about the intent of your previous comment, but it is clear now. It seems that along the way the patch got marked as rejected, presumably on the basis of earlier discussion in this thread. But that seems inappropriate now, so let me see if this will bring it back under consideration. pw-bot: under-review For reference, the same change was also submitted as: - [PATCH net] net: hsr: Fix null-ptr-deref in hsr_proxy_announce https://lore.kernel.org/all/tencent_CF67CC46D7D2DBC677898AEEFBAECD0CAB06@qq.com/ I will attempt to somehow mark that as a duplicate in patchwork. It also seems that there are duplicate syzbot reports for this problem [1][2] I will also attempt to mark [2] as a duplicate of [1]. [1] https://syzkaller.appspot.com/bug?extid=02a42d9b1bd395cbcab4 [2] https://syzkaller.appspot.com/bug?extid=c229849f5b6c82eba3c2
On Wed, 11 Sep 2024 10:00:07 +0200 Lukasz Majewski wrote: > > The structs have no refcounting - should the timers be deleted with > > _sync() inside hsr_check_announce()? > > The timers don't need to be conditionally enabled (and removed) as we > discussed it previously (as they only do useful work when they are > configured and almost take no resources when declared during the > driver probe). My concern is admittedly quite theoretical, and perhaps completely impossible given current RCU implementation. But what I was saying is that timer may be running, and interrupted by a very long running interrupt, say on CPU 0. Then, say, we unregister and free hsr_dev on CPU 1. When CPU 0 resumes running the timer code it will UAF on hsr_dev. Again, probably completely theoretical. > Anyway: > > Acked-by: Lukasz Majewski <lukma@denx.de> Thanks!
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Sun, 8 Sep 2024 04:03:41 +0900 you wrote: > In the function hsr_proxy_annouance() added in the previous commit > 5f703ce5c981 ("net: hsr: Send supervisory frames to HSR network > with ProxyNodeTable data"), the return value of the hsr_port_get_hsr() > function is not checked to be a NULL pointer, which causes a NULL > pointer dereference. > > To solve this, we need to add code to check whether the return value > of hsr_port_get_hsr() is NULL. > > [...] Here is the summary with links: - [net] net: hsr: prevent NULL pointer dereference in hsr_proxy_announce() https://git.kernel.org/netdev/net/c/a7789fd4caaf You are awesome, thank you!
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c index e4cc6b78dcfc..b3191968e53a 100644 --- a/net/hsr/hsr_device.c +++ b/net/hsr/hsr_device.c @@ -427,6 +427,9 @@ static void hsr_proxy_announce(struct timer_list *t) * of SAN nodes stored in ProxyNodeTable. */ interlink = hsr_port_get_hsr(hsr, HSR_PT_INTERLINK); + if (!interlink) + goto done; + list_for_each_entry_rcu(node, &hsr->proxy_node_db, mac_list) { if (hsr_addr_is_redbox(hsr, node->macaddress_A)) continue; @@ -441,6 +444,7 @@ static void hsr_proxy_announce(struct timer_list *t) mod_timer(&hsr->announce_proxy_timer, jiffies + interval); } +done: rcu_read_unlock(); }
In the function hsr_proxy_annouance() added in the previous commit 5f703ce5c981 ("net: hsr: Send supervisory frames to HSR network with ProxyNodeTable data"), the return value of the hsr_port_get_hsr() function is not checked to be a NULL pointer, which causes a NULL pointer dereference. To solve this, we need to add code to check whether the return value of hsr_port_get_hsr() is NULL. Reported-by: syzbot+02a42d9b1bd395cbcab4@syzkaller.appspotmail.com Fixes: 5f703ce5c981 ("net: hsr: Send supervisory frames to HSR network with ProxyNodeTable data") Signed-off-by: Jeongjun Park <aha310510@gmail.com> --- net/hsr/hsr_device.c | 4 ++++ 1 file changed, 4 insertions(+) --