On 12/22/2023 5:23 PM, Paul Moore wrote:
> On Tue, Dec 19, 2023 at 4:09 AM Alfred Piccioni <alpic@xxxxxxxxxx> wrote:
>> Some ioctl commands do not require ioctl permission, but are routed to
>> other permissions such as FILE_GETATTR or FILE_SETATTR. This routing is
>> done by comparing the ioctl cmd to a set of 64-bit flags (FS_IOC_*).
>>
>> However, if a 32-bit process is running on a 64-bit kernel, it emits
>> 32-bit flags (FS_IOC32_*) for certain ioctl operations. These flags are
>> being checked erroneously, which leads to these ioctl operations being
>> routed to the ioctl permission, rather than the correct file
>> permissions.
>>
>> This was also noted in a RED-PEN finding from a while back -
>> "/* RED-PEN how should LSM module know it's handling 32bit? */".
>>
>> This patch introduces a new hook, security_file_ioctl_compat, that is
>> called from the compat ioctl syscall. All current LSMs have been changed
>> to support this hook.
>>
>> Reviewing the three places where we are currently using
>> security_file_ioctl, it appears that only SELinux needs a dedicated
>> compat change; TOMOYO and SMACK appear to be functional without any
>> change.
>>
>> Fixes: 0b24dcb7f2f7 ("Revert "selinux: simplify ioctl checking"")
>> Signed-off-by: Alfred Piccioni <alpic@xxxxxxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx
>> ---
>> fs/ioctl.c | 3 +--
>> include/linux/lsm_hook_defs.h | 2 ++
>> include/linux/security.h | 7 +++++++
>> security/security.c | 17 +++++++++++++++++
>> security/selinux/hooks.c | 28 ++++++++++++++++++++++++++++
>> security/smack/smack_lsm.c | 1 +
>> security/tomoyo/tomoyo.c | 1 +
>> 7 files changed, 57 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ioctl.c b/fs/ioctl.c
>> index f5fd99d6b0d4..76cf22ac97d7 100644
>> --- a/fs/ioctl.c
>> +++ b/fs/ioctl.c
>> @@ -920,8 +920,7 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd,
>> if (!f.file)
>> return -EBADF;
>>
>> - /* RED-PEN how should LSM module know it's handling 32bit? */
>> - error = security_file_ioctl(f.file, cmd, arg);
>> + error = security_file_ioctl_compat(f.file, cmd, arg);
>> if (error)
>> goto out;
> This is interesting ... if you look at the normal ioctl() syscall
> definition in the kernel you see 'ioctl(unsigned int fd, unsigned int
> cmd, unsigned long arg)' and if you look at the compat definition you
> see 'ioctl(unsigned int fd, unsigned int cmd, compat_ulong_t arg)'. I
> was expecting the second parameter, @cmd, to be a long type in the
> normal definition, but it is an int type in both cases. It looks like
> it has been that way long enough that it is correct, but I'm a little
> lost ...
>
>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>> index ac962c4cb44b..626aa8cf930d 100644
>> --- a/include/linux/lsm_hook_defs.h
>> +++ b/include/linux/lsm_hook_defs.h
>> @@ -171,6 +171,8 @@ LSM_HOOK(int, 0, file_alloc_security, struct file *file)
>> LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
>> LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
>> unsigned long arg)
>> +LSM_HOOK(int, 0, file_ioctl_compat, struct file *file, unsigned int cmd,
>> + unsigned long arg)
>> LSM_HOOK(int, 0, mmap_addr, unsigned long addr)
>> LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot,
>> unsigned long prot, unsigned long flags)
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 5f16eecde00b..22a82b7c59f1 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -389,6 +389,7 @@ int security_file_permission(struct file *file, int mask);
>> int security_file_alloc(struct file *file);
>> void security_file_free(struct file *file);
>> int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
>> +int security_file_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg);
>> int security_mmap_file(struct file *file, unsigned long prot,
>> unsigned long flags);
>> int security_mmap_addr(unsigned long addr);
>> @@ -987,6 +988,12 @@ static inline int security_file_ioctl(struct file *file, unsigned int cmd,
>> return 0;
>> }
>>
>> +static inline int security_file_ioctl_compat(struct file *file, unsigned int cmd,
>> + unsigned long arg)
>> +{
>> + return 0;
>> +}
>> +
>> static inline int security_mmap_file(struct file *file, unsigned long prot,
>> unsigned long flags)
>> {
>> diff --git a/security/security.c b/security/security.c
>> index 23b129d482a7..5c16ffc99b1e 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2648,6 +2648,23 @@ int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> }
>> EXPORT_SYMBOL_GPL(security_file_ioctl);
>>
>> +/**
>> + * security_file_ioctl_compat() - Check if an ioctl is allowed in 32-bit compat mode
>> + * @file: associated file
>> + * @cmd: ioctl cmd
>> + * @arg: ioctl arguments
>> + *
>> + * Compat version of security_file_ioctl() that correctly handles 32-bit processes
>> + * running on 64-bit kernels.
>> + *
>> + * Return: Returns 0 if permission is granted.
>> + */
>> +int security_file_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg)
>> +{
>> + return call_int_hook(file_ioctl_compat, 0, file, cmd, arg);
>> +}
>> +EXPORT_SYMBOL_GPL(security_file_ioctl_compat);
>> +
>> static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
>> {
>> /*
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 2aa0e219d721..c617ae21dba8 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -3731,6 +3731,33 @@ static int selinux_file_ioctl(struct file *file, unsigned int cmd,
>> return error;
>> }
>>
>> +static int selinux_file_ioctl_compat(struct file *file, unsigned int cmd,
>> + unsigned long arg)
>> +{
>> + /*
>> + * If we are in a 64-bit kernel running 32-bit userspace, we need to make
>> + * sure we don't compare 32-bit flags to 64-bit flags.
>> + */
>> + switch (cmd) {
>> + case FS_IOC32_GETFLAGS:
>> + cmd = FS_IOC_GETFLAGS;
>> + break;
>> + case FS_IOC32_SETFLAGS:
>> + cmd = FS_IOC_SETFLAGS;
>> + break;
>> + case FS_IOC32_GETVERSION:
>> + cmd = FS_IOC_GETVERSION;
>> + break;
>> + case FS_IOC32_SETVERSION:
>> + cmd = FS_IOC_SETVERSION;
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + return selinux_file_ioctl(file, cmd, arg);
>> +}
> Is it considered valid for a native 64-bit task to use 32-bit
> FS_IO32_XXX flags? If not, do we want to remove the FS_IO32_XXX flag
> checks in selinux_file_ioctl()?
>
>> static int default_noexec __ro_after_init;
>>
>> static int file_map_prot_check(struct file *file, unsigned long prot, int shared)
>> @@ -7036,6 +7063,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
>> LSM_HOOK_INIT(file_permission, selinux_file_permission),
>> LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
>> LSM_HOOK_INIT(file_ioctl, selinux_file_ioctl),
>> + LSM_HOOK_INIT(file_ioctl_compat, selinux_file_ioctl_compat),
>> LSM_HOOK_INIT(mmap_file, selinux_mmap_file),
>> LSM_HOOK_INIT(mmap_addr, selinux_mmap_addr),
>> LSM_HOOK_INIT(file_mprotect, selinux_file_mprotect),
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 65130a791f57..1f1ea8529421 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -4973,6 +4973,7 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
>>
>> LSM_HOOK_INIT(file_alloc_security, smack_file_alloc_security),
>> LSM_HOOK_INIT(file_ioctl, smack_file_ioctl),
>> + LSM_HOOK_INIT(file_ioctl_compat, smack_file_ioctl),
>> LSM_HOOK_INIT(file_lock, smack_file_lock),
>> LSM_HOOK_INIT(file_fcntl, smack_file_fcntl),
>> LSM_HOOK_INIT(mmap_file, smack_mmap_file),
>> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
>> index 25006fddc964..298d182759c2 100644
>> --- a/security/tomoyo/tomoyo.c
>> +++ b/security/tomoyo/tomoyo.c
>> @@ -568,6 +568,7 @@ static struct security_hook_list tomoyo_hooks[] __ro_after_init = {
>> LSM_HOOK_INIT(path_rename, tomoyo_path_rename),
>> LSM_HOOK_INIT(inode_getattr, tomoyo_inode_getattr),
>> LSM_HOOK_INIT(file_ioctl, tomoyo_file_ioctl),
>> + LSM_HOOK_INIT(file_ioctl_compat, tomoyo_file_ioctl),
>> LSM_HOOK_INIT(path_chmod, tomoyo_path_chmod),
>> LSM_HOOK_INIT(path_chown, tomoyo_path_chown),
>> LSM_HOOK_INIT(path_chroot, tomoyo_path_chroot),
> I agree that it looks like Smack and TOMOYO should be fine, but I
> would like to hear from Casey and Tetsuo to confirm.
Smack should be OK.
