lsm: new security_file_ioctl_compat() hook

commit f1bb47a31dff6d4b34fb14e99850860ee74bb003 upstream.

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.

Cc: stable@vger.kernel.org
Fixes: 0b24dcb7f2f7 ("Revert "selinux: simplify ioctl checking"")
Signed-off-by: Alfred Piccioni <alpic@google.com>
Reviewed-by: Stephen Smalley <stephen.smalley.work@gmail.com>
[PM: subject tweak, line length fixes, and alignment corrections]
Signed-off-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/fs/ioctl.c b/fs/ioctl.c
index f5fd99d6b0d4ed981811797e35f254c98cd70e25..76cf22ac97d76256104a6dbc4f819247168ce769 100644 (file)
--- 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 ff217a5ce552143e39717acc4a25ed9c8109fe7b..472cb16458b0b1f628d3e3223148ff2e6155ed39 100644 (file)
--- 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 1d1df326c881c7e1a97d748066d046590c4aea72..9d3138c6364c36a6af0417c609f548b08073a4c3 100644 (file)
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -389,6 +389,8 @@ 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 +989,13 @@ 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 dcb3e7014f9bdd4b6115f0407c5b1d7d3a588e9c..266cec94369b2dad7330107d8e5dedec3b0a6948 100644 (file)
--- a/security/security.c
+++ b/security/security.c
@@ -2648,6 +2648,24 @@ 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 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 0fb890bd72cb6589a31c5c716c4ab68abfc3881e..102a1f0c09e0cb7030ab04457cd8b127221c4164 100644 (file)
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3725,6 +3725,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);
+}
+
 static int default_noexec __ro_after_init;
 
 static int file_map_prot_check(struct file *file, unsigned long prot, int shared)
@@ -7037,6 +7064,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 65130a791f5730c4f1e1eb6e442d0c31684c66ad..1f1ea8529421fd949c0c85f4bdf0e5f0d4d26b21 100644 (file)
--- 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 255f1b4702955b355a0ae5c678443cd55119ef2c..1bf4b3da9cbc50cb149b4d6b2c3832d5eefe2c11 100644 (file)
--- 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),