Re: [PATCH 1/2] capabilities: Check PR_SET_SECUREBITS with ns_capable()

["Serge Hallyn (AMD)" <sergeh@kernel.org>]

On Tue, Nov 10, 2026 at 09:28:06PM +0100, Mickaël Salaün wrote:
> Per-process securebits were introduced with a direct cap_capable() check
> for CAP_SETPCAP.  The capable LSM hook and SELinux capability policy
> already existed, and SELinux forwarded task_prctl operations to
> commoncap.  The direct call therefore bypassed SELinux policy from the
> start.  It now also bypasses AppArmor, Landlock, and BPF LSM programs
> attached to the capable hook.
> 
> A task with CAP_SETPCAP in its effective set can consequently change
> privileged securebits despite an LSM denial.  This can alter how its
> capabilities evolve across UID and execution transitions without the
> expected LSM denial or audit record.
> 
> Use ns_capable(), as cap_prctl_drop() already does for PR_CAPBSET_DROP.
> Call it only for a no-op, which requires privilege for legacy reasons,
> or when a privileged bit changes.  Changes limited to
> SECURE_ALL_UNPRIVILEGED and their lock bits do not need the capability,
> so they must not invoke an LSM check or set PF_SUPERPRIV.  Successful
> privileged checks now set PF_SUPERPRIV, as other ns_capable() callers
> do.  Without other LSMs, the same requests remain allowed as before.
> 
> The check now reaches every capable hook before commoncap commits the
> new credentials.  A later task_prctl hook cannot provide equivalent
> mediation because commoncap runs first and commits the change itself.
> 
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Günther Noack <gnoack@google.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: John Johansen <john.johansen@canonical.com>
> Cc: KP Singh <kpsingh@kernel.org>
> Cc: Matt Bobrowski <matt@bobrowski.net>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Serge Hallyn <serge@hallyn.com>

The commit message is a bit wordy.  All it really needed to say was
that 1. s/cap_capable/ns_capable to query other lsms, and 2. move
the check to a concise helper.  I really do not relish looking through
a git log where every commit message re-litigates the full history of
any code that is touched or moved.

But it looks good, thanks.

Reviewed-by: Serge Hallyn (AMD) <sergeh@kernel.org>

> Cc: Stephen Smalley <stephen.smalley.work@gmail.com>
> Cc: stable@vger.kernel.org
> Reported-by: Ada Smith <adasmithgt@gmail.com>
> Fixes: 3898b1b4ebff ("capabilities: implement per-process securebits")
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
>  security/commoncap.c | 42 ++++++++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/security/commoncap.c b/security/commoncap.c
> index d47ab3022343..1947128396e2 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1283,6 +1283,23 @@ static int cap_prctl_drop(unsigned long cap)
>  	return commit_creds(new);
>  }
>  
> +/*
> + * Doing anything with securebits requires privilege (go read about the
> + * "sendmail capabilities bug"), except for unprivileged bits.  Indeed, the
> + * SECURE_ALL_UNPRIVILEGED bits are not restrictions enforced by the kernel but
> + * by user space on itself.  For legacy reasons, a non-change requires
> + * privilege too.  Only checking CAP_SETPCAP when it is needed keeps LSMs from
> + * seeing (and logging) a capability check for unprivileged changes.
> + */
> +static bool securebits_need_setpcap(unsigned long old, unsigned long new)
> +{
> +	const unsigned long unpriv_and_locks = SECURE_ALL_UNPRIVILEGED |
> +					       SECURE_ALL_UNPRIVILEGED << 1;
> +	const unsigned long changed = old ^ new;
> +

FWIW, I think the code was clearer when the !changed was separate with a
one line comment above it, than rolled into this return.

> +	return !changed || (changed & ~unpriv_and_locks);
> +}
> +
>  /**
>   * cap_task_prctl - Implement process control functions for this security module
>   * @option: The process control function requested
> @@ -1346,28 +1363,9 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
>  			/* cannot change a locked bit */
>  			return -EPERM;
>  
> -		/*
> -		 * Doing anything requires privilege (go read about the
> -		 * "sendmail capabilities bug"), except for unprivileged bits.
> -		 * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
> -		 * restrictions enforced by the kernel but by user space on
> -		 * itself.
> -		 */
> -		if (cap_capable(current_cred(), current_cred()->user_ns,
> -				CAP_SETPCAP, CAP_OPT_NONE) != 0) {
> -			const unsigned long unpriv_and_locks =
> -				SECURE_ALL_UNPRIVILEGED |
> -				SECURE_ALL_UNPRIVILEGED << 1;
> -			const unsigned long changed = old->securebits ^ arg2;
> -
> -			/* For legacy reason, denies non-change. */
> -			if (!changed)
> -				return -EPERM;
> -
> -			/* Denies privileged changes. */
> -			if (changed & ~unpriv_and_locks)
> -				return -EPERM;
> -		}
> +		if (securebits_need_setpcap(old->securebits, arg2) &&
> +		    !ns_capable(current_user_ns(), CAP_SETPCAP))
> +			return -EPERM;
>  
>  		new = prepare_creds();
>  		if (!new)
> -- 
> 2.56.0
>