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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wi5ZxjGBKsseL2eNHpDVDF=W_EDZcXVfmJ2Dk2Vh7o+nQ@mail.gmail.com>
Date: Sun, 24 Nov 2024 10:00:24 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Christian Brauner <brauner@...nel.org>
Cc: Amir Goldstein <amir73il@...il.com>, 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, 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. You did have a couple of cases where you
actually explained why the bump wasn't necessary, but there were a
couple where I would have wished for that "the reference count is held
by X, which is stable over the whole sequence" kind of notes.

But not a big deal. Even in this form, I think this is a clear and
good improvement.

Thanks,
                    Linus