linux-kernel - Re: [PATCH 01/11] exec: Reduce bprm->per

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: <87pnanvphc.fsf@x220.int.ebiederm.org>
Date:   Thu, 28 May 2020 14:17:19 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Oleg Nesterov <oleg@...hat.com>, Jann Horn <jannh@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Greg Ungerer <gerg@...ux-m68k.org>,
        Rob Landley <rob@...dley.net>,
        Bernd Edlinger <bernd.edlinger@...mail.de>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Casey Schaufler <casey@...aufler-ca.com>,
        LSM List <linux-security-module@...r.kernel.org>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Andy Lutomirski <luto@...capital.net>
Subject: Re: [PATCH 01/11] exec: Reduce bprm->per_clear to a single bit

Linus Torvalds <torvalds@...ux-foundation.org> writes:

> On Thu, May 28, 2020 at 8:45 AM Eric W. Biederman <ebiederm@...ssion.com> wrote:
>>
>> -       me->personality &= ~bprm->per_clear;
>> +       if (bprm->per_clear)
>> +               me->personality &= ~PER_CLEAR_ON_SETID;\
>
> My only problem with this patch is that I find that 'per_clear' thing
> to be a horrid horrid name,
>
> Obviously the name didn't change, but the use *did* change, and as
> such the name got worse. It used do do things like
>
>                bprm->per_clear |= PER_CLEAR_ON_SETID;
>
> and now it does
>
>                bprm->per_clear = 1;
>
> and honestly, there's a lot more semantic context in the old code that
> is now missing entirely. At least you used to be able to grep for
> PER_CLEAR_ON_SETID and it would make you go "Ahh.."
>
> Put another way, I can kind of see what a line like
>
>                bprm->per_clear |= PER_CLEAR_ON_SETID;
>
> does, simply because now it kind of hints at what is up.
>
> But what the heck does
>
>                bprm->per_clear = 1;
>
> mean? Nothing. You have to really know the code. "per_clear" makes no
> sense, and now it's a short line that doesn't need to be that short.
>
> I think "bprm->clear_personality_bits" would maybe describe what the
> _effect_ of that field is. It doesn't explain _why_, but it at least
> explains "what" much better than "per_clear", which just makes me go
> "per what?".
>
> Alternatively, "bprm->creds_changed" would describe what the bit
> conceptually is about, and code like
>
>           if (bprm->creds_changed)
>                   me->personality &= ~PER_CLEAR_ON_SETID;\
>
> looks sensible to me and kind of matches the comment about the
> PER_CLEAR_ON_SETID bits are.
>
> So I think that using a bitfield is fine, but I'd really like it to be
> named something much better.
>
> Plus changing the name means that you can't have any code that now
> mistakenly uses the new semantics but expects the old bitmask.
> Generally when something changes semantics that radically, you want to
> make sure the type changes sufficiently that any out-of-tree patch
> that hasn't been merged yet will get a clear warning or error if
> people don't realize.
>
> Please?

Yes.  That will make a very nice change to the patch.

I think I will go with bprm->clear_unsafe_personality_bits or
something to that effect. 

I would really love to have a bit that means creds_changes or
privilegeds_elevated.  But right now we have 2 of two fields that mean
essentially that (per_clear and secureexec) and they don't agree on when
they get set.

I will make them agree as much as possible, and this patchset is a first
step in that direction but until we can actually make them agree, I want
to keep them both grounded in what they do.  That way it is possible to
have a reasonable discussion on when they should be set.

Eric