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:   Tue, 29 Mar 2022 23:08:55 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     Fedor Pchelkin <aissur0002@...il.com>,
        Alexey Khoroshilov <khoroshilov@...ras.ru>,
        Eric Biggers <ebiggers@...nel.org>,
        Christian Brauner <brauner@...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 Tue, Mar 29, 2022 at 10:21 PM Jason A. Donenfeld <Jason@...c4.com> wrote:
>
> Peppering some printks, it looks like in `max_fds = ALIGN(max_fds,
> BITS_PER_LONG);`, max_fds is sometimes 4294967295 before the call, and
> that ALIGN winds up bringing it to 0.

Gaah. I actually went back and forth on the location of the ALIGN().

And then ended up putting it where it is for just a "smallest patch"
reason, which was clearly not the right thing to do.

The easier and more obvious fix was to just make the ALIGN() be at the
final 'return' statement, but I ended up moving it up just because I
didn't like how complicated the expression looked.

That was obviously very very wrong of me.

So does it help if you just remove that

        max_fds = ALIGN(max_fds, BITS_PER_LONG);

and instead make the final return be

        return ALIGN(min(count, max_fds), BITS_PER_LONG);

instead?

And now I feel like I should as penance just do what I tried to get
Christian to do, which was to just integrate the whole "don't even
bother looking past that passed-in max_fds" in count_open_files().

The whole organization of that "calculate current highest fd, only to
then ignore it if we didn't want that many file descriptors" is just
historical baggage.

                 Linus