linux-kernel - Re: [PATCH 4/4] file: Fix file descriptor leak in copy_fd

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]

Date:   Sat, 26 Mar 2022 15:15:19 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Alexey Khoroshilov <khoroshilov@...ras.ru>,
        Eric Biggers <ebiggers@...nel.org>,
        Christian Brauner <brauner@...nel.org>
Cc:     Fedor Pchelkin <aissur0002@...il.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] file: Fix file descriptor leak in copy_fd_bitmaps()

Sorry, quoting everything below to bring in Eric Biggers because he
touched that particular code last.

And Christian Brauner, because he worked on all teh bitmap code with
the whole close_range thing.

I think this is all ok because the number of files aren't just
byte-aligned, they are long-aligned:

         * We make sure that nr remains a multiple of BITS_PER_LONG - otherwise
         * bitmaps handling below becomes unpleasant, to put it mildly...

but maybe I'm missing something.

The fact that there's a

     Found by Syzkaller (https://github.com/google/syzkaller).

thing in that suggested commit message makes me think there _is_
something I'm missing.

Certainly NR_OPEN_DEFAULT, sane_fdtable_size() and max_fds should
always be a multiple of BITS_PER_LONG.

So I don't _think_ there is any bug here, although it might be good to

 (a) document that "we explicitly do things in BITS_PER_LONG chunks"
even more in places

 (b) have people double-check my thinking because clearly that
syzcaller thing implies I'm full of crap

Eric, Christian?

Can somebody point to the actual syzkaller report?

                Linus

On Sat, Mar 26, 2022 at 7:17 AM Alexey Khoroshilov
<khoroshilov@...ras.ru> wrote:
>
> Looks like bfp has a set of macro suitable for such cases:
>
> #define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK)
> #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
> #define BITS_ROUNDUP_BYTES(bits) \
>         (BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
>
> May be it makes sense to move them to a generic header and to use here?
>
> --
> Alexey Khoroshilov
>
>
> On 26.03.2022 14:40, Fedor Pchelkin wrote:
> > If count argument in copy_fd_bitmaps() is not a multiple of
> > BITS_PER_BYTE, then one byte is lost and is not used in further
> > manipulations with cpy value in memcpy() and memset()
> > causing a leak. The leak was introduced with close_range() call
> > using CLOSE_RANGE_UNSHARE flag.
> >
> > The patch suggests implementing an indicator (named add_byte)
> > of count being multiple of BITS_PER_BYTE and adding it to the
> > cpy value.
> >
> > Found by Syzkaller (https://github.com/google/syzkaller).
> >
> > Signed-off-by: Fedor Pchelkin <aissur0002@...il.com>
> > Signed-off-by: Alexey Khoroshilov <khoroshilov@...ras.ru>
> > ---
> >  fs/file.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/fs/file.c b/fs/file.c
> > index 3ef1479df203..3c64a6423604 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -56,10 +56,8 @@ static void copy_fd_bitmaps(struct fdtable *nfdt, struct fdtable *ofdt,
> >  {
> >       unsigned int cpy, set;
> >       unsigned int add_byte = 0;
> > -
> >       if (count % BITS_PER_BYTE != 0)
> >               add_byte = 1;
> > -
> >       cpy = count / BITS_PER_BYTE + add_byte;
> >       set = (nfdt->max_fds - count) / BITS_PER_BYTE;
> >       memcpy(nfdt->open_fds, ofdt->open_fds, cpy);
> >
>