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

[Paul Moore]

On Tue, Jan 24, 2023 at 12:14 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, Jan 24, 2023 at 9:00 AM Paul Moore <paul@xxxxxxxxxxxxxx> 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".

Perhaps I didn't do a very good job explaining my concern, or it got a
little twisted as the thread went on (likely due to my use of
"duplication"), but my concern wasn't so much that
access_override_creds() or the proposed access_need_override_creds()
duplicates code elsewhere, it was that the proposed
access_need_override_creds() function is a separate check from the
code which is actually responsible for doing the credential fixup for
AT_EACCESS.  I'm worried about a subtle change in one function not
being reflected in the other and causing an access control bug.

> 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.

Possibly, and the comment should help.

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.

> 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).

You're probably right.  As I said earlier, I'm just really sensitive
to code that is vulnerable to going out of sync like this and I try to
avoid it whenever possible.

-- 
paul-moore.com