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]

Message-ID: <20220329102347.iu6mlbv5c76ci3j7@wittgenstein>
Date:   Tue, 29 Mar 2022 12:23:47 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Fedor Pchelkin <aissur0002@...il.com>,
        Alexey Khoroshilov <khoroshilov@...ras.ru>,
        Eric Biggers <ebiggers@...nel.org>,
        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()

On Sun, Mar 27, 2022 at 03:21:18PM -0700, Linus Torvalds wrote:
> On Sun, Mar 27, 2022 at 2:53 PM <aissur0002@...il.com> wrote:
> >
> > I am sorry, that was my first attempt to contribute to the kernel and
> > I messed up a little bit with the patch tag: it is actually a single,
> > standalone patch and there has been nothing posted previously.
> 
> No problem, thanks for clarifying.
> 
> But the patch itself in that case is missing some detail, since it
> clearly doesn't apply to upstream.
> 
> Anyway:
> 
> > In few words, an error occurs while executing close_range() call with
> > CLOSE_RANGE_UNSHARE flag: in __close_range() the value of
> > max_unshare_fds (later used as max_fds in dup_fd() and copy_fd_bitmaps())
> > can be an arbitrary number.
> >
> >   if (max_fd >= last_fd(files_fdtable(cur_fds)))
> >     max_unshare_fds = fd;
> >
> > and not be a multiple of BITS_PER_LONG or BITS_PER_BYTE.
> 
> Very good, that's the piece I was missing. I only looked in fs/file.c,
> and missed how that max_unshare_fds gets passed from __close_range()
> into fork.c for unshare_fd() and then back to file.c and dup_fd(). And
> then affects sane_fdtable_size().
> 
> I _think_ it should be sufficient to just do something like
> 
>        max_fds = ALIGN(max_fds, BITS_PER_LONG)
> 
> in sane_fdtable_size(), but honestly, I haven't actually thought about
> it all that much. That's just a gut feel without really analyzing
> things - sane_fdtable_size() really should never return a value that
> isn't BITS_PER_LONG aligned.
> 
> And that whole close_range() is why I added Christian Brauner to the
> participant list, though, so let's see if Christian has any comments.
> 
> Christian?

(Sorry, I was heads-deep in some other fs work and went into airplaine
mode. I'm back.)

So I investigated a similar report a little while back and I spent quite
a lot of time trying to track this down but didn't find the cause.
If you'd call:

close_range(131, -1, CLOSE_RANGE_UNSHARE);

for an fdtable that is smaller than 131 then we'd call:

unshare_fd(..., 131)
\dup_fd(..., 131)
  \sane_fdtable_size(..., 131)

So sane_fdtable_size() would return 131 which is not aligned. This
couldn't happen before CLOSE_RANGE_UNSHARE afaict. I'll try to do a
repro with this with your suggested fix applied.

Christian