linux-kernel - Re: [Linux-kernel-mentees] [PATCH] net: rose: Fix Null pointer dereference in rose_send

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: <20201015155051.GB66528@kroah.com>
Date:   Thu, 15 Oct 2020 17:50:51 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Anmol Karn <anmol.karan123@...il.com>
Cc:     ralf@...ux-mips.org, davem@...emloft.net, kuba@...nel.org,
        netdev@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        linux-kernel@...r.kernel.org,
        syzbot+a1c743815982d9496393@...kaller.appspotmail.com,
        linux-hams@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [Linux-kernel-mentees] [PATCH] net: rose: Fix Null pointer
 dereference in rose_send_frame()

On Thu, Oct 15, 2020 at 07:40:12PM +0530, Anmol Karn wrote:
> On Thu, Oct 15, 2020 at 07:12:25AM +0200, Greg KH wrote:
> > On Thu, Oct 15, 2020 at 05:47:12AM +0530, Anmol Karn wrote:
> > > In rose_send_frame(), when comparing two ax.25 addresses, it assigns rose_call to 
> > > either global ROSE callsign or default port, but when the former block triggers and 
> > > rose_call is assigned by (ax25_address *)neigh->dev->dev_addr, a NULL pointer is 
> > > dereferenced by 'neigh' when dereferencing 'dev'.
> > > 
> > > - net/rose/rose_link.c
> > > This bug seems to get triggered in this line:
> > > 
> > > rose_call = (ax25_address *)neigh->dev->dev_addr;
> > > 
> > > Prevent it by checking NULL condition for neigh->dev before comparing addressed for 
> > > rose_call initialization.
> > > 
> > > Reported-by: syzbot+a1c743815982d9496393@...kaller.appspotmail.com 
> > > Link: https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3 
> > > Signed-off-by: Anmol Karn <anmol.karan123@...il.com>
> > > ---
> > > I am bit sceptical about the error return code, please suggest if anything else is 
> > > appropriate in place of '-ENODEV'.
> > > 
> > >  net/rose/rose_link.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/net/rose/rose_link.c b/net/rose/rose_link.c
> > > index f6102e6f5161..92ea6a31d575 100644
> > > --- a/net/rose/rose_link.c
> > > +++ b/net/rose/rose_link.c
> > > @@ -97,6 +97,9 @@ static int rose_send_frame(struct sk_buff *skb, struct rose_neigh *neigh)
> > >  	ax25_address *rose_call;
> > >  	ax25_cb *ax25s;
> > >  
> > > +	if (!neigh->dev)
> > > +		return -ENODEV;
> > 
> > How can ->dev not be set at this point in time?  Shouldn't that be
> > fixed, because it could change right after you check this, right?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hello Sir,
> 
> Thanks for the review,
> After following the call trace i thought, if neigh->dev is NULL it should
> be checked, but I will figure out what is going on with the crash reproducer,
> and I think rose_loopback_timer() is the place where problem started. 
> 
> Also, I have created a diff for checking neigh->dev before assigning ROSE callsign
> , please give your suggestions on this.
> 
> 
> diff --git a/net/rose/rose_link.c b/net/rose/rose_link.c
> index f6102e6f5161..2ddd5e559442 100644
> --- a/net/rose/rose_link.c
> +++ b/net/rose/rose_link.c
> @@ -97,10 +97,14 @@ static int rose_send_frame(struct sk_buff *skb, struct rose_neigh *neigh)
>         ax25_address *rose_call;
>         ax25_cb *ax25s;
>  
> -       if (ax25cmp(&rose_callsign, &null_ax25_address) == 0)
> -               rose_call = (ax25_address *)neigh->dev->dev_addr;
> -       else
> -               rose_call = &rose_callsign;
> +       if (neigh->dev) {
> +               if (ax25cmp(&rose_callsign, &null_ax25_address) == 0)
> +                       rose_call = (ax25_address *)neigh->dev->dev_addr;
> +               else
> +                       rose_call = &rose_callsign;
> +       } else {
> +               return -ENODEV;
> +       }

The point I am trying to make is that if someone else is setting ->dev
to NULL in some other thread/context/whatever, while this is running,
checking for it like this will not work.

What is the lifetime rules of that pointer?  Who initializes it, and who
sets it to NULL.  Figure that out first please to determine how to check
for this properly.

thanks,

greg k-h