From: Anmol Karn <anmol.karan123@gmail.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: ralf@linux-mips.org, davem@davemloft.net, kuba@kernel.org,
netdev@vger.kernel.org, syzkaller-bugs@googlegroups.com,
linux-kernel@vger.kernel.org,
syzbot+a1c743815982d9496393@syzkaller.appspotmail.com,
linux-hams@vger.kernel.org,
linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [Linux-kernel-mentees] [PATCH] net: rose: Fix Null pointer dereference in rose_send_frame()
Date: Thu, 15 Oct 2020 23:58:27 +0530 [thread overview]
Message-ID: <20201015182827.GA84192@Thinkpad> (raw)
In-Reply-To: <20201015155051.GB66528@kroah.com>
On Thu, Oct 15, 2020 at 05:50:51PM +0200, Greg KH wrote:
> 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@syzkaller.appspotmail.com
> > > > Link: https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3
> > > > Signed-off-by: Anmol Karn <anmol.karan123@gmail.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
Sure sir, understood.
Thanks,
Anmol
next prev parent reply other threads:[~2020-10-15 18:28 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-15 0:17 [Linux-kernel-mentees] [PATCH] net: rose: Fix Null pointer dereference in rose_send_frame() Anmol Karn
2020-10-15 5:12 ` Greg KH
2020-10-15 14:10 ` Anmol Karn
2020-10-15 15:50 ` Greg KH
2020-10-15 18:28 ` Anmol Karn [this message]
2020-10-30 10:54 ` Anmol Karn
2020-11-01 11:02 ` Greg KH
2020-11-01 15:48 ` Anmol Karn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201015182827.GA84192@Thinkpad \
--to=anmol.karan123@gmail.com \
--cc=davem@davemloft.net \
--cc=gregkh@linuxfoundation.org \
--cc=kuba@kernel.org \
--cc=linux-hams@vger.kernel.org \
--cc=linux-kernel-mentees@lists.linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=ralf@linux-mips.org \
--cc=syzbot+a1c743815982d9496393@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).