[RFC PATCH 0/3] LSM syscall tweaks

[RFC PATCH 0/3] LSM syscall tweaks

[Paul Moore]

Three rather small LSM patches to address some minor issues found during
the review of the latest LSM syscall patchset that now lives in the
lsm/next-queue tree.

I'm marking these as RFC patches as they have yet to be properly tested,
but I'm building a kernel now to do that and I'll report back when testing
has completed.  In the meantime, reviews and ACKs are appreciated.

-- 
paul-moore.com

[RFC PATCH 1/3] lsm: cleanup the size counters in security_getselfattr()

[Paul Moore]

Zero out all of the size counters in the -E2BIG case (buffer too
small) to help make the current code a bit more robust in the face of
future code changes.

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 security/security.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/security.c b/security/security.c
index 988483fcf153..9c63acded4ee 100644
--- a/security/security.c
+++ b/security/security.c
@@ -3951,8 +3951,9 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
 			continue;
 		}
 		if (rc == -E2BIG) {
-			toobig = true;
+			rc = 0;
 			left = 0;
+			toobig = true;
 		} else if (rc < 0)
 			return rc;
 		else
-- 
2.42.0

Re: [RFC PATCH 1/3] lsm: cleanup the size counters in security_getselfattr()

[Casey Schaufler]

On 10/24/2023 2:35 PM, Paul Moore wrote:
> Zero out all of the size counters in the -E2BIG case (buffer too
> small) to help make the current code a bit more robust in the face of
> future code changes.

I don't see how this change would have the described effect.
What it looks like it would do is change the return from -E2BIG
to 0, which would not have the desired result.

>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  security/security.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/security/security.c b/security/security.c
> index 988483fcf153..9c63acded4ee 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -3951,8 +3951,9 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
>  			continue;
>  		}
>  		if (rc == -E2BIG) {
> -			toobig = true;
> +			rc = 0;
>  			left = 0;
> +			toobig = true;
>  		} else if (rc < 0)
>  			return rc;
>  		else

Re: [RFC PATCH 1/3] lsm: cleanup the size counters in security_getselfattr()

[Paul Moore]

On Tue, Oct 24, 2023 at 6:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 10/24/2023 2:35 PM, Paul Moore wrote:
> > Zero out all of the size counters in the -E2BIG case (buffer too
> > small) to help make the current code a bit more robust in the face of
> > future code changes.
>
> I don't see how this change would have the described effect.
> What it looks like it would do is change the return from -E2BIG
> to 0, which would not have the desired result.

When @toobig is true, which it will be when one of the individual LSMs
return -E2BIG, the return value of security_getselfattr() is fixed to
-E2BIG (check the if-statements at the end of the function).  Setting
@rc to zero as in this patch simply preserves some sanity in the
@count variable as we are no longer subtracting the E2BIG errno from
the @count value.  Granted, in the @toobig case, @count doesn't do
anything meaningful, but I believe this does harden the code against
future changes.

Look at the discussion between Mickaël and I in the v15 04/11 patch
for more background.

https://lore.kernel.org/linux-security-module/20230912205658.3432-5-casey@schaufler-ca.com

> > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > ---
> >  security/security.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/security.c b/security/security.c
> > index 988483fcf153..9c63acded4ee 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -3951,8 +3951,9 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
> >                       continue;
> >               }
> >               if (rc == -E2BIG) {
> > -                     toobig = true;
> > +                     rc = 0;
> >                       left = 0;
> > +                     toobig = true;
> >               } else if (rc < 0)
> >                       return rc;
> >               else

-- 
paul-moore.com

Re: [RFC PATCH 1/3] lsm: cleanup the size counters in security_getselfattr()

[Casey Schaufler]

On 10/24/2023 6:43 PM, Paul Moore wrote:
> On Tue, Oct 24, 2023 at 6:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 10/24/2023 2:35 PM, Paul Moore wrote:
>>> Zero out all of the size counters in the -E2BIG case (buffer too
>>> small) to help make the current code a bit more robust in the face of
>>> future code changes.
>> I don't see how this change would have the described effect.
>> What it looks like it would do is change the return from -E2BIG
>> to 0, which would not have the desired result.
> When @toobig is true, which it will be when one of the individual LSMs
> return -E2BIG, the return value of security_getselfattr() is fixed to
> -E2BIG (check the if-statements at the end of the function).  Setting
> @rc to zero as in this patch simply preserves some sanity in the
> @count variable as we are no longer subtracting the E2BIG errno from
> the @count value.  Granted, in the @toobig case, @count doesn't do
> anything meaningful, but I believe this does harden the code against
> future changes.
>
> Look at the discussion between Mickaël and I in the v15 04/11 patch
> for more background.
>
> https://lore.kernel.org/linux-security-module/20230912205658.3432-5-casey@schaufler-ca.com

OK. My bad for not looking beyond the patch.

Acked-by: Casey Schaufler <casey@schaufler-ca.com>

>
>>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>>> ---
>>>  security/security.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/security/security.c b/security/security.c
>>> index 988483fcf153..9c63acded4ee 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -3951,8 +3951,9 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
>>>                       continue;
>>>               }
>>>               if (rc == -E2BIG) {
>>> -                     toobig = true;
>>> +                     rc = 0;
>>>                       left = 0;
>>> +                     toobig = true;
>>>               } else if (rc < 0)
>>>                       return rc;
>>>               else

Re: [RFC PATCH 1/3] lsm: cleanup the size counters in security_getselfattr()

[Paul Moore]

On Wed, Oct 25, 2023 at 11:19 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 10/24/2023 6:43 PM, Paul Moore wrote:
> > On Tue, Oct 24, 2023 at 6:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 10/24/2023 2:35 PM, Paul Moore wrote:
> >>> Zero out all of the size counters in the -E2BIG case (buffer too
> >>> small) to help make the current code a bit more robust in the face of
> >>> future code changes.
> >> I don't see how this change would have the described effect.
> >> What it looks like it would do is change the return from -E2BIG
> >> to 0, which would not have the desired result.
> > When @toobig is true, which it will be when one of the individual LSMs
> > return -E2BIG, the return value of security_getselfattr() is fixed to
> > -E2BIG (check the if-statements at the end of the function).  Setting
> > @rc to zero as in this patch simply preserves some sanity in the
> > @count variable as we are no longer subtracting the E2BIG errno from
> > the @count value.  Granted, in the @toobig case, @count doesn't do
> > anything meaningful, but I believe this does harden the code against
> > future changes.
> >
> > Look at the discussion between Mickaël and I in the v15 04/11 patch
> > for more background.
> >
> > https://lore.kernel.org/linux-security-module/20230912205658.3432-5-casey@schaufler-ca.com
>
> OK. My bad for not looking beyond the patch.

No problem, we all get caught out from time to time, thanks for the review/ACK.

-- 
paul-moore.com

Re: [RFC PATCH 1/3] lsm: cleanup the size counters in security_getselfattr()

[Mickaël Salaün]

On Tue, Oct 24, 2023 at 05:35:27PM -0400, Paul Moore wrote:
> Zero out all of the size counters in the -E2BIG case (buffer too
> small) to help make the current code a bit more robust in the face of
> future code changes.
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>

Reviewed-by: Mickaël Salaün <mic@digikod.net>

> ---
>  security/security.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/security/security.c b/security/security.c
> index 988483fcf153..9c63acded4ee 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -3951,8 +3951,9 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
>  			continue;
>  		}
>  		if (rc == -E2BIG) {
> -			toobig = true;
> +			rc = 0;
>  			left = 0;
> +			toobig = true;
>  		} else if (rc < 0)
>  			return rc;
>  		else
> -- 
> 2.42.0
> 

[RFC PATCH 2/3] lsm: correct error codes in security_getselfattr()

[Paul Moore]

We should return -EINVAL if the user specifies LSM_FLAG_SINGLE without
supplying a valid lsm_ctx struct buffer.

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 security/security.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/security.c b/security/security.c
index 9c63acded4ee..67ded406a5ea 100644
--- a/security/security.c
+++ b/security/security.c
@@ -3923,9 +3923,9 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
 		/*
 		 * Only flag supported is LSM_FLAG_SINGLE
 		 */
-		if (flags != LSM_FLAG_SINGLE)
+		if (flags != LSM_FLAG_SINGLE || !uctx)
 			return -EINVAL;
-		if (uctx && copy_from_user(&lctx, uctx, sizeof(lctx)))
+		if (copy_from_user(&lctx, uctx, sizeof(lctx)))
 			return -EFAULT;
 		/*
 		 * If the LSM ID isn't specified it is an error.
-- 
2.42.0

Re: [RFC PATCH 2/3] lsm: correct error codes in security_getselfattr()

[Casey Schaufler]

On 10/24/2023 2:35 PM, Paul Moore wrote:
> We should return -EINVAL if the user specifies LSM_FLAG_SINGLE without
> supplying a valid lsm_ctx struct buffer.
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>

Acked-by: Casey Schaufler <casey@schaufler-ca.com>

> ---
>  security/security.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 9c63acded4ee..67ded406a5ea 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -3923,9 +3923,9 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
>  		/*
>  		 * Only flag supported is LSM_FLAG_SINGLE
>  		 */
> -		if (flags != LSM_FLAG_SINGLE)
> +		if (flags != LSM_FLAG_SINGLE || !uctx)
>  			return -EINVAL;
> -		if (uctx && copy_from_user(&lctx, uctx, sizeof(lctx)))
> +		if (copy_from_user(&lctx, uctx, sizeof(lctx)))
>  			return -EFAULT;
>  		/*
>  		 * If the LSM ID isn't specified it is an error.

Re: [RFC PATCH 2/3] lsm: correct error codes in security_getselfattr()

[Mickaël Salaün]

On Tue, Oct 24, 2023 at 05:35:28PM -0400, Paul Moore wrote:
> We should return -EINVAL if the user specifies LSM_FLAG_SINGLE without
> supplying a valid lsm_ctx struct buffer.
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>

Reviewed-by: Mickaël Salaün <mic@digikod.net>

> ---
>  security/security.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/security/security.c b/security/security.c
> index 9c63acded4ee..67ded406a5ea 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -3923,9 +3923,9 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
>  		/*
>  		 * Only flag supported is LSM_FLAG_SINGLE
>  		 */
> -		if (flags != LSM_FLAG_SINGLE)
> +		if (flags != LSM_FLAG_SINGLE || !uctx)
>  			return -EINVAL;
> -		if (uctx && copy_from_user(&lctx, uctx, sizeof(lctx)))
> +		if (copy_from_user(&lctx, uctx, sizeof(lctx)))
>  			return -EFAULT;
>  		/*
>  		 * If the LSM ID isn't specified it is an error.
> -- 
> 2.42.0
> 

[RFC PATCH 3/3] lsm: consolidate buffer size handling into lsm_fill_user_ctx()

[Paul Moore]

While we have a lsm_fill_user_ctx() helper function designed to make
life easier for LSMs which return lsm_ctx structs to userspace, we
didn't include all of the buffer length safety checks and buffer
padding adjustments in the helper.  This led to code duplication
across the different LSMs and the possibility for mistakes across the
different LSM subsystems.  In order to reduce code duplication and
decrease the chances of silly mistakes, we're consolidating all of
this code into the lsm_fill_user_ctx() helper.

The buffer padding is also modified from a fixed 8-byte alignment to
an alignment that matches the word length of the machine
(BITS_PER_LONG / 8).

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 include/linux/security.h   |  9 ++++---
 security/apparmor/lsm.c    | 15 +++--------
 security/security.c        | 55 +++++++++++++++++++++-----------------
 security/selinux/hooks.c   | 42 +++++++++++++++--------------
 security/smack/smack_lsm.c | 23 +++++-----------
 5 files changed, 67 insertions(+), 77 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 334f75aa7289..750130a7b9dd 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -492,8 +492,8 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
 int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
 int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
 int security_locked_down(enum lockdown_reason what);
-int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
-		      size_t context_size, u64 id, u64 flags);
+int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
+		      void *val, size_t val_len, u64 id, u64 flags);
 #else /* CONFIG_SECURITY */
 
 static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
@@ -1424,8 +1424,9 @@ static inline int security_locked_down(enum lockdown_reason what)
 {
 	return 0;
 }
-static inline int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
-				    size_t context_size, u64 id, u64 flags)
+static inline int lsm_fill_user_ctx(struct lsm_ctx __user *uctx,
+				    size_t *uctx_len, void *val, size_t val_len,
+				    u64 id, u64 flags)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 5e16c03936b9..6df97eb6e7d9 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -636,7 +636,6 @@ static int apparmor_getselfattr(unsigned int attr, struct lsm_ctx __user *lx,
 	int error = -ENOENT;
 	struct aa_task_ctx *ctx = task_ctx(current);
 	struct aa_label *label = NULL;
-	size_t total_len = 0;
 	char *value;
 
 	switch (attr) {
@@ -658,22 +657,14 @@ static int apparmor_getselfattr(unsigned int attr, struct lsm_ctx __user *lx,
 
 	if (label) {
 		error = aa_getprocattr(label, &value, false);
-		if (error > 0) {
-			total_len = ALIGN(struct_size(lx, ctx, error), 8);
-			if (total_len > *size)
-				error = -E2BIG;
-			else if (lx)
-				error = lsm_fill_user_ctx(lx, value, error,
-							  LSM_ID_APPARMOR, 0);
-			else
-				error = 1;
-		}
+		if (error > 0)
+			error = lsm_fill_user_ctx(lx, size, value, error,
+						  LSM_ID_APPARMOR, 0);
 		kfree(value);
 	}
 
 	aa_put_label(label);
 
-	*size = total_len;
 	if (error < 0)
 		return error;
 	return 1;
diff --git a/security/security.c b/security/security.c
index 67ded406a5ea..45c4f5440c95 100644
--- a/security/security.c
+++ b/security/security.c
@@ -773,42 +773,49 @@ static int lsm_superblock_alloc(struct super_block *sb)
 
 /**
  * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
- * @ctx: an LSM context to be filled
- * @context: the new context value
- * @context_size: the size of the new context value
+ * @uctx: a userspace LSM context to be filled
+ * @uctx_len: available uctx size (input), used uctx size (output)
+ * @val: the new LSM context value
+ * @val_len: the size of the new LSM context value
  * @id: LSM id
  * @flags: LSM defined flags
  *
- * Fill all of the fields in a user space lsm_ctx structure.
- * Caller is assumed to have verified that @ctx has enough space
- * for @context.
+ * Fill all of the fields in a userspace lsm_ctx structure.
  *
- * Returns 0 on success, -EFAULT on a copyout error, -ENOMEM
- * if memory can't be allocated.
+ * Returns 0 on success, -E2BIG if userspace buffer is not large enough,
+ * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated.
  */
-int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
-		      size_t context_size, u64 id, u64 flags)
+int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
+		      void *val, size_t val_len,
+		      u64 id, u64 flags)
 {
-	struct lsm_ctx *lctx;
-	size_t locallen = struct_size(lctx, ctx, context_size);
+	struct lsm_ctx *nctx = NULL;
+	size_t nctx_len;
 	int rc = 0;
 
-	lctx = kzalloc(locallen, GFP_KERNEL);
-	if (lctx == NULL)
-		return -ENOMEM;
+	nctx_len = ALIGN(struct_size(nctx, ctx, val_len), BITS_PER_LONG / 8);
+	if (nctx_len > *uctx_len) {
+		rc = -E2BIG;
+		goto out;
+	}
 
-	lctx->id = id;
-	lctx->flags = flags;
-	lctx->ctx_len = context_size;
-	lctx->len = locallen;
+	nctx = kzalloc(nctx_len, GFP_KERNEL);
+	if (nctx == NULL) {
+		rc = -ENOMEM;
+		goto out;
+	}
+	nctx->id = id;
+	nctx->flags = flags;
+	nctx->len = nctx_len;
+	nctx->ctx_len = val_len;
+	memcpy(nctx->ctx, val, val_len);
 
-	memcpy(lctx->ctx, context, context_size);
-
-	if (copy_to_user(ctx, lctx, locallen))
+	if (copy_to_user(uctx, nctx, nctx_len))
 		rc = -EFAULT;
 
-	kfree(lctx);
-
+out:
+	kfree(nctx);
+	*uctx_len = nctx_len;
 	return rc;
 }
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1fe30e635923..c32794979aab 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6480,30 +6480,32 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
 	return error;
 }
 
+/**
+ * selinux_getselfattr - Get SELinux current task attributes
+ * @attr: the requested attribute
+ * @ctx: buffer to receive the result
+ * @size: buffer size (input), buffer size used (output)
+ * @flags: unused
+ *
+ * Fill the passed user space @ctx with the details of the requested
+ * attribute.
+ *
+ * Returns the number of attributes on success, an error code otherwise.
+ * There will only ever be one attribute.
+ */
 static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
 			       size_t *size, u32 flags)
 {
-	char *value;
-	size_t total_len;
-	int len;
-	int rc = 0;
+	int rc;
+	char *val;
+	int val_len;
 
-	len = selinux_lsm_getattr(attr, current, &value);
-	if (len < 0)
-		return len;
-
-	total_len = ALIGN(struct_size(ctx, ctx, len), 8);
-
-	if (total_len > *size)
-		rc = -E2BIG;
-	else if (ctx)
-		rc = lsm_fill_user_ctx(ctx, value, len, LSM_ID_SELINUX, 0);
-
-	kfree(value);
-	*size = total_len;
-	if (rc < 0)
-		return rc;
-	return 1;
+	val_len = selinux_lsm_getattr(attr, current, &val);
+	if (val_len < 0)
+		return val_len;
+	rc = lsm_fill_user_ctx(ctx, size, val, val_len, LSM_ID_SELINUX, 0);
+	kfree(val);
+	return (!rc ? 1 : rc);
 }
 
 static int selinux_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 12160d060cc1..99664c8cf867 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3642,28 +3642,17 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
 static int smack_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
 			     size_t *size, u32 flags)
 {
-	struct smack_known *skp = smk_of_current();
-	int total;
-	int slen;
 	int rc;
+	struct smack_known *skp;
 
 	if (attr != LSM_ATTR_CURRENT)
 		return -EOPNOTSUPP;
 
-	slen = strlen(skp->smk_known) + 1;
-	total = ALIGN(slen + sizeof(*ctx), 8);
-	if (total > *size)
-		rc = -E2BIG;
-	else if (ctx)
-		rc = lsm_fill_user_ctx(ctx, skp->smk_known, slen, LSM_ID_SMACK,
-				       0);
-	else
-		rc = 1;
-
-	*size = total;
-	if (rc >= 0)
-		return 1;
-	return rc;
+	skp = smk_of_current();
+	rc = lsm_fill_user_ctx(ctx, size,
+			       skp->smk_known, strlen(skp->smk_known) + 1,
+			       LSM_ID_SMACK, 0);
+	return (!rc ? 1 : rc);
 }
 
 /**
-- 
2.42.0

Re: [RFC PATCH 3/3] lsm: consolidate buffer size handling into lsm_fill_user_ctx()

[Mickaël Salaün]

On Tue, Oct 24, 2023 at 05:35:29PM -0400, Paul Moore wrote:
> While we have a lsm_fill_user_ctx() helper function designed to make
> life easier for LSMs which return lsm_ctx structs to userspace, we
> didn't include all of the buffer length safety checks and buffer
> padding adjustments in the helper.  This led to code duplication
> across the different LSMs and the possibility for mistakes across the
> different LSM subsystems.  In order to reduce code duplication and
> decrease the chances of silly mistakes, we're consolidating all of
> this code into the lsm_fill_user_ctx() helper.
> 
> The buffer padding is also modified from a fixed 8-byte alignment to
> an alignment that matches the word length of the machine
> (BITS_PER_LONG / 8).
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---

> diff --git a/security/security.c b/security/security.c
> index 67ded406a5ea..45c4f5440c95 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -773,42 +773,49 @@ static int lsm_superblock_alloc(struct super_block *sb)
>  
>  /**
>   * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
> - * @ctx: an LSM context to be filled
> - * @context: the new context value
> - * @context_size: the size of the new context value
> + * @uctx: a userspace LSM context to be filled
> + * @uctx_len: available uctx size (input), used uctx size (output)
> + * @val: the new LSM context value
> + * @val_len: the size of the new LSM context value
>   * @id: LSM id
>   * @flags: LSM defined flags
>   *
> - * Fill all of the fields in a user space lsm_ctx structure.
> - * Caller is assumed to have verified that @ctx has enough space
> - * for @context.
> + * Fill all of the fields in a userspace lsm_ctx structure.
>   *
> - * Returns 0 on success, -EFAULT on a copyout error, -ENOMEM
> - * if memory can't be allocated.
> + * Returns 0 on success, -E2BIG if userspace buffer is not large enough,
> + * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated.
>   */
> -int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
> -		      size_t context_size, u64 id, u64 flags)
> +int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
> +		      void *val, size_t val_len,
> +		      u64 id, u64 flags)
>  {
> -	struct lsm_ctx *lctx;
> -	size_t locallen = struct_size(lctx, ctx, context_size);
> +	struct lsm_ctx *nctx = NULL;
> +	size_t nctx_len;
>  	int rc = 0;
>  
> -	lctx = kzalloc(locallen, GFP_KERNEL);
> -	if (lctx == NULL)
> -		return -ENOMEM;
> +	nctx_len = ALIGN(struct_size(nctx, ctx, val_len), BITS_PER_LONG / 8);

Why the arch-dependent constant?

I'm not even sure why we want to align this size. We'll only copy the
actual size right?

> +	if (nctx_len > *uctx_len) {
> +		rc = -E2BIG;
> +		goto out;
> +	}
>  
> -	lctx->id = id;
> -	lctx->flags = flags;
> -	lctx->ctx_len = context_size;
> -	lctx->len = locallen;
> +	nctx = kzalloc(nctx_len, GFP_KERNEL);
> +	if (nctx == NULL) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +	nctx->id = id;
> +	nctx->flags = flags;
> +	nctx->len = nctx_len;
> +	nctx->ctx_len = val_len;
> +	memcpy(nctx->ctx, val, val_len);
>  
> -	memcpy(lctx->ctx, context, context_size);
> -
> -	if (copy_to_user(ctx, lctx, locallen))
> +	if (copy_to_user(uctx, nctx, nctx_len))
>  		rc = -EFAULT;
>  
> -	kfree(lctx);
> -
> +out:
> +	kfree(nctx);
> +	*uctx_len = nctx_len;
>  	return rc;
>  }
>  
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1fe30e635923..c32794979aab 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6480,30 +6480,32 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
>  	return error;
>  }
>  
> +/**
> + * selinux_getselfattr - Get SELinux current task attributes
> + * @attr: the requested attribute
> + * @ctx: buffer to receive the result
> + * @size: buffer size (input), buffer size used (output)
> + * @flags: unused
> + *
> + * Fill the passed user space @ctx with the details of the requested
> + * attribute.
> + *
> + * Returns the number of attributes on success, an error code otherwise.
> + * There will only ever be one attribute.
> + */
>  static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
>  			       size_t *size, u32 flags)
>  {
> -	char *value;
> -	size_t total_len;
> -	int len;
> -	int rc = 0;
> +	int rc;
> +	char *val;
> +	int val_len;
>  
> -	len = selinux_lsm_getattr(attr, current, &value);
> -	if (len < 0)
> -		return len;
> -
> -	total_len = ALIGN(struct_size(ctx, ctx, len), 8);
> -
> -	if (total_len > *size)
> -		rc = -E2BIG;
> -	else if (ctx)
> -		rc = lsm_fill_user_ctx(ctx, value, len, LSM_ID_SELINUX, 0);
> -
> -	kfree(value);
> -	*size = total_len;
> -	if (rc < 0)
> -		return rc;
> -	return 1;
> +	val_len = selinux_lsm_getattr(attr, current, &val);
> +	if (val_len < 0)
> +		return val_len;
> +	rc = lsm_fill_user_ctx(ctx, size, val, val_len, LSM_ID_SELINUX, 0);
> +	kfree(val);
> +	return (!rc ? 1 : rc);
>  }
>  
>  static int selinux_setselfattr(unsigned int attr, struct lsm_ctx *ctx,

Re: [RFC PATCH 3/3] lsm: consolidate buffer size handling into lsm_fill_user_ctx()

[Paul Moore]

On Thu, Oct 26, 2023 at 11:13 AM Mickaël Salaün <mic@digikod.net> wrote:
> On Tue, Oct 24, 2023 at 05:35:29PM -0400, Paul Moore wrote:
> > While we have a lsm_fill_user_ctx() helper function designed to make
> > life easier for LSMs which return lsm_ctx structs to userspace, we
> > didn't include all of the buffer length safety checks and buffer
> > padding adjustments in the helper.  This led to code duplication
> > across the different LSMs and the possibility for mistakes across the
> > different LSM subsystems.  In order to reduce code duplication and
> > decrease the chances of silly mistakes, we're consolidating all of
> > this code into the lsm_fill_user_ctx() helper.
> >
> > The buffer padding is also modified from a fixed 8-byte alignment to
> > an alignment that matches the word length of the machine
> > (BITS_PER_LONG / 8).
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > ---
>
> > diff --git a/security/security.c b/security/security.c
> > index 67ded406a5ea..45c4f5440c95 100644
> > --- a/security/security.c
> > +++ b/security/security.c

...

> > +int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
> > +                   void *val, size_t val_len,
> > +                   u64 id, u64 flags)
> >  {
> > -     struct lsm_ctx *lctx;
> > -     size_t locallen = struct_size(lctx, ctx, context_size);
> > +     struct lsm_ctx *nctx = NULL;
> > +     size_t nctx_len;
> >       int rc = 0;
> >
> > -     lctx = kzalloc(locallen, GFP_KERNEL);
> > -     if (lctx == NULL)
> > -             return -ENOMEM;
> > +     nctx_len = ALIGN(struct_size(nctx, ctx, val_len), BITS_PER_LONG / 8);
>
> Why the arch-dependent constant?

My thinking is that most arches tend to perform better when data is
aligned on a word boundary and this should help achieve that in a way
that doesn't assume the arch's word length.  If you have an idea on
how to do this differently I'm open to suggestions.

It's worth noting that this is something we can change in the future
as the lsm_ctx struct has the len field which we can use for arbitrary
amounts of padding, including none.

> I'm not even sure why we want to align this size. We'll only copy the
> actual size right?

We allocate, zero out, and copy @nctx_len/@nctx->len.

> > +     if (nctx_len > *uctx_len) {
> > +             rc = -E2BIG;
> > +             goto out;
> > +     }
> >
> > -     lctx->id = id;
> > -     lctx->flags = flags;
> > -     lctx->ctx_len = context_size;
> > -     lctx->len = locallen;
> > +     nctx = kzalloc(nctx_len, GFP_KERNEL);
> > +     if (nctx == NULL) {
> > +             rc = -ENOMEM;
> > +             goto out;
> > +     }
> > +     nctx->id = id;
> > +     nctx->flags = flags;
> > +     nctx->len = nctx_len;
> > +     nctx->ctx_len = val_len;
> > +     memcpy(nctx->ctx, val, val_len);
> >
> > -     memcpy(lctx->ctx, context, context_size);
> > -
> > -     if (copy_to_user(ctx, lctx, locallen))
> > +     if (copy_to_user(uctx, nctx, nctx_len))
> >               rc = -EFAULT;
> >
> > -     kfree(lctx);
> > -
> > +out:
> > +     kfree(nctx);
> > +     *uctx_len = nctx_len;
> >       return rc;
> >  }

-- 
paul-moore.com

Re: [RFC PATCH 3/3] lsm: consolidate buffer size handling into lsm_fill_user_ctx()

[Aishwarya TCV]

On 24/10/2023 22:35, Paul Moore wrote:
> While we have a lsm_fill_user_ctx() helper function designed to make
> life easier for LSMs which return lsm_ctx structs to userspace, we
> didn't include all of the buffer length safety checks and buffer
> padding adjustments in the helper.  This led to code duplication
> across the different LSMs and the possibility for mistakes across the
> different LSM subsystems.  In order to reduce code duplication and
> decrease the chances of silly mistakes, we're consolidating all of
> this code into the lsm_fill_user_ctx() helper.
> 
> The buffer padding is also modified from a fixed 8-byte alignment to
> an alignment that matches the word length of the machine
> (BITS_PER_LONG / 8).
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  include/linux/security.h   |  9 ++++---
>  security/apparmor/lsm.c    | 15 +++--------
>  security/security.c        | 55 +++++++++++++++++++++-----------------
>  security/selinux/hooks.c   | 42 +++++++++++++++--------------
>  security/smack/smack_lsm.c | 23 +++++-----------
>  5 files changed, 67 insertions(+), 77 deletions(-)
> 

Hi Paul,

While building the kernel against next-master for arch arm64
> security/security.c:810:2: warning: ‘memcpy’ offset 32 is out of the bounds [0, 0] [-Warray-bounds]
warning is observed. On some other architectures like i386 and x86_64,
an error is observed. > arch/x86/include/asm/string_32.h:150:25: error:
‘__builtin_memcpy’ offset 32 is out of the bounds [0, 0]
[-Werror=array-bounds]

The links of the logs is listed below:
https://storage.kernelci.org/next/master/next-20231220/arm64/defconfig/gcc-10/logs/build-warnings.log
https://storage.kernelci.org/next/master/next-20231220/i386/i386_defconfig/gcc-10/logs/build-errors.log

The logs of all the architecture built against next-master can be found
here (select the 'All' category in the table to view):
https://linux.kernelci.org/build/next/branch/master/kernel/next-20231220/


Find this issue filed at KSPP/linux here:
https://github.com/KSPP/linux/issues/347


A bisect done by building kernel against next-master for arch arm64
(full log below) identified this patch as introducing the failure.

git bisect log:
git bisect start
# good: [b85ea95d086471afb4ad062012a4d73cd328fa86] Linux 6.7-rc1
git bisect good b85ea95d086471afb4ad062012a4d73cd328fa86
# bad: [5ba73bec5e7b0494da7fdca3e003d8b97fa932cd] Add linux-next
specific files for 20231114
git bisect bad 5ba73bec5e7b0494da7fdca3e003d8b97fa932cd
# good: [a15c6466b909f03889150df57b227702a7bd6bd5] Merge branch
'for-next' of
git://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git
git bisect good a15c6466b909f03889150df57b227702a7bd6bd5
# good: [6a8b8b208098a27488a3649966d64894da948a02] Merge branch
'for-next' of
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
git bisect good 6a8b8b208098a27488a3649966d64894da948a02
# bad: [81105901f053f9684a111c0569eb35474b2a86f9] Merge branch 'next' of
git://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git
git bisect bad 81105901f053f9684a111c0569eb35474b2a86f9
# bad: [585a8722efb6f823e961f16bd9be818f994d4804] Merge branch
'rcu/next' of
git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
git bisect bad 585a8722efb6f823e961f16bd9be818f994d4804
# good: [c867caae623b3dd488a849df5538e79a59b0a47f] Merge branch into
tip/master: 'x86/percpu'
git bisect good c867caae623b3dd488a849df5538e79a59b0a47f
# bad: [381a25d3e3d440ccc05de8ddd56a055423ac9fe5] Merge branch 'next' of
git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git
git bisect bad 381a25d3e3d440ccc05de8ddd56a055423ac9fe5
# good: [762c934317e6f4b576eb4aa75e5facf4968a4a8f] SELinux: Add selfattr
hooks
git bisect good 762c934317e6f4b576eb4aa75e5facf4968a4a8f
# good: [fdcf699b60712ecd6e41d9fc09137279257a4bf8] lsm: correct error
codes in security_getselfattr()
git bisect good fdcf699b60712ecd6e41d9fc09137279257a4bf8
# bad: [9ba8802c8b66fbde2ee32ab4c44cd418f9444486] lsm: convert
security_setselfattr() to use memdup_user()
git bisect bad 9ba8802c8b66fbde2ee32ab4c44cd418f9444486
# bad: [41793202292fd2acf99fdc09eff8323cc27c80eb] lsm: align based on
pointer length in lsm_fill_user_ctx()
git bisect bad 41793202292fd2acf99fdc09eff8323cc27c80eb
# bad: [d7cf3412a9f6c547e5ee443fa7644e08898aa3e2] lsm: consolidate
buffer size handling into lsm_fill_user_ctx()
git bisect bad d7cf3412a9f6c547e5ee443fa7644e08898aa3e2
# first bad commit: [d7cf3412a9f6c547e5ee443fa7644e08898aa3e2] lsm:
consolidate buffer size handling into lsm_fill_user_ctx()

Thanks,
Aishwarya

Re: [RFC PATCH 3/3] lsm: consolidate buffer size handling into lsm_fill_user_ctx()

[Paul Moore]

On Wed, Dec 20, 2023 at 5:31 PM Aishwarya TCV <aishwarya.tcv@arm.com> wrote:
> On 24/10/2023 22:35, Paul Moore wrote:
> > While we have a lsm_fill_user_ctx() helper function designed to make
> > life easier for LSMs which return lsm_ctx structs to userspace, we
> > didn't include all of the buffer length safety checks and buffer
> > padding adjustments in the helper.  This led to code duplication
> > across the different LSMs and the possibility for mistakes across the
> > different LSM subsystems.  In order to reduce code duplication and
> > decrease the chances of silly mistakes, we're consolidating all of
> > this code into the lsm_fill_user_ctx() helper.
> >
> > The buffer padding is also modified from a fixed 8-byte alignment to
> > an alignment that matches the word length of the machine
> > (BITS_PER_LONG / 8).
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > ---
> >  include/linux/security.h   |  9 ++++---
> >  security/apparmor/lsm.c    | 15 +++--------
> >  security/security.c        | 55 +++++++++++++++++++++-----------------
> >  security/selinux/hooks.c   | 42 +++++++++++++++--------------
> >  security/smack/smack_lsm.c | 23 +++++-----------
> >  5 files changed, 67 insertions(+), 77 deletions(-)
>
> Hi Paul,
>
> While building the kernel against next-master for arch arm64
> > security/security.c:810:2: warning: ‘memcpy’ offset 32 is out of the bounds [0, 0] [-Warray-bounds]
> warning is observed. On some other architectures like i386 and x86_64,
> an error is observed. > arch/x86/include/asm/string_32.h:150:25: error:
> ‘__builtin_memcpy’ offset 32 is out of the bounds [0, 0]
> [-Werror=array-bounds]

I believe the code is correct, I'm guessing this is simply a question
of the compiler not seeing whatever syntactic magic is required for
your compilation flags.  While I'm not entirely sure of the "[0, 0]"
"bounds" in the warning/error message, if that were a
offset/limit/length a double zero value would also seem to indicate
this is more of a compiler annotation issue than a code issue.

Looking at the lsm_ctx definition in include/uapi/linux/lsm.h I see
the following:

  struct lsm_ctx {
    __u64 id;       /* offset:  0 */
    __u64 flags;    /* offset:  8 */
    __u64 len;      /* offset: 16 */
    __u64 ctx_len;  /* offset: 24 */
    __u8 ctx[];     /* offset: 32 */
  };

and given that the offending line of code is trying to do a memcpy
into the ctx field, an offset of 32 looks correct to me.

Suggestions on how to annotate the struct, or the code doing the
memcpy() are welcome.

-- 
paul-moore.com

Re: [RFC PATCH 3/3] lsm: consolidate buffer size handling into lsm_fill_user_ctx()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

On Wed, Dec 20, 2023 at 08:40:24PM -0500, Paul Moore wrote:

> Looking at the lsm_ctx definition in include/uapi/linux/lsm.h I see
> the following:

>   struct lsm_ctx {
>     __u64 id;       /* offset:  0 */
>     __u64 flags;    /* offset:  8 */
>     __u64 len;      /* offset: 16 */
>     __u64 ctx_len;  /* offset: 24 */
>     __u8 ctx[];     /* offset: 32 */
>   };

> and given that the offending line of code is trying to do a memcpy
> into the ctx field, an offset of 32 looks correct to me.

> Suggestions on how to annotate the struct, or the code doing the
> memcpy() are welcome.

You're looking for a __counted_by() annotation here I think.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 3/3] lsm: consolidate buffer size handling into lsm_fill_user_ctx()

[Paul Moore]

On Thu, Dec 21, 2023 at 8:01 AM Mark Brown <broonie@kernel.org> wrote:
> On Wed, Dec 20, 2023 at 08:40:24PM -0500, Paul Moore wrote:
> > Looking at the lsm_ctx definition in include/uapi/linux/lsm.h I see
> > the following:
>
> >   struct lsm_ctx {
> >     __u64 id;       /* offset:  0 */
> >     __u64 flags;    /* offset:  8 */
> >     __u64 len;      /* offset: 16 */
> >     __u64 ctx_len;  /* offset: 24 */
> >     __u8 ctx[];     /* offset: 32 */
> >   };
>
> > and given that the offending line of code is trying to do a memcpy
> > into the ctx field, an offset of 32 looks correct to me.
>
> > Suggestions on how to annotate the struct, or the code doing the
> > memcpy() are welcome.
>
> You're looking for a __counted_by() annotation here I think.

Can you verify and submit a patch for that?  I'm asking because my
build/toolchain configuration never produced these warnings/errors
during my testing.

-- 
paul-moore.com

Re: [RFC PATCH 3/3] lsm: consolidate buffer size handling into lsm_fill_user_ctx()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

On Thu, Dec 21, 2023 at 10:21:04AM -0500, Paul Moore wrote:
> On Thu, Dec 21, 2023 at 8:01 AM Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Dec 20, 2023 at 08:40:24PM -0500, Paul Moore wrote:

> > > Suggestions on how to annotate the struct, or the code doing the
> > > memcpy() are welcome.

> > You're looking for a __counted_by() annotation here I think.

> Can you verify and submit a patch for that?  I'm asking because my
> build/toolchain configuration never produced these warnings/errors
> during my testing.

Huh, actually it's not __counted_by() since this shows up with
arm64/gcc-10 (and some other arches) which doesn't have that.  I'll
submit the __counted_by() patch anyway since it's clearly a good
annotation but it doesn't actually shut the warning up in this case.
Adding the hardening people, and I'll have a further look.

You can reproduce with

   tuxmake -r docker -k defconfig -a arm64 -t gcc-10

using https://www.tuxmake.org/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/3] LSM syscall tweaks

[Paul Moore]

On Tue, Oct 24, 2023 at 5:39 PM Paul Moore <paul@paul-moore.com> wrote:
>
> Three rather small LSM patches to address some minor issues found during
> the review of the latest LSM syscall patchset that now lives in the
> lsm/next-queue tree.
>
> I'm marking these as RFC patches as they have yet to be properly tested,
> but I'm building a kernel now to do that and I'll report back when testing
> has completed.  In the meantime, reviews and ACKs are appreciated.

I went ahead and merged these into lsm/dev-staging and rebased the
branch on Linus' latest to incorporate the syscall additions in his
tree.  As the merge window is open, I did not do the corresponding
update to the lsm/next branch, that will be updated when the merge
window is closed and -rc1 is released.

-- 
paul-moore.com

Re: [RFC PATCH 0/3] LSM syscall tweaks

[Casey Schaufler]

On 11/1/2023 2:24 PM, Paul Moore wrote:
> On Tue, Oct 24, 2023 at 5:39 PM Paul Moore <paul@paul-moore.com> wrote:
>> Three rather small LSM patches to address some minor issues found during
>> the review of the latest LSM syscall patchset that now lives in the
>> lsm/next-queue tree.
>>
>> I'm marking these as RFC patches as they have yet to be properly tested,
>> but I'm building a kernel now to do that and I'll report back when testing
>> has completed.  In the meantime, reviews and ACKs are appreciated.
> I went ahead and merged these into lsm/dev-staging and rebased the
> branch on Linus' latest to incorporate the syscall additions in his
> tree.  As the merge window is open, I did not do the corresponding
> update to the lsm/next branch, that will be updated when the merge
> window is closed and -rc1 is released.

Excellent. Thank you.

Re: [RFC PATCH 0/3] LSM syscall tweaks

[Paul Moore]

On Tue, Oct 24, 2023 at 5:39 PM Paul Moore <paul@paul-moore.com> wrote:
>
> Three rather small LSM patches to address some minor issues found during
> the review of the latest LSM syscall patchset that now lives in the
> lsm/next-queue tree.
>
> I'm marking these as RFC patches as they have yet to be properly tested,
> but I'm building a kernel now to do that and I'll report back when testing
> has completed.  In the meantime, reviews and ACKs are appreciated.

These have been merged into lsm/dev.

-- 
paul-moore.com