linux-kernel - Re: [PATCH] fuse: add a null-ptr check

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJnrk1b1zM=Zyn+LiV2bLbShQoCj4z5b++W2H4h7zR0QbTdZjg@mail.gmail.com>
Date: Mon, 2 Dec 2024 12:40:16 -0800
From: Joanne Koong <joannelkoong@...il.com>
To: Bernd Schubert <bernd.schubert@...tmail.fm>
Cc: Nihar Chaithanya <niharchaithanya@...il.com>, miklos@...redi.hu, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	skhan@...uxfoundation.org, 
	syzbot+87b8e6ed25dbc41759f7@...kaller.appspotmail.com
Subject: Re: [PATCH] fuse: add a null-ptr check

On Sat, Nov 30, 2024 at 12:22 AM Bernd Schubert
<bernd.schubert@...tmail.fm> wrote:
>
> On 11/30/24 07:51, Nihar Chaithanya wrote:

Hi Nihar and Bernd,

> > The bug KASAN: null-ptr-deref is triggered due to *val being
> > dereferenced when it is null in fuse_copy_do() when performing
> > memcpy().

It's not clear to me that syzbot's "null-ptr-deref" complaint is about
*val being dereferenced when val is NULL.

The stack trace [1] points to the 2nd memcpy in fuse_copy_do():

/* Do as much copy to/from userspace buffer as we can */
static int fuse_copy_do(struct fuse_copy_state *cs, void **val, unsigned *size)
{
        unsigned ncpy = min(*size, cs->len);
        if (val) {
                void *pgaddr = kmap_local_page(cs->pg);
                void *buf = pgaddr + cs->offset;

                if (cs->write)
                        memcpy(buf, *val, ncpy);
                else
                        memcpy(*val, buf, ncpy);

                kunmap_local(pgaddr);
                *val += ncpy;
        }
...
}

but AFAICT, if val is NULL then we never try to deref val since it's
guarded by the "if (val)" check.

It seems like syzbot is either complaining about buf being NULL / *val
being NULL and then trying to deference those inside the memcpy call,
or maybe it actually is (mistakenly) complaining about val being NULL.

It's not clear to me either how the "fuse: convert direct io to use
folios" patch (on the fuse tree, it's commit 3b97c36) [2] directly
causes this.

If I'm remembering correctly, it's possible to add debug printks to a
patch and syzbot will print out the debug messages as it triggers the
issue? It'd be interesting to see which request opcode triggers this,
and what exactly is being deref-ed here that is NULL. I need to look
at this more deeply but so far, nothing stands out as to what could be
the culprit.


Thanks,
Joanne

[1] https://lore.kernel.org/linux-fsdevel/67475f25.050a0220.253251.005b.GAE@google.com/
[2] https://lore.kernel.org/linux-fsdevel/20241024171809.3142801-13-joannelkoong@gmail.com/

> > Add a check in fuse_copy_one() to prevent this.
> >
> > Reported-by: syzbot+87b8e6ed25dbc41759f7@...kaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=87b8e6ed25dbc41759f7
> > Fixes: 3b97c3652d91 ("fuse: convert direct io to use folios")
> > Tested-by: syzbot+87b8e6ed25dbc41759f7@...kaller.appspotmail.com
> > Signed-off-by: Nihar Chaithanya <niharchaithanya@...il.com>
> > ---
> >  fs/fuse/dev.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > index 563a0bfa0e95..9c93759ac14b 100644
> > --- a/fs/fuse/dev.c
> > +++ b/fs/fuse/dev.c
> > @@ -1070,6 +1070,9 @@ static int fuse_copy_pages(struct fuse_copy_state *cs, unsigned nbytes,
> >  /* Copy a single argument in the request to/from userspace buffer */
> >  static int fuse_copy_one(struct fuse_copy_state *cs, void *val, unsigned size)
> >  {
> > +     if (!val)
> > +             return -EINVAL;
> > +
> >       while (size) {
> >               if (!cs->len) {
> >                       int err = fuse_copy_fill(cs);
>
> I'm going to read through Joannes patches in the evening. Without
> further explanation I find it unusual to have size, but no value.
>
>
> Thanks,
> Bernd