linux-kernel - Re: [PATCH] do_open(): Fix O_DIRECTORY

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: <CAHk-=wg2nJ3Z8x-nDGi9iCJvDCgbhpN+qnZt6V1JPnHqxX2fhQ@mail.gmail.com>
Date:   Tue, 21 Mar 2023 14:47:55 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Christian Brauner <brauner@...nel.org>
Cc:     Pedro Falcato <pedro.falcato@...il.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Aleksa Sarai <cyphar@...har.com>
Subject: Re: [PATCH] do_open(): Fix O_DIRECTORY | O_CREAT behavior

On Tue, Mar 21, 2023 at 1:16 PM Christian Brauner <brauner@...nel.org> wrote:
>
> But yes, that is a valid complaint so - without having tested - sm like:

I'd actually go a bit further, and really spell all the bits out explicitly.

I mean, I was *literally* involved in that original O_TMPFILE_MASK thing:

   https://lore.kernel.org/all/CA+55aFxA3qoM5wpMUya7gEA8SZyJep7kMBRjrPOsOm_OudD8aQ@mail.gmail.com/

with the whole O_DIRECOTY games to make O_TMPFILE safer, but despite
that I didn't remember this at all, and my suggested "maybe something
like this" patch was broken for the O_TMPFILE case.

So while we do have all this documented in our history (both git
commit logs and lore.kernel.org), I actually think it would be lovely
to just make build_open_flags() be very explicit about all the exact
O_xyz flags, and really write out the logic fully.

For example, even your clarified version that gets rid of the
"O_TMPFILE_MASK" thing still eends up doing

        if (flags & __O_TMPFILE) {
                if ((flags & O_TMPFILE) != O_TMPFILE)
                        return -EINVAL;

and so when you look at that code, you don't actually realize that
O_TMPFILE _cotnains_ that __O_TMPFILE bit, and what the above really
means is "also check O_DIRECTORY".

So considering how I couldn't remember this mess myself, despite
having been involved with it personally (a decade ago..), I really do
think that maybe this shoudl be open-coded with a comment, and the
above code should instead be

        if (flags & __O_TMPFILE) {
                if (!(flags & O_DIRECTORY))
                        return -EINVAL;

together with an explicit comment about how O_TMPFILE is the
*combination* of __O_TMPFILE and O_DIRECTORY, along with a short
explanation as to why.

Now, I agree that that test for O_DIRECTORY then _looks_ odd, but the
thing is, it then makes the reality of this all much more explicit.

In contrast, doing that

                if ((flags & O_TMPFILE) != O_TMPFILE)

may *look* more natural in that context, but if you actually start
thinking about it, that check makes no sense unless you then look up
what O_TMPFILE is, and the history behind it.

So I'd rather have code that looks a bit odd, but that explains itself
and is explicit about what it does, than code that _tries_ to look
natural but actually hides the reason for what it is doing.

And then next time somebody looks at that O_DIRECTORY | O_CREAT
combination, suddenly the __O_TMPFILE interaction is there, and very
explicit.

Hmm?

I don't feel *hugely* strongly about this, so in the end I'll bow to
your decision, but considering that my initial patch looked sane but
was buggy because I had forgotten about O_TMPFILE, I really think we
should make this more explicit at a source level..

               Linus

            Linus