linux-kernel - Re: [PATCH v2 2/2] vfs: avoid duplicating creds in faccessat if possible

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:   Wed, 25 Jan 2023 17:17:40 +0100
From:   Mateusz Guzik <mjguzik@...il.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        viro@...iv.linux.org.uk, serge@...lyn.com,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org
Subject: Re: [PATCH v2 2/2] vfs: avoid duplicating creds in faccessat if possible

On 1/25/23, Mateusz Guzik <mjguzik@...il.com> wrote:
> On 1/24/23, Paul Moore <paul@...l-moore.com> wrote:
>> Although I'm looking at this again and realized that only
>> do_faccessat() calls access_override_creds(), so why not just fold the
>> new access_need_override_creds() logic into access_override_creds()?
>> Just have one function that takes the flag value, and returns an
>> old_cred/NULL pointer (or pass old_cred to the function by reference
>> and return an error code); that should still provide the performance
>> win Mateusz is looking for while providing additional safety against
>> out-of-sync changes.  I would guess the code would be smaller too.
>>
>
> It is unclear from the description if you are arguing for moving the new
> func into access_override_creds almost as is just put prior to existing
> code *or* mixing checks with assignments.
>
> static bool *access_override_creds(struct cred **ptr)
>         [snip]
>         if (!uid_eq(cred->fsuid, cred->uid) ||
>             !gid_eq(cred->fsgid, cred->gid))
>                 return false;
>         /* remaining checks go here as well */
>         [snip]
>
>         override_cred = prepare_creds();
>         if (!override_cred) {
>                 *ptr = NULL;
>                 return true;
>         }
>
>         override_cred->fsuid = override_cred->uid;
>         override_cred->fsgid = override_cred->gid;
>         [snip]
>
> If this is what you had in mind, I note it retains all the duplication
> except in one func body which I'm confident does not buy anything,
> provided the warning comment is added.
>
> At the same time the downside is that it uglifies error handling at the
> callsite, so I would say a net loss.
>
> Alternatively, if you want to somehow keep tests aroung assignments the
> code gets super hairy.
>
> But maybe you wanted something else?
>
> As I noted in another email this already got more discussion than it
> warrants.
>
> Addition of the warning comment makes sense, but concerns after that
> don't sound legitimate to me.
>

So I posted v3 with the comment, you are CC'ed.

I'm not going to further argue about the patch. If you want to write
your own variant that's fine with me, feel free to take my bench results
and denote they come from a similar version.

There is other stuff I want to post and unslowed access() helps making
the case. The CONFIG_INIT_ON_ALLOC_DEFAULT_ON option is enabled on
Debian, Ubuntu and Arch and as a side effect it zeroes bufs allocated
every time a path lookup is performed. As these are 4096 bytes in size
it is not pretty whatsoever and I'm confident not worth the hardening
promised by mandatory zeroing. Got patches to add exemption support for
caches to sort it out without disabling the opt.

-- 
Mateusz Guzik <mjguzik gmail.com>