On Mon, Dec 18, 2023 at 8:46 AM Stephen Smalley stephen.smalley.work@gmail.com wrote:
On Mon, Dec 18, 2023 at 7:43 AM Alfred Piccioni alpic@google.com 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 emmits
s/emmits/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 replaces security_file_ioctl if the CONFIG_COMPAT flag is on. All current LSMs have been changed to hook into the compat flag.
It doesn't (or shouldn't) replace security_file_ioctl, and the hook doesn't appear to be conditional on CONFIG_COMPAT per se. It is a new hook that is called from the compat ioctl syscall. The old hook continues to be used from the regular ioctl syscall and elsewhere.
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@google.com Cc: stable@vger.kernel.org
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c index 83ef66644c21..170687b5985b 100644 --- a/fs/overlayfs/inode.c +++ b/fs/overlayfs/inode.c @@ -751,7 +751,11 @@ static int ovl_security_fileattr(const struct path *realpath, struct fileattr *f else cmd = fa->fsx_valid ? FS_IOC_FSGETXATTR : FS_IOC_GETFLAGS;
+#ifdef CONFIG_COMPAT
err = security_file_ioctl_compat(file, cmd, 0);+# else err = security_file_ioctl(file, cmd, 0); +#endif
I don't understand why you made this change, possibly a leftover of an earlier version of the patch that tried to replace security_file_ioctl() everywhere?
By the way, for extra credit, you could augment the ioctl tests in the selinux-testsuite to also exercise this new hook and confirm that it works correctly. See https://github.com/SELinuxProject/selinux-testsuite particularly tests/ioctl and policy/test_ioctl.te. Feel free to ask for help on that.