Re: [PATCH 1/2] lockdown: Switch implementation to using bitmap

[sergeh@kernel.org]

On Fri, Mar 21, 2025 at 12:24:20PM +0200, Nikolay Borisov wrote:
> Tracking the lockdown at the depth granularity rather than at the
> individual is somewhat inflexible as it provides an "all or nothing"
> approach. Instead there are use cases where it  will be useful to be
> able to lockdown individual features - TDX for example wants to disable
> access to just /dev/mem.
> 
> To accommodate this use case switch the internal implementation to using
> a bitmap so that individual lockdown features can be turned on. At the
> same time retain the existing semantic where
> INTEGRITY_MAX/CONFIDENTIALITY_MAX are treated as wildcards meaning "lock
> everything below me".
> 
> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>

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

but one comment below

> ---
>  security/lockdown/lockdown.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
> index cf83afa1d879..5014d18c423f 100644
> --- a/security/lockdown/lockdown.c
> +++ b/security/lockdown/lockdown.c
> @@ -10,12 +10,13 @@
>   * 2 of the Licence, or (at your option) any later version.
>   */
>  
> +#include <linux/bitmap.h>
>  #include <linux/security.h>
>  #include <linux/export.h>
>  #include <linux/lsm_hooks.h>
>  #include <uapi/linux/lsm.h>
>  
> -static enum lockdown_reason kernel_locked_down;
> +static DECLARE_BITMAP(kernel_locked_down, LOCKDOWN_CONFIDENTIALITY_MAX);
>  
>  static const enum lockdown_reason lockdown_levels[] = {LOCKDOWN_NONE,
>  						 LOCKDOWN_INTEGRITY_MAX,
> @@ -26,10 +27,15 @@ static const enum lockdown_reason lockdown_levels[] = {LOCKDOWN_NONE,
>   */
>  static int lock_kernel_down(const char *where, enum lockdown_reason level)
>  {
> -	if (kernel_locked_down >= level)
> -		return -EPERM;
>  
> -	kernel_locked_down = level;
> +	if (level > LOCKDOWN_CONFIDENTIALITY_MAX)
> +		return -EINVAL;
> +
> +	if (level == LOCKDOWN_INTEGRITY_MAX || level == LOCKDOWN_CONFIDENTIALITY_MAX)
> +		bitmap_set(kernel_locked_down, 1, level);
> +	else
> +		bitmap_set(kernel_locked_down, level, 1);
> +
>  	pr_notice("Kernel is locked down from %s; see man kernel_lockdown.7\n",
>  		  where);
>  	return 0;
> @@ -62,13 +68,12 @@ static int lockdown_is_locked_down(enum lockdown_reason what)
>  		 "Invalid lockdown reason"))
>  		return -EPERM;
>  
> -	if (kernel_locked_down >= what) {
> +	if (test_bit(what, kernel_locked_down)) {
>  		if (lockdown_reasons[what])
>  			pr_notice_ratelimited("Lockdown: %s: %s is restricted; see man kernel_lockdown.7\n",
>  				  current->comm, lockdown_reasons[what]);
>  		return -EPERM;
>  	}
> -
>  	return 0;
>  }
>  
> @@ -105,7 +110,7 @@ static ssize_t lockdown_read(struct file *filp, char __user *buf, size_t count,

Context here is:

static ssize_t lockdown_read(struct file *filp, char __user *buf, size_t count,
                             loff_t *ppos)
{
        char temp[80] = "";
        int i, offset = 0;

        for (i = 0; i < ARRAY_SIZE(lockdown_levels); i++) {
                enum lockdown_reason level = lockdown_levels[i];

...

>  		if (lockdown_reasons[level]) {
>  			const char *label = lockdown_reasons[level];
>  
> -			if (kernel_locked_down == level)
> +			if (test_bit(level, kernel_locked_down))

Right now this is still just looping over the lockdown_levels, and so
it can't get longer than "none [integrity] [confidentiality]" which fits
easily into the 80 chars of temp.  But I'm worried that someone will
change this loop i a way that violates that.  Could you just switch
this to a snprintf that checks its result for < 0 and >= n , or some
other sanity check?

>  				offset += sprintf(temp+offset, "[%s] ", label);
>  			else
>  				offset += sprintf(temp+offset, "%s ", label);
> -- 
> 2.43.0
>