Re: [PATCH] SELinux: Introduce security_file_ioctl_compat hook

From: Stephen Smalley <stephen.smalley.work@gmail.com>
To: Alfred Piccioni <alpic@google.com>
Cc: Paul Moore <paul@paul-moore.com>,
	Eric Paris <eparis@parisplace.org>,
	stable@vger.kernel.org,  SElinux list <selinux@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] SELinux: Introduce security_file_ioctl_compat hook
Date: Mon, 18 Dec 2023 11:03:02 -0500	[thread overview]
Message-ID: <CAEjxPJ7xVgp+4V-OnGRgdWTd6fV0dQr32S=8H41F2-J6Snygmw@mail.gmail.com> (raw)
In-Reply-To: <CAEjxPJ40jPsBS5xZEgS1CCVYORDJhwNOrAj5Oepa3rK=Y1BnYg@mail.gmail.com>

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.