Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <00000000000069c40405be6bdad4@google.com> <CACT4Y+baP24jKmj-trhF8bG_d_zkz8jN7L1kYBnUR=EAY6hOaA@mail.gmail.com>
 <20210326091207.5si6knxs7tn6rmod@wittgenstein> <CACT4Y+atQdf_fe3BPFRGVCzT1Ba3V_XjAo6XsRciL8nwt4wasw@mail.gmail.com>
 <CAHrFyr7iUpMh4sicxrMWwaUHKteU=qHt-1O-3hojAAX3d5879Q@mail.gmail.com> <20210326135011.wscs4pxal7vvsmmw@wittgenstein>
In-Reply-To: <20210326135011.wscs4pxal7vvsmmw@wittgenstein>
From:   Dmitry Vyukov <dvyukov@google.com>
Date:   Fri, 26 Mar 2021 15:22:39 +0100
Message-ID: <CACT4Y+bHe2BG_DiVE9Bg3Vq60xqivpE=qRda+Ti20FJnWUqCDQ@mail.gmail.com>
Subject: Re: [syzbot] KASAN: null-ptr-deref Read in filp_close (2)
To:     Christian Brauner <christian.brauner@ubuntu.com>
Cc:     syzbot <syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
        Al Viro <viro@zeniv.linux.org.uk>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Mar 26, 2021 at 2:50 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Fri, Mar 26, 2021 at 10:34:28AM +0100, Christian Brauner wrote:
> > On Fri, Mar 26, 2021, 10:21 Dmitry Vyukov <dvyukov@google.com> wrote:
> >
> > > On Fri, Mar 26, 2021 at 10:12 AM Christian Brauner
> > > <christian.brauner@ubuntu.com> wrote:
> > > >
> > > > On Fri, Mar 26, 2021 at 09:02:08AM +0100, Dmitry Vyukov wrote:
> > > > > On Fri, Mar 26, 2021 at 8:55 AM syzbot
> > > > > <syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com> wrote:
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > syzbot found the following issue on:
> > > > > >
> > > > > > HEAD commit:    5ee96fa9 Merge tag 'irq-urgent-2021-03-21' of git://
> > > git.ke..
> > > > > > git tree:       upstream
> > > > > > console output:
> > > https://syzkaller.appspot.com/x/log.txt?x=17fb84bed00000
> > > > > > kernel config:
> > > https://syzkaller.appspot.com/x/.config?x=6abda3336c698a07
> > > > > > dashboard link:
> > > https://syzkaller.appspot.com/bug?extid=283ce5a46486d6acdbaf
> > > > > >
> > > > > > Unfortunately, I don't have any reproducer for this issue yet.
> > > > > >
> > > > > > IMPORTANT: if you fix the issue, please add the following tag to the
> > > commit:
> > > > > > Reported-by: syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com
> > > > >
> > > > > I was able to reproduce this with the following C program:
> > > > >
> > > https://gist.githubusercontent.com/dvyukov/00fb7aae489f22c60b4e64b45ef14d60/raw/cb368ca523d01986c2917f4414add0893b8f4243/gistfile1.txt
> > > > >
> > > > > +Christian
> > > > > The repro also contains close_range as the previous similar crash:
> > > > >
> > > https://syzkaller.appspot.com/bug?id=1bef50bdd9622a1969608d1090b2b4a588d0c6ac
> > > > > I don't know if it's related or not in this case, but looks suspicious.
> > > >
> > > > Hm, I fail to reproduce this with your repro. Do you need to have it run
> > > > for a long time?
> > > > One thing that strucky my eye is that binfmt_misc gets setup which made
> > > > me go huh and I see commit
> > > >
> > > > commit e7850f4d844e0acfac7e570af611d89deade3146
> > > > Author: Lior Ribak <liorribak@gmail.com>
> > > > Date:   Fri Mar 12 21:07:41 2021 -0800
> > > >
> > > >     binfmt_misc: fix possible deadlock in bm_register_write
> > > >
> > > > which uses filp_close() after having called open_exec() on the
> > > > interpreter which makes me wonder why this doesn't have to use fput()
> > > > like in all other codepaths for binfmnt_*.
> > > >
> > > > Can you revert this commit and see if you can reproduce this issue.
> > > > Maybe this is a complete red herring but worth a try.
> > >
> > >
> > > This program reproduces the crash for me almost immediately. Are you
> > > sure you used the right commit/config?
> > >
> >
> > I was trying to reproduce on v5.12-rc3 with all KASAN, KCSAN, KFENCE etc.
> > turned on.
> > I have an appointment I need to go to but will try to reproduce with commit
> > and config you provided when I get home.
> > I really hope it's not reproducible with v5.12-rc3 and only later commits
> > since that would allow easier bisection.
>
> Ok, I think I know what's going on. This fixes it for me. Can you test
> too, please? I tried the #syz test way but syzbot doesn't have the
> reproducer you gave me:


The crash does not happen with this patch.

Tested-by: Dmitry Vyukov <dvyukov@google.com>

Thanks for the quick fix!


> Thank you!
> Christian
>
> From eeb120d02f40b15a925f54ebcf2b4c747c741ad0 Mon Sep 17 00:00:00 2001
> From: Christian Brauner <christian.brauner@ubuntu.com>
> Date: Fri, 26 Mar 2021 13:33:03 +0100
> Subject: [PATCH] file: fix close_range() for unshare+cloexec
>
> syzbot reported a bug when putting the last reference to a tasks file
> descriptor table. Debugging this showed we didn't recalculate the
> current maximum fd number for CLOSE_RANGE_UNSHARE | CLOSE_RANGE_CLOEXEC
> after we unshared the file descriptors table. So max_fd could exceed the
> current fdtable maximum causing us to set excessive bits. As a concrete
> example, let's say the user requested everything from fd 4 to ~0UL to be
> closed and their current fdtable size is 256 with their highest open fd
> being 4.  With CLOSE_RANGE_UNSHARE the caller will end up with a new
> fdtable which has room for 64 file descriptors since that is the lowest
> fdtable size we accept. But now max_fd will still point to 255 and needs
> to be adjusted. Fix this by retrieving the correct maximum fd value in
> __range_cloexec().
>
> Reported-by: syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  fs/file.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/file.c b/fs/file.c
> index f3a4bac2cbe9..5ef62377d924 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -632,6 +632,7 @@ EXPORT_SYMBOL(close_fd); /* for ksys_close() */
>  static inline void __range_cloexec(struct files_struct *cur_fds,
>                                    unsigned int fd, unsigned int max_fd)
>  {
> +       unsigned int cur_max;
>         struct fdtable *fdt;
>
>         if (fd > max_fd)
> @@ -639,7 +640,12 @@ static inline void __range_cloexec(struct files_struct *cur_fds,
>
>         spin_lock(&cur_fds->file_lock);
>         fdt = files_fdtable(cur_fds);
> -       bitmap_set(fdt->close_on_exec, fd, max_fd - fd + 1);
> +       /* make very sure we're using the correct maximum value */
> +       cur_max = fdt->max_fds;
> +       cur_max--;
> +       cur_max = min(max_fd, cur_max);
> +       if (fd <= cur_max)
> +               bitmap_set(fdt->close_on_exec, fd, cur_max - fd + 1);
>         spin_unlock(&cur_fds->file_lock);
>  }
>
> --
> 2.27.0
>