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

[Nikolay Borisov]

On 21.03.25 г. 22:34 ч., sergeh@xxxxxxxxxx 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@xxxxxxxx>
>
> Reviewed-by: Serge Hallyn <sergeh@xxxxxxxxxx>
>
> 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