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

[Nikolay Borisov <nik.borisov@suse.com>]

On 21.03.25 г. 22:34 ч., sergeh@kernel.org wrote:
> 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?

How about the following:

diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
index 412184121279..47b47c4f7b98 100644
--- a/security/lockdown/lockdown.c
+++ b/security/lockdown/lockdown.c
@@ -114,9 +114,9 @@ static ssize_t lockdown_read(struct file *filp, char __user *buf, size_t count,
                         const char *label = lockdown_reasons[level];
  
                         if (test_bit(level, kernel_locked_down))
-                               offset += sprintf(temp+offset, "[%s] ", label);
+                               offset += snprintf(temp+offset, 80-offset, "[%s] ", label);
                         else
-                               offset += sprintf(temp+offset, "%s ", label);
+                               offset += snprintf(temp+offset, 80-offset, "%s ", label);
                 }
         }

It prevents buffer overflow but doesn't prevent buffer truncation.

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