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(+)

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();
 }
 
--

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

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!

+ 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 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>