Re: [PATCH V2] SELinux: Check correct permissions for FS_IOC32_*

[Paul Moore]

On Mon, Sep 11, 2023 at 9:19 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
> On Fri, Sep 8, 2023 at 6:54 PM kernel test robot <lkp@xxxxxxxxx> wrote:
> >
> > Hi Alfred,
> >
> > kernel test robot noticed the following build errors:
> >
> > [auto build test ERROR on 50a510a78287c15cee644f345ef8bac8977986a7]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Alfred-Piccioni/SELinux-Check-correct-permissions-for-FS_IOC32_/20230906-200131
> > base:   50a510a78287c15cee644f345ef8bac8977986a7
> > patch link:    https://lore.kernel.org/r/20230906115928.3749928-1-alpic%40google.com
> > patch subject: [PATCH V2] SELinux: Check correct permissions for FS_IOC32_*
> > config: i386-debian-10.3 (https://download.01.org/0day-ci/archive/20230909/202309090600.NSyo7d2q-lkp@xxxxxxxxx/config)
> > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230909/202309090600.NSyo7d2q-lkp@xxxxxxxxx/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202309090600.NSyo7d2q-lkp@xxxxxxxxx/
> >
> > All errors (new ones prefixed by >>):
> >
> >    security/selinux/hooks.c: In function 'selinux_file_ioctl':
> > >> security/selinux/hooks.c:3647:9: error: duplicate case value
> >     3647 |         case FS_IOC32_GETFLAGS:
> >          |         ^~~~
> >    security/selinux/hooks.c:3645:9: note: previously used here
> >     3645 |         case FS_IOC_GETFLAGS:
> >          |         ^~~~
> >    security/selinux/hooks.c:3648:9: error: duplicate case value
> >     3648 |         case FS_IOC32_GETVERSION:
> >          |         ^~~~
> >    security/selinux/hooks.c:3646:9: note: previously used here
> >     3646 |         case FS_IOC_GETVERSION:
> >          |         ^~~~
> >    security/selinux/hooks.c:3654:9: error: duplicate case value
> >     3654 |         case FS_IOC32_SETFLAGS:
> >          |         ^~~~
> >    security/selinux/hooks.c:3652:9: note: previously used here
> >     3652 |         case FS_IOC_SETFLAGS:
> >          |         ^~~~
> >    security/selinux/hooks.c:3655:9: error: duplicate case value
> >     3655 |         case FS_IOC32_SETVERSION:
> >          |         ^~~~
> >    security/selinux/hooks.c:3653:9: note: previously used here
> >     3653 |         case FS_IOC_SETVERSION:
> >          |         ^~~~
>
> Not sure of the right way to fix this while addressing the original
> issue that this patch was intended to fix. Looking in fs/ioctl.c, I
> see that the some FS_IOC32 values are remapped to the corresponding
> FS_IOC values by the compat ioctl syscall entrypoint. Also notice this
> comment there:
>
>         /* RED-PEN how should LSM module know it's handling 32bit? */
>         error = security_file_ioctl(f.file, cmd, arg);
>         if (error)
>                 goto out;

What is both interesting and scary is that the "RED-PEN" comment seems
to go back at least as far as git, which is a lifetime these days.
This has been broken for a while.

> So perhaps this is a defect in LSM that needs to be addressed?

I think so.  I would suggest a new security_file_ioctl_compat() hook
as I often worry that flags are too easy to misuse whereas a separate
hook, especially one with "_compat" at the end, are a bit more clear.
The good news is that of the three LSMs that have file_ioctl hook
implementations it looks like only SELinux will need a dedicated
compat hook implementation (*maybe* TOMOYO, but I think it should be
okay reusing the existing tomoyo_file_ioctl() implementation).

-- 
paul-moore.com