On Mon, Dec 18, 2023 at 10:58 AM Stephen Smalley stephen.smalley.work@gmail.com wrote:
On Mon, Dec 18, 2023 at 9:17 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 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
s/syscal/syscall/ Might to consider checking using codespell to catch such things although it is imperfect.
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@google.com Cc: stable@vger.kernel.org
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.
Paul doesn't like C++-style comments so rewrite using kernel coding style for multi-line comments or drop. I don't think kernel coding style strictly prohibits use for single-line comments and it isn't detected by checkpatch.pl but he has previously raised this on other patches. I actually like the C++-style comments for one-liners especially for comments at the end of a line of code but Paul is the maintainer so he gets the final word.
switch (cmd) {
case FS_IOC32_GETFLAGS:
cmd = FS_IOC_GETFLAGS;
break;
case FS_IOC32_SETFLAGS:
cmd = FS_IOC_GETFLAGS;
Sorry, missed this the first time but cut-and-paste error above: s/GETFLAGS/SETFLAGS/
I didn't do an audit but does anything need to be updated for the BPF LSM or does it auto-magically pick up new hooks?
Also, IIRC, Paul prefers putting a pair of parentheses after function names to distinguish them, so in the subject line and description it should be security_file_ioctl_compat() and security_file_ioctl(), and you should put a patch version in the [PATCH] prefix e.g. [PATCH v3] to make clear that it is a later version, and usually one doesn't capitalize SELinux or the leading verb in the subject line (just "selinux: introduce").
Actually, since this spans more than just SELinux, the prefix likely needs to reflect that (e.g. security: introduce ...) and the patch should go to the linux-security-module mailing list too and perhaps linux-fsdevel for the ioctl change.