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]

Message-ID: <CAGudoHG-42ziSNT0g8asRj8iGzx-Gn=ETZuXkswER3Daov37=A@mail.gmail.com>
Date:   Wed, 25 Jan 2023 16:00:19 +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/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.

-- 
Mateusz Guzik <mjguzik gmail.com>