Re: [PATCH net] net: hsr: prevent NULL pointer dereference in hsr_proxy_announce()

[Simon Horman]

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

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@xxxxxx/

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