Received-SPF: pass (google.com: domain of linux-kernel+bounces-102383-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 13 Mar 2024 14:46:21 -0400
Message-ID: <6353ba2abd868cd83186f54e7b71c840@paul-moore.com>
Precedence: bulk
MIME-Version: 1.0 
Content-Type: text/plain; charset=utf-8 
Content-Disposition: inline 
Content-Transfer-Encoding: 8bit
From: Paul Moore <paul@paul-moore.com>
To: Casey Schaufler <casey@schaufler-ca.com>, "Dmitry V. Levin" <ldv@strace.io>, LSM List <linux-security-module@vger.kernel.org>
Cc: Linux kernel mailing list <linux-kernel@vger.kernel.org>, linux-api@vger.kernel.org, =?UTF-8?Q?Micka=C3=ABl_Sala=C3=BCn?= <mic@digikod.net>, James Morris <jmorris@namei.org>, Serge Hallyn <serge@hallyn.com>, John Johansen <john.johansen@canonical.com>, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>, Stephen Smalley <stephen.smalley.work@gmail.com>, Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: [PATCH v2] LSM: use 32 bit compatible data types in LSM syscalls.
References: <045f54ea-4057-43b8-81e2-5cc1b3966d04@schaufler-ca.com>
In-Reply-To: <045f54ea-4057-43b8-81e2-5cc1b3966d04@schaufler-ca.com>

On Mar 13, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> 
> LSM: use 32 bit compatible data types in LSM syscalls.
> 
> Change the size paramters in lsm_list_modules(), lsm_set_self_attr()

s/paramters/parameters/

> and lsm_get_self_attr() from size_t to u32. This avoids the need to
> have different interfaces for 32 and 64 bit systems.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

We should add the following 'Fixes:' tags as well as a stable marking:

  Cc: stable@vger.kernel.org
  Fixes: a04a1198088a ("LSM: syscalls for current process attributes")
  Fixes: ad4aff9ec25f ("LSM: Create lsm_list_modules system call")

> ---
>  include/linux/lsm_hook_defs.h                        |  4 ++--
>  include/linux/security.h                             |  8 ++++----
>  security/apparmor/lsm.c                              |  4 ++--
>  security/lsm_syscalls.c                              | 10 +++++-----
>  security/security.c                                  | 14 +++++++-------
>  security/selinux/hooks.c                             |  4 ++--
>  security/smack/smack_lsm.c                           |  4 ++--
>  tools/testing/selftests/lsm/common.h                 |  6 +++---
>  tools/testing/selftests/lsm/lsm_get_self_attr_test.c | 12 ++++++------
>  tools/testing/selftests/lsm/lsm_list_modules_test.c  |  8 ++++----
>  tools/testing/selftests/lsm/lsm_set_self_attr_test.c |  6 +++---
>  11 files changed, 40 insertions(+), 40 deletions(-)

..

> diff --git a/security/security.c b/security/security.c
> index 7035ee35a393..a0f9caf89ae1 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -810,7 +810,7 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
>  	nctx->ctx_len = val_len;
>  	memcpy(nctx->ctx, val, val_len);
>  
> -	if (copy_to_user(uctx, nctx, nctx_len))
> +	if (uctx && copy_to_user(uctx, nctx, nctx_len))
>  		rc = -EFAULT;

Hey, where did that @uctx check come from?

I'm trying to work through if that is a good/bad change, but regardless
of if we want to make that change, it really should be in a separate
patch as it has nothing to do with the syscall parameter changes.

> diff --git a/tools/testing/selftests/lsm/lsm_get_self_attr_test.c b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
> index e0e313d9047a..288302a444e0 100644
> --- a/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
> +++ b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
> @@ -76,8 +76,8 @@ TEST(flags_zero_lsm_get_self_attr)
>  {
>  	const long page_size = sysconf(_SC_PAGESIZE);
>  	struct lsm_ctx *ctx = calloc(page_size, 1);
> -	__u64 *syscall_lsms = calloc(page_size, 1);
> -	size_t size;
> +	__u32 *syscall_lsms = calloc(page_size, 1);

I believe that should remain a __u64 pointer as we didn't change the
first parameter to lsm_list_modules().  I'm guessing this was an victim
of an overzealous /u64/u32/ search-n-replace going from v1 to v2.

> +	__u32 size;
>  	int lsmcount;
>  	int i;
>  

In the interest of speeding things along, I'm happy to make the above
changes while merging Casey, but if you would prefer to do a respin
that's fine with me - let me know either way so I can plan accordingly.

--
paul-moore.com