linux-kernel - Re: [PATCH 00/26] cred: rework {override,revert}

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: <CAOQ4uxgYf2kEkYSz=AC++B6cb643Aq82En5QjwDwsSpPRf+A6w@mail.gmail.com>
Date: Mon, 25 Nov 2024 13:55:25 +0100
From: Amir Goldstein <amir73il@...il.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Christian Brauner <brauner@...nel.org>, Miklos Szeredi <miklos@...redi.hu>, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 00/26] cred: rework {override,revert}_creds()

On Sun, Nov 24, 2024 at 7:00 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Sun, 24 Nov 2024 at 05:44, Christian Brauner <brauner@...nel.org> wrote:
> >
> > This series does all that. Afaict, most callers can be directly
> > converted over and can avoid the extra reference count completely.
> >
> > Lightly tested.
>
> Thanks, this looks good to me. I only had two reactions:
>
>  (a) I was surprised that using get_new_cred() apparently "just worked".
>
> I was expecting us to have cases where the cred was marked 'const',
> because I had this memory of us actively marking things const to make
> sure people didn't play games with modifying the creds in-place (and
> then casting away the const just for ref updates).
>
> But apparently that's never the case for override_creds() users, so
> your patch actually ended up even simpler than I expected in that you
> didn't end up needing any new helper for just incrementing the
> refcount on a const cred.
>
>  (b) a (slight) reaction was to wish for a short "why" on the
> pointless reference bumps
>
> partly to show that it was thought about, but also partly to
> discourage people from doing it entirely mindlessly in other cases.
>
> I mean, sometimes the reference bumps were just obviously pointless
> because they ended up being right next to each other after being
> exposed, like the get/put pattern in access_override_creds().
>
> But in some other cases, like the aio_write case, I think it would
> have been good to just say
>
>  "The refcount is held by iocb->fsync.creds that cannot change over
> the operation"
>
> or similar. Or - very similarly - the binfmt_misc uses "file->f_cred",
> and again, file->f_cred is set at open time and never changed, so we
> can rely on it staying around for the file lifetime.
>
> I actually don't know if there were any exceptions to this (ie cases
> where the source of the override cred could actually go away from
> under us during the operation) where you didn't end up removing the
> refcount games as a result.

I was asking myself the same question.

I see that cachefiles_{begin,end}_secure() bump the refcount, but they
mostly follow a very similar pattern to the cases that do not bump the refcount,
so I wonder if you left this out because they were hidden in those
inline helpers
or because of the non-trivial case of  cachefiles_determine_cache_security()
which replaces the 'master' cache_creds?

Other that that, I stared at the creds code in nfsd_file_acquire_local() and
nfsd_setuser() more than I would like to admit, with lines like:

        /* discard any old override before preparing the new set */
        put_cred(revert_creds(get_cred(current_real_cred())));

And my only conclusion was this code is complicated enough,
so it'd better not use borrowed creds..

Thanks,
Amir.