[01/11] exec: Reduce bprm->per_clear to a single bit
diff mbox series

Message ID 87eer4ysm5.fsf_-_@x220.int.ebiederm.org
State New
Headers show
Series
  • [01/11] exec: Reduce bprm->per_clear to a single bit
Related show

Commit Message

Eric W. Biederman May 28, 2020, 3:41 p.m. UTC
The bprm->per_clear field only takes the values 0 and
PER_CLEAR_ON_SETID.  Reduce the field to a signle bit to make it clear
that the only question is should the dangerous personality bits be
cleared or not.

Update the documentation of the security lsm hooks.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/exec.c                  | 7 ++++---
 include/linux/binfmts.h    | 4 +++-
 include/linux/lsm_hooks.h  | 4 ++++
 security/apparmor/domain.c | 2 +-
 security/commoncap.c       | 2 +-
 security/selinux/hooks.c   | 2 +-
 security/smack/smack_lsm.c | 2 +-
 7 files changed, 15 insertions(+), 8 deletions(-)

Comments

Linus Torvalds May 28, 2020, 7:04 p.m. UTC | #1
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
Eric W. Biederman May 28, 2020, 7:17 p.m. UTC | #2
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

Patch
diff mbox series

diff --git a/fs/exec.c b/fs/exec.c
index c3c879a55d65..51fab62b9fca 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1354,7 +1354,8 @@  int begin_new_exec(struct linux_binprm * bprm)
 	me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
 					PF_NOFREEZE | PF_NO_SETAFFINITY);
 	flush_thread();
-	me->personality &= ~bprm->per_clear;
+	if (bprm->per_clear)
+		me->personality &= ~PER_CLEAR_ON_SETID;
 
 	/*
 	 * We have to apply CLOEXEC before we change whether the process is
@@ -1628,12 +1629,12 @@  static void bprm_fill_uid(struct linux_binprm *bprm)
 		return;
 
 	if (mode & S_ISUID) {
-		bprm->per_clear |= PER_CLEAR_ON_SETID;
+		bprm->per_clear = 1;
 		bprm->cred->euid = uid;
 	}
 
 	if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
-		bprm->per_clear |= PER_CLEAR_ON_SETID;
+		bprm->per_clear = 1;
 		bprm->cred->egid = gid;
 	}
 }
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 7fc05929c967..e7959a6a895a 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -26,6 +26,9 @@  struct linux_binprm {
 	unsigned long p; /* current top of mem */
 	unsigned long argmin; /* rlimit marker for copy_strings() */
 	unsigned int
+		/* Should unsafe personality bits be cleared? */
+		per_clear:1,
+
 		/* Should an execfd be passed to userspace? */
 		have_execfd:1,
 
@@ -55,7 +58,6 @@  struct linux_binprm {
 	struct file * file;
 	struct cred *cred;	/* new credentials */
 	int unsafe;		/* how unsafe this exec is (mask of LSM_UNSAFE_*) */
-	unsigned int per_clear;	/* bits to clear in current->personality */
 	int argc, envc;
 	const char * filename;	/* Name of binary as seen by procps */
 	const char * interp;	/* Name of the binary really executed. Most
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index d618ecc4d660..0ca68ad53592 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -42,6 +42,8 @@ 
  *	(e.g. for transitions between security domains).
  *	The hook must set @bprm->secureexec to 1 if AT_SECURE should be set to
  *	request libc enable secure mode.
+ *	The hook must set @bprm->per_clear to 1 if the dangerous personality
+ *	bits must be cleared from current->personality.
  *	@bprm contains the linux_binprm structure.
  *	Return 0 if the hook is successful and permission is granted.
  * @bprm_repopulate_creds:
@@ -55,6 +57,8 @@ 
  *	transitions between security domains).
  *	The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be set to
  *	request libc enable secure mode.
+ *	The hook must set @bprm->per_clear to 1 if the dangerous personality
+ *	bits must be cleared from current->personality.
  *	@bprm contains the linux_binprm structure.
  *	Return 0 if the hook is successful and permission is granted.
  * @bprm_check_security:
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 0b870a647488..c6d00735a40a 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -962,7 +962,7 @@  int apparmor_bprm_creds_for_exec(struct linux_binprm *bprm)
 			aa_label_printk(new, GFP_KERNEL);
 			dbg_printk("\n");
 		}
-		bprm->per_clear |= PER_CLEAR_ON_SETID;
+		bprm->per_clear = 1;
 	}
 	aa_put_label(cred_label(bprm->cred));
 	/* transfer reference, released when cred is freed */
diff --git a/security/commoncap.c b/security/commoncap.c
index 77b04cb6feac..48b556046483 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -826,7 +826,7 @@  int cap_bprm_repopulate_creds(struct linux_binprm *bprm)
 
 	/* if we have fs caps, clear dangerous personality flags */
 	if (__cap_gained(permitted, new, old))
-		bprm->per_clear |= PER_CLEAR_ON_SETID;
+		bprm->per_clear = 1;
 
 	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
 	 * credentials unless they have the appropriate permit.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 718345dd76bb..6bea1b879fdb 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2385,7 +2385,7 @@  static int selinux_bprm_creds_for_exec(struct linux_binprm *bprm)
 		}
 
 		/* Clear any possibly unsafe personality bits on exec: */
-		bprm->per_clear |= PER_CLEAR_ON_SETID;
+		bprm->per_clear = 1;
 
 		/* Enable secure mode for SIDs transitions unless
 		   the noatsecure permission is granted between
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0ac8f4518d07..a0d2fad27b33 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -933,7 +933,7 @@  static int smack_bprm_creds_for_exec(struct linux_binprm *bprm)
 		return -EPERM;
 
 	bsp->smk_task = isp->smk_task;
-	bprm->per_clear |= PER_CLEAR_ON_SETID;
+	bprm->per_clear = 1;
 
 	/* Decide if this is a secure exec. */
 	if (bsp->smk_task != bsp->smk_forked)