diff mbox

[RFC,V2,1/4] capabilities: use macros to make the logic easier to follow and verify

Message ID f676394915d5a3b3feb3ffc2f1587575bc13deaa.1494527628.git.rgb@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Richard Guy Briggs May 11, 2017, 8:42 p.m. UTC
This change is intended to be logic-neutral and simply make the logic easier to
read in natural language and verify without getting distracted by details.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 security/commoncap.c |   53 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 34 insertions(+), 19 deletions(-)

Comments

Serge Hallyn May 12, 2017, 5:35 a.m. UTC | #1
On Thu, May 11, 2017 at 04:42:40PM -0400, Richard Guy Briggs wrote:
> This change is intended to be logic-neutral and simply make the logic easier to
> read in natural language and verify without getting distracted by details.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  security/commoncap.c |   53 ++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 78b3783..9520f0a 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -497,6 +497,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  	int ret;
>  	kuid_t root_uid;
>  

The #defines make me uncomfortable, especially the lack of parens around
them.  The way they are used seems fine, but they seem like potential
future maintenance issues.  I definately appreciate the way you broke
the functionality down, though.  And I'm not sure I can improve on it.

> +#define SROOT !issecure(SECURE_NOROOT) /* root is special */

maybe

static inline bool root_privileged() { return !issecure(SECURE_NOROOT); }

> +#define RROOT uid_eq(new->uid, root_uid) /* real root */
> +#define EROOT uid_eq(new->euid, root_uid) /* effective root */
> +#define SETUIDROOT !RROOT && EROOT /* set uid root */

Yeah every time I start typing an alternative it doesn't look as good.

> +#define SUID !uid_eq(new->euid, old->uid) /* set uid */
> +#define SGID !gid_eq(new->egid, old->gid) /* set gid */
> +#define pPADD !cap_issubset(new->cap_permitted, old->cap_permitted) /* process permitted capabilities have been added */
> +#define pESET !cap_issubset(new->cap_effective, new->cap_ambient) /* process effective capabilities have been set */
> +#define pEALL cap_issubset(CAP_FULL_SET, new->cap_effective) /* process effective capabilities are full set */
> +#define pAADD !cap_issubset(new->cap_ambient, old->cap_ambient) /* process ambient capabilities have been added */
>  	if (WARN_ON(!cap_ambient_invariant_ok(old)))
>  		return -EPERM;
>  
> @@ -507,13 +517,13 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  
>  	root_uid = make_kuid(new->user_ns, 0);
>  
> -	if (!issecure(SECURE_NOROOT)) {
> +	if (SROOT) {
>  		/*
>  		 * If the legacy file capability is set, then don't set privs
>  		 * for a setuid root binary run by a non-root user.  Do set it
>  		 * for a root user just to cause least surprise to an admin.
>  		 */
> -		if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
> +		if (has_cap && SETUIDROOT) {
>  			warn_setuid_and_fcaps_mixed(bprm->filename);
>  			goto skip;
>  		}
> @@ -521,33 +531,32 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  		 * To support inheritance of root-permissions and suid-root
>  		 * executables under compatibility mode, we override the
>  		 * capability sets for the file.
> -		 *
> -		 * If only the real uid is 0, we do not set the effective bit.
>  		 */
> -		if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
> +		if (EROOT || RROOT) {
>  			/* pP' = (cap_bset & ~0) | (pI & ~0) */
>  			new->cap_permitted = cap_combine(old->cap_bset,
>  							 old->cap_inheritable);
>  		}
> -		if (uid_eq(new->euid, root_uid))
> +		/*
> +		 * If only the real uid is root, we do not set the effective bit.
> +		 */
> +		if (EROOT)
>  			effective = true;
>  	}
>  skip:
>  
>  	/* if we have fs caps, clear dangerous personality flags */
> -	if (!cap_issubset(new->cap_permitted, old->cap_permitted))
> +	if (pPADD)
>  		bprm->per_clear |= PER_CLEAR_ON_SETID;
>  
> +	is_setid = SUID || SGID;
>  
>  	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
>  	 * credentials unless they have the appropriate permit.
>  	 *
>  	 * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
>  	 */
> -	is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
> -
> -	if ((is_setid ||
> -	     !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
> +	if ((is_setid || pPADD) &&
>  	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
>  	     !ptracer_capable(current, new->user_ns))) {
>  		/* downgrade; they get no more than they had, and maybe less */
> @@ -599,14 +608,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  	 * Number 1 above might fail if you don't have a full bset, but I think
>  	 * that is interesting information to audit.
>  	 */
> -	if (!cap_issubset(new->cap_effective, new->cap_ambient)) {
> -		if (!cap_issubset(CAP_FULL_SET, new->cap_effective) ||
> -		    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
> -		    issecure(SECURE_NOROOT)) {
> -			ret = audit_log_bprm_fcaps(bprm, new, old);
> -			if (ret < 0)
> -				return ret;
> -		}
> +	if (pESET && (!pEALL || !EROOT || !RROOT || !SROOT) ) {

This might be better served by a separate helper 

	if (nonroot_raised_e(new, root_uid)) {
		ret = audit_log_bprm_fcaps(bprm, new, old);
		if (ret < 0)
			return ret;
	}

> +		ret = audit_log_bprm_fcaps(bprm, new, old);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
>  	new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
> @@ -615,6 +620,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  		return -EPERM;
>  
>  	return 0;
> +#undef SROOT
> +#undef RROOT
> +#undef EROOT
> +#undef SETUIDROOT
> +#undef SUID
> +#undef SGID
> +#undef pPADD
> +#undef pESET
> +#undef pEALL
> +#undef pAADD
>  }
>  
>  /**
> -- 
> 1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Guy Briggs May 12, 2017, 11:37 a.m. UTC | #2
On 2017-05-12 00:35, Serge E. Hallyn wrote:
> On Thu, May 11, 2017 at 04:42:40PM -0400, Richard Guy Briggs wrote:
> > This change is intended to be logic-neutral and simply make the logic easier to
> > read in natural language and verify without getting distracted by details.
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  security/commoncap.c |   53 ++++++++++++++++++++++++++++++++-----------------
> >  1 files changed, 34 insertions(+), 19 deletions(-)
> > 
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index 78b3783..9520f0a 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -497,6 +497,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
> >  	int ret;
> >  	kuid_t root_uid;
> >  
> 
> The #defines make me uncomfortable, especially the lack of parens around
> them.  The way they are used seems fine, but they seem like potential
> future maintenance issues.  I definately appreciate the way you broke
> the functionality down, though.  And I'm not sure I can improve on it.

I wrestled with #defines vs functions and the #defines won because they
were cleaner without parameters in parens to cloud the reading of the
logic.  I could go with local variables to assign these a value, but
they would be evaluated at the time of setting the local variable (which
would be fine for most of the variables involved) but also use local
stack space, though the compiler may be smart enough to optimize it
away.  This was also why I quickly undefined them as soon as
practicable.

> > +#define SROOT !issecure(SECURE_NOROOT) /* root is special */
> 
> maybe
> 
> static inline bool root_privileged() { return !issecure(SECURE_NOROOT); }

That is one of the few clean ones that doesn't need any other parameters
(such as "new" and "old" and the recently computed "root_uid").

> > +#define RROOT uid_eq(new->uid, root_uid) /* real root */
> > +#define EROOT uid_eq(new->euid, root_uid) /* effective root */
> > +#define SETUIDROOT !RROOT && EROOT /* set uid root */
> 
> Yeah every time I start typing an alternative it doesn't look as good.

Yup, this one was originally SUIDROOT which would have conflicted with SUID.

> > +#define SUID !uid_eq(new->euid, old->uid) /* set uid */
> > +#define SGID !gid_eq(new->egid, old->gid) /* set gid */
> > +#define pPADD !cap_issubset(new->cap_permitted, old->cap_permitted) /* process permitted capabilities have been added */
> > +#define pESET !cap_issubset(new->cap_effective, new->cap_ambient) /* process effective capabilities have been set */
> > +#define pEALL cap_issubset(CAP_FULL_SET, new->cap_effective) /* process effective capabilities are full set */
> > +#define pAADD !cap_issubset(new->cap_ambient, old->cap_ambient) /* process ambient capabilities have been added */
> >  	if (WARN_ON(!cap_ambient_invariant_ok(old)))
> >  		return -EPERM;
> >  
> > @@ -507,13 +517,13 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
> >  
> >  	root_uid = make_kuid(new->user_ns, 0);
> >  
> > -	if (!issecure(SECURE_NOROOT)) {
> > +	if (SROOT) {
> >  		/*
> >  		 * If the legacy file capability is set, then don't set privs
> >  		 * for a setuid root binary run by a non-root user.  Do set it
> >  		 * for a root user just to cause least surprise to an admin.
> >  		 */
> > -		if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
> > +		if (has_cap && SETUIDROOT) {
> >  			warn_setuid_and_fcaps_mixed(bprm->filename);
> >  			goto skip;
> >  		}
> > @@ -521,33 +531,32 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
> >  		 * To support inheritance of root-permissions and suid-root
> >  		 * executables under compatibility mode, we override the
> >  		 * capability sets for the file.
> > -		 *
> > -		 * If only the real uid is 0, we do not set the effective bit.
> >  		 */
> > -		if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
> > +		if (EROOT || RROOT) {
> >  			/* pP' = (cap_bset & ~0) | (pI & ~0) */
> >  			new->cap_permitted = cap_combine(old->cap_bset,
> >  							 old->cap_inheritable);
> >  		}
> > -		if (uid_eq(new->euid, root_uid))
> > +		/*
> > +		 * If only the real uid is root, we do not set the effective bit.
> > +		 */
> > +		if (EROOT)
> >  			effective = true;
> >  	}
> >  skip:
> >  
> >  	/* if we have fs caps, clear dangerous personality flags */
> > -	if (!cap_issubset(new->cap_permitted, old->cap_permitted))
> > +	if (pPADD)
> >  		bprm->per_clear |= PER_CLEAR_ON_SETID;
> >  
> > +	is_setid = SUID || SGID;
> >  
> >  	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
> >  	 * credentials unless they have the appropriate permit.
> >  	 *
> >  	 * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
> >  	 */
> > -	is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
> > -
> > -	if ((is_setid ||
> > -	     !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
> > +	if ((is_setid || pPADD) &&
> >  	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
> >  	     !ptracer_capable(current, new->user_ns))) {
> >  		/* downgrade; they get no more than they had, and maybe less */
> > @@ -599,14 +608,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
> >  	 * Number 1 above might fail if you don't have a full bset, but I think
> >  	 * that is interesting information to audit.
> >  	 */
> > -	if (!cap_issubset(new->cap_effective, new->cap_ambient)) {
> > -		if (!cap_issubset(CAP_FULL_SET, new->cap_effective) ||
> > -		    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
> > -		    issecure(SECURE_NOROOT)) {
> > -			ret = audit_log_bprm_fcaps(bprm, new, old);
> > -			if (ret < 0)
> > -				return ret;
> > -		}
> > +	if (pESET && (!pEALL || !EROOT || !RROOT || !SROOT) ) {
> 
> This might be better served by a separate helper 
> 
> 	if (nonroot_raised_e(new, root_uid)) {
> 		ret = audit_log_bprm_fcaps(bprm, new, old);
> 		if (ret < 0)
> 			return ret;
> 	}
> 
> > +		ret = audit_log_bprm_fcaps(bprm, new, old);
> > +		if (ret < 0)
> > +			return ret;
> >  	}
> >  
> >  	new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
> > @@ -615,6 +620,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
> >  		return -EPERM;
> >  
> >  	return 0;
> > +#undef SROOT
> > +#undef RROOT
> > +#undef EROOT
> > +#undef SETUIDROOT
> > +#undef SUID
> > +#undef SGID
> > +#undef pPADD
> > +#undef pESET
> > +#undef pEALL
> > +#undef pAADD
> >  }
> >  
> >  /**
> > -- 
> > 1.7.1

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Serge Hallyn May 12, 2017, 1:50 p.m. UTC | #3
Quoting Richard Guy Briggs (rgb@redhat.com):
> This change is intended to be logic-neutral and simply make the logic easier to
> read in natural language and verify without getting distracted by details.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  security/commoncap.c |   53 ++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 78b3783..9520f0a 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -497,6 +497,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  	int ret;
>  	kuid_t root_uid;
>  
> +#define SROOT !issecure(SECURE_NOROOT) /* root is special */
> +#define RROOT uid_eq(new->uid, root_uid) /* real root */
> +#define EROOT uid_eq(new->euid, root_uid) /* effective root */
> +#define SETUIDROOT !RROOT && EROOT /* set uid root */
> +#define SUID !uid_eq(new->euid, old->uid) /* set uid */
> +#define SGID !gid_eq(new->egid, old->gid) /* set gid */
> +#define pPADD !cap_issubset(new->cap_permitted, old->cap_permitted) /* process permitted capabilities have been added */
> +#define pESET !cap_issubset(new->cap_effective, new->cap_ambient) /* process effective capabilities have been set */
> +#define pEALL cap_issubset(CAP_FULL_SET, new->cap_effective) /* process effective capabilities are full set */
> +#define pAADD !cap_issubset(new->cap_ambient, old->cap_ambient) /* process ambient capabilities have been added */
>  	if (WARN_ON(!cap_ambient_invariant_ok(old)))
>  		return -EPERM;

Ok, I'm going to offer a few alternatives below.  Please feel free to
say you think yours is better, and I'll simply ack that.  I just feel
obliged to give it the old college try,

> @@ -507,13 +517,13 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  
>  	root_uid = make_kuid(new->user_ns, 0);
>  
> -	if (!issecure(SECURE_NOROOT)) {
> +	if (SROOT) {
>  		/*
>  		 * If the legacy file capability is set, then don't set privs
>  		 * for a setuid root binary run by a non-root user.  Do set it
>  		 * for a root user just to cause least surprise to an admin.
>  		 */
> -		if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
> +		if (has_cap && SETUIDROOT) {

Thinking about this some more last night, there are two things which make these
checks harder to read than they should be.  One is the fact that the brain can
have a hard time with negation.  The second is that we have long conditionals
in which the pieces are expressed using the mechanism we need to express what
we want, rather than what we are really asking.    So sometimes just coming up
with a different name for a function can help clarify its use, i.e.
uid_changed(from, to) instead of !uid_eq.

So the above I think would be much easier as

		if (has_cap && did_setuid_to(new, root_uid)) {

Needing to predefine root_uid is unfortunate.  Perhaps giving it a shorter name
like 'root' would help in some of these lines.  So then

		if (has_cap && did_setuid_to(new, root)) {

>  			warn_setuid_and_fcaps_mixed(bprm->filename);
>  			goto skip;
>  		}
> @@ -521,33 +531,32 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  		 * To support inheritance of root-permissions and suid-root
>  		 * executables under compatibility mode, we override the
>  		 * capability sets for the file.
> -		 *
> -		 * If only the real uid is 0, we do not set the effective bit.
>  		 */
> -		if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
> +		if (EROOT || RROOT) {

		if (same_real_uid(new, root) || same_eff_uid(new, root))

>  			/* pP' = (cap_bset & ~0) | (pI & ~0) */
>  			new->cap_permitted = cap_combine(old->cap_bset,
>  							 old->cap_inheritable);
>  		}
> -		if (uid_eq(new->euid, root_uid))
> +		/*
> +		 * If only the real uid is root, we do not set the effective bit.
> +		 */
> +		if (EROOT)
>  			effective = true;

		if (same_eff_uid(new, root))

>  	}
>  skip:
>  
>  	/* if we have fs caps, clear dangerous personality flags */
> -	if (!cap_issubset(new->cap_permitted, old->cap_permitted))
> +	if (pPADD)
>  		bprm->per_clear |= PER_CLEAR_ON_SETID;

		if (p_caps_grew(old, new))

Not quite sure whether p_caps_grew, e_caps_grew, a_caps_grew() are
nicer, or caps_grew(old, new, field).  i like the latter, but when
seeing the final patch maybe the former would be clearer.  But so maybe

		if (caps_grew(old, new, permitted))

>  
> +	is_setid = SUID || SGID;

	is_setid = eff_uid_changed(old, new) || eff_gid_changed(old, new)

uid_changed() is a trivial change, but i think is a lot more helpful while
reading this.

>  
>  	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
>  	 * credentials unless they have the appropriate permit.
>  	 *
>  	 * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
>  	 */
> -	is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
> -
> -	if ((is_setid ||
> -	     !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
> +	if ((is_setid || pPADD) &&

	if ((is_setid || p_caps_grew(old, new)) &&

>  	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
>  	     !ptracer_capable(current, new->user_ns))) {
>  		/* downgrade; they get no more than they had, and maybe less */
> @@ -599,14 +608,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  	 * Number 1 above might fail if you don't have a full bset, but I think
>  	 * that is interesting information to audit.
>  	 */
> -	if (!cap_issubset(new->cap_effective, new->cap_ambient)) {
> -		if (!cap_issubset(CAP_FULL_SET, new->cap_effective) ||
> -		    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
> -		    issecure(SECURE_NOROOT)) {
> -			ret = audit_log_bprm_fcaps(bprm, new, old);
> -			if (ret < 0)
> -				return ret;
> -		}
> +	if (pESET && (!pEALL || !EROOT || !RROOT || !SROOT) ) {

And this I still think this should be a separate function, though what
you have obviously helped you reason about its correctness.

> +		ret = audit_log_bprm_fcaps(bprm, new, old);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
>  	new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
> @@ -615,6 +620,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  		return -EPERM;
>  
>  	return 0;
> +#undef SROOT
> +#undef RROOT
> +#undef EROOT
> +#undef SETUIDROOT
> +#undef SUID
> +#undef SGID
> +#undef pPADD
> +#undef pESET
> +#undef pEALL
> +#undef pAADD

Lastly, the first !issecure(SECURE_NOROOT) block could stand to be a
separate function.

	handle_privileged_root(bprm, has_cap, &effective, root);
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/security/commoncap.c b/security/commoncap.c
index 78b3783..9520f0a 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -497,6 +497,16 @@  int cap_bprm_set_creds(struct linux_binprm *bprm)
 	int ret;
 	kuid_t root_uid;
 
+#define SROOT !issecure(SECURE_NOROOT) /* root is special */
+#define RROOT uid_eq(new->uid, root_uid) /* real root */
+#define EROOT uid_eq(new->euid, root_uid) /* effective root */
+#define SETUIDROOT !RROOT && EROOT /* set uid root */
+#define SUID !uid_eq(new->euid, old->uid) /* set uid */
+#define SGID !gid_eq(new->egid, old->gid) /* set gid */
+#define pPADD !cap_issubset(new->cap_permitted, old->cap_permitted) /* process permitted capabilities have been added */
+#define pESET !cap_issubset(new->cap_effective, new->cap_ambient) /* process effective capabilities have been set */
+#define pEALL cap_issubset(CAP_FULL_SET, new->cap_effective) /* process effective capabilities are full set */
+#define pAADD !cap_issubset(new->cap_ambient, old->cap_ambient) /* process ambient capabilities have been added */
 	if (WARN_ON(!cap_ambient_invariant_ok(old)))
 		return -EPERM;
 
@@ -507,13 +517,13 @@  int cap_bprm_set_creds(struct linux_binprm *bprm)
 
 	root_uid = make_kuid(new->user_ns, 0);
 
-	if (!issecure(SECURE_NOROOT)) {
+	if (SROOT) {
 		/*
 		 * If the legacy file capability is set, then don't set privs
 		 * for a setuid root binary run by a non-root user.  Do set it
 		 * for a root user just to cause least surprise to an admin.
 		 */
-		if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
+		if (has_cap && SETUIDROOT) {
 			warn_setuid_and_fcaps_mixed(bprm->filename);
 			goto skip;
 		}
@@ -521,33 +531,32 @@  int cap_bprm_set_creds(struct linux_binprm *bprm)
 		 * To support inheritance of root-permissions and suid-root
 		 * executables under compatibility mode, we override the
 		 * capability sets for the file.
-		 *
-		 * If only the real uid is 0, we do not set the effective bit.
 		 */
-		if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
+		if (EROOT || RROOT) {
 			/* pP' = (cap_bset & ~0) | (pI & ~0) */
 			new->cap_permitted = cap_combine(old->cap_bset,
 							 old->cap_inheritable);
 		}
-		if (uid_eq(new->euid, root_uid))
+		/*
+		 * If only the real uid is root, we do not set the effective bit.
+		 */
+		if (EROOT)
 			effective = true;
 	}
 skip:
 
 	/* if we have fs caps, clear dangerous personality flags */
-	if (!cap_issubset(new->cap_permitted, old->cap_permitted))
+	if (pPADD)
 		bprm->per_clear |= PER_CLEAR_ON_SETID;
 
+	is_setid = SUID || SGID;
 
 	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
 	 * credentials unless they have the appropriate permit.
 	 *
 	 * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
 	 */
-	is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
-
-	if ((is_setid ||
-	     !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
+	if ((is_setid || pPADD) &&
 	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
 	     !ptracer_capable(current, new->user_ns))) {
 		/* downgrade; they get no more than they had, and maybe less */
@@ -599,14 +608,10 @@  int cap_bprm_set_creds(struct linux_binprm *bprm)
 	 * Number 1 above might fail if you don't have a full bset, but I think
 	 * that is interesting information to audit.
 	 */
-	if (!cap_issubset(new->cap_effective, new->cap_ambient)) {
-		if (!cap_issubset(CAP_FULL_SET, new->cap_effective) ||
-		    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
-		    issecure(SECURE_NOROOT)) {
-			ret = audit_log_bprm_fcaps(bprm, new, old);
-			if (ret < 0)
-				return ret;
-		}
+	if (pESET && (!pEALL || !EROOT || !RROOT || !SROOT) ) {
+		ret = audit_log_bprm_fcaps(bprm, new, old);
+		if (ret < 0)
+			return ret;
 	}
 
 	new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
@@ -615,6 +620,16 @@  int cap_bprm_set_creds(struct linux_binprm *bprm)
 		return -EPERM;
 
 	return 0;
+#undef SROOT
+#undef RROOT
+#undef EROOT
+#undef SETUIDROOT
+#undef SUID
+#undef SGID
+#undef pPADD
+#undef pESET
+#undef pEALL
+#undef pAADD
 }
 
 /**