Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <87h7wujhmz.fsf@x220.int.ebiederm.org> <87sgga6ze4.fsf@x220.int.ebiederm.org>
 <87v9l4zyla.fsf_-_@x220.int.ebiederm.org> <877dx822er.fsf_-_@x220.int.ebiederm.org>
 <87k10wysqz.fsf_-_@x220.int.ebiederm.org> <87eer4ysm5.fsf_-_@x220.int.ebiederm.org>
In-Reply-To: <87eer4ysm5.fsf_-_@x220.int.ebiederm.org>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 28 May 2020 12:04:12 -0700
Message-ID: <CAHk-=wgQAgKnEX3V_vep3Ah392tjiekDspnu+y6kkx2oFZBV=g@mail.gmail.com>
Subject: Re: [PATCH 01/11] exec: Reduce bprm->per_clear to a single bit
To:     "Eric W. Biederman" <ebiederm@xmission.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Oleg Nesterov <oleg@redhat.com>, Jann Horn <jannh@google.com>,
        Kees Cook <keescook@chromium.org>,
        Greg Ungerer <gerg@linux-m68k.org>,
        Rob Landley <rob@landley.net>,
        Bernd Edlinger <bernd.edlinger@hotmail.de>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Casey Schaufler <casey@schaufler-ca.com>,
        LSM List <linux-security-module@vger.kernel.org>,
        James Morris <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Andy Lutomirski <luto@amacapital.net>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, May 28, 2020 at 8:45 AM Eric W. Biederman <ebiederm@xmission.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?

           Linus