linux-kernel - Re: [PATCH v2] LSM: use 32 bit compatible data types in LSM syscalls.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5a1cf6bc-ac51-44e3-b3d3-ade727781cf6@schaufler-ca.com>
Date: Wed, 13 Mar 2024 11:57:52 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: Paul Moore <paul@...l-moore.com>, "Dmitry V. Levin" <ldv@...ace.io>,
 LSM List <linux-security-module@...r.kernel.org>
Cc: Linux kernel mailing list <linux-kernel@...r.kernel.org>,
 linux-api@...r.kernel.org, Mickaël Salaün
 <mic@...ikod.net>, James Morris <jmorris@...ei.org>,
 Serge Hallyn <serge@...lyn.com>, John Johansen
 <john.johansen@...onical.com>,
 Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
 Stephen Smalley <stephen.smalley.work@...il.com>,
 Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v2] LSM: use 32 bit compatible data types in LSM syscalls.

On 3/13/2024 11:46 AM, Paul Moore wrote:
> On Mar 13, 2024 Casey Schaufler <casey@...aufler-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@...aufler-ca.com>
> We should add the following 'Fixes:' tags as well as a stable marking:
>
>   Cc: stable@...r.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.

I'll respin. Shouldn't take very long.