[PATCH] SELinux: Introduce security_file_ioctl_compat hook

[PATCH] SELinux: Introduce security_file_ioctl_compat hook

[Alfred Piccioni]

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@google.com>
Cc: stable@vger.kernel.org
---
 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)
 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
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH] SELinux: Introduce security_file_ioctl_compat hook

[Alfred Piccioni]

> s/emmits/emits/

Fixed.

> 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.

Yup, reworded to be more correct. Partially lack of understanding on
my part of how the ioctl syscalls were being made.

> 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?

Correct. Forgot to go back and remove it. Fixed.

> 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.

I do like extra credit. I'll take a look and see if it's something I
can tackle. I'm primarily doing ad hoc checks on Android devices, so
I'm unsure how easy it will be for me to run the suite. I'll get back
to you shortly on that.

Re: [PATCH] SELinux: Introduce security_file_ioctl_compat hook

[Stephen Smalley]

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").

Re: [PATCH] SELinux: Introduce security_file_ioctl_compat hook

[Stephen Smalley]

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.

Re: [PATCH] SELinux: Introduce security_file_ioctl_compat hook

[Casey Schaufler]

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@google.com>
> Cc: stable@vger.kernel.org

This *really* needs to go the the LSM email list:
	linux-security-module@vger.kernel.org

> ---
>  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

Re: [PATCH] SELinux: Introduce security_file_ioctl_compat hook

[Stephen Smalley]

On Mon, Dec 18, 2023 at 12:11 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> 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@google.com>
> > Cc: stable@vger.kernel.org
>
> This *really* needs to go the the LSM email list:
>         linux-security-module@vger.kernel.org

Yep, pointed that out a little earlier in this thread.

> > ---
> > 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.

Paul told him the opposite earlier.

Re: [PATCH] SELinux: Introduce security_file_ioctl_compat hook

[Casey Schaufler]

On 12/18/2023 9:36 AM, Stephen Smalley wrote:
> On Mon, Dec 18, 2023 at 12:11 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> 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@google.com>
>>> Cc: stable@vger.kernel.org
>> This *really* needs to go the the LSM email list:
>>         linux-security-module@vger.kernel.org
> Yep, pointed that out a little earlier in this thread.
>
>>> ---
>>> 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.
> Paul told him the opposite earlier.

Sigh. Paul's wrong, but as he's the maintainer I shan't push further.

Re: [PATCH] SELinux: Introduce security_file_ioctl_compat hook

[Stephen Smalley]

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.

Re: [PATCH] SELinux: Introduce security_file_ioctl_compat hook

[Stephen Smalley]

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?

[PATCH] SELinux: Introduce security_file_ioctl_compat hook

[Alfred Piccioni]

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
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.

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
---
 fs/ioctl.c                    |  3 +--
 fs/overlayfs/inode.c          |  4 ++++
 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 +
 8 files changed, 59 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/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
 	fput(file);
 
 	return err;
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..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
-- 
2.43.0.472.g3155946c3a-goog