MIME-Version: 1.0
References: <20230116212105.1840362-1-mjguzik@gmail.com> <20230116212105.1840362-2-mjguzik@gmail.com>
 <CAHC9VhSKEyyd-s_j=1UbA0+vOK7ggyCp6e-FNSG7XVYvCxoLnA@mail.gmail.com>
 <CAGudoHF+bg0qiq+ByVpysa9t8J=zpF8=d1CqDVS5GmOGpVM9rQ@mail.gmail.com>
 <CAHC9VhTnpWKnKRu3wFTNfub_qdcDePdEXYZWOpvpqL0fcfS_Uw@mail.gmail.com>
 <CAGudoHEWQJKMS=pL9Ate4COshgQaC-fjQ2RN3LiYmdS=0MVruA@mail.gmail.com> <CAHC9VhSYg-BbJvNBZd3dayYCf8bzedASoidnX23_i4iK7P-WxQ@mail.gmail.com>
In-Reply-To: <CAHC9VhSYg-BbJvNBZd3dayYCf8bzedASoidnX23_i4iK7P-WxQ@mail.gmail.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 24 Jan 2023 09:14:17 -0800
Message-ID: <CAHk-=wiG5wdWrx2uXRK3-i31Zp416krnu_KjmBbS3BVkiAUXLQ@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] vfs: avoid duplicating creds in faccessat if possible
To:     Paul Moore <paul@paul-moore.com>
Cc:     Mateusz Guzik <mjguzik@gmail.com>, viro@zeniv.linux.org.uk,
        serge@hallyn.com, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jan 24, 2023 at 9:00 AM Paul Moore <paul@paul-moore.com> wrote:
>
> My main concern is the duplication between the cred check and the cred
> override functions leading to a bug at some unknown point in the
> future.

Yeah, it might be good to try to have some common logic for this,
although it's kind of messy.

The access_override_creds() logic is fairly different from the "do I
need to create new creds" decision, since instead of *testing* whether
the fs[ug]id and [ug]id matches, it just sets the fs[ug]id to the
expected values.

So that part of the test doesn't really exist.

And the same is true of the !SECURE_NO_SETUID_FIXUP logic case - the
current access() override doesn't _test_ those variables for equality,
it just sets them.

So Mateusz' patch doesn't really duplicate any actual logic, it just
has similarities in that it checks "would that new cred that
access_override_creds() would create be the same as the old one".

So sharing code is hard, because the code is fundamentally not the same.

The new access_need_override_creds() function is right next to the
pre-existing access_override_creds() one, so at least they are close
to each other. That may be the best that can be done.

Maybe some of the "is it the root uid" logic could be shared, though.
Both cases do have this part in common:

        if (!issecure(SECURE_NO_SETUID_FIXUP)) {
                /* Clear the capabilities if we switch to a non-root user */
                kuid_t root_uid = make_kuid(override_cred->user_ns, 0);
                if (!uid_eq(override_cred->uid, root_uid))

and that is arguably the nastiest part of it all.

I don't think it's all that likely to change in the future, though
(except for possible changes due to user_ns re-orgs, but then changing
both would be very natural).

               Linus