On 12/18/2023 6:16 AM, Alfred Piccioni 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 syscal. 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
This *really* needs to go the the LSM email list:
linux-security-module@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 | 26 ++++++++++++++++++++++++++
> security/smack/smack_lsm.c | 1 +
> security/tomoyo/tomoyo.c | 1 +
> 7 files changed, 55 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;
>
> 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)
Please add a flags parameter to file_ioctl() rather than a new hook.
> 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..de96d156e6ea 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3731,6 +3731,31 @@ 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_GETFLAGS;
> + 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);
> +}
> +
> static int default_noexec __ro_after_init;
>
> static int file_map_prot_check(struct file *file, unsigned long prot, int shared)
> @@ -7036,6 +7061,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),
>
> base-commit: 196e95aa8305aecafc4e1857b7d3eff200d953b6
