Re: [PATCH] security: new security_file_ioctl_compat() hook

[Casey Schaufler]

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.