On Mar 13, 2024 Casey Schaufler <[email protected]> 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 <[email protected]>
We should add the following 'Fixes:' tags as well as a stable marking:
Cc: [email protected]
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
On 3/13/2024 11:46 AM, Paul Moore wrote:
> On Mar 13, 2024 Casey Schaufler <[email protected]> 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 <[email protected]>
> We should add the following 'Fixes:' tags as well as a stable marking:
>
> Cc: [email protected]
> 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.