lkml.org 
[lkml]   [2020]   [May]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 01/11] exec: Reduce bprm->per_clear to a single bit
    Linus Torvalds <torvalds@linux-foundation.org> writes:

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

    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

    \
     
     \ /
      Last update: 2020-05-28 21:22    [W:4.751 / U:0.436 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site