On Mon, Sep 11, 2023 at 3:49 PM Stephen Smalley
<[email protected]> wrote:
>
> On Mon, Sep 11, 2023 at 9:19 AM Stephen Smalley
> <[email protected]> wrote:
> >
> > On Fri, Sep 8, 2023 at 6:54 PM kernel test robot <[email protected]> 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/[email protected]/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/[email protected]/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 <[email protected]>
> > > | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > >
> > > 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;
> >
> > So perhaps this is a defect in LSM that needs to be addressed?
>
> Note btw that some of the 32-bit ioctl commands are only handled in
> the fs-specific compat_ioctl routines, e.g. ext4_compat_ioctl()
> handles EXT4_IOC32_GETVERSION == FS_IOC32_GETVERSION and ditto for
> _SETVERSION.
>
> >
> >
> > >
> > >
> > > vim +3647 security/selinux/hooks.c
> > >
> > > 3634
> > > 3635 static int selinux_file_ioctl(struct file *file, unsigned int cmd,
> > > 3636 unsigned long arg)
> > > 3637 {
> > > 3638 const struct cred *cred = current_cred();
> > > 3639 int error = 0;
> > > 3640
> > > 3641 switch (cmd) {
> > > 3642 case FIONREAD:
> > > 3643 case FIBMAP:
> > > 3644 case FIGETBSZ:
> > > 3645 case FS_IOC_GETFLAGS:
> > > 3646 case FS_IOC_GETVERSION:
> > > > 3647 case FS_IOC32_GETFLAGS:
> > > 3648 case FS_IOC32_GETVERSION:
> > > 3649 error = file_has_perm(cred, file, FILE__GETATTR);
> > > 3650 break;
> > > 3651
> > > 3652 case FS_IOC_SETFLAGS:
> > > 3653 case FS_IOC_SETVERSION:
> > > 3654 case FS_IOC32_SETFLAGS:
> > > 3655 case FS_IOC32_SETVERSION:
> > > 3656 error = file_has_perm(cred, file, FILE__SETATTR);
> > > 3657 break;
> > > 3658
> > > 3659 /* sys_ioctl() checks */
> > > 3660 case FIONBIO:
> > > 3661 case FIOASYNC:
> > > 3662 error = file_has_perm(cred, file, 0);
> > > 3663 break;
> > > 3664
> > > 3665 case KDSKBENT:
> > > 3666 case KDSKBSENT:
> > > 3667 error = cred_has_capability(cred, CAP_SYS_TTY_CONFIG,
> > > 3668 CAP_OPT_NONE, true);
> > > 3669 break;
> > > 3670
> > > 3671 case FIOCLEX:
> > > 3672 case FIONCLEX:
> > > 3673 if (!selinux_policycap_ioctl_skip_cloexec())
> > > 3674 error = ioctl_has_perm(cred, file, FILE__IOCTL, (u16) cmd);
> > > 3675 break;
> > > 3676
> > > 3677 /* default case assumes that the command will go
> > > 3678 * to the file's ioctl() function.
> > > 3679 */
> > > 3680 default:
> > > 3681 error = ioctl_has_perm(cred, file, FILE__IOCTL, (u16) cmd);
> > > 3682 }
> > > 3683 return error;
> > > 3684 }
> > > 3685
Hey Stephen,
Thanks for looking into it a bit deeper! This seems a bit of a pickle.
I can think of a few somewhat hacky ways to fix this.
I can just set the flags to check `if FS_IOC32_*; set FS_IOC_*;`,
which is the quickest but kinda hacky.
I can go with the other plan of dropping the irrelevant bytes from the
cmd code, so all codes will be read as u16. This effectively does the
same thing, but may be unclear.
I can also look into whether this can be solved at the LSM or a higher
level. Perhaps the filesystems setting `if FS_IOC32_*; set FS_IOC_*;`
is a hint that something else interesting is going wrong.
I'll spend a little time thinking and investigating and get back with
a more concrete solution. I'll also need to do a bit more robust
testing; it built on my machine!
Thanks!
On Tue, Sep 12, 2023 at 5:00 AM Alfred Piccioni <[email protected]> wrote:
>
> On Mon, Sep 11, 2023 at 3:49 PM Stephen Smalley
> <[email protected]> wrote:
> >
> > On Mon, Sep 11, 2023 at 9:19 AM Stephen Smalley
> > <[email protected]> wrote:
> > >
> > > On Fri, Sep 8, 2023 at 6:54 PM kernel test robot <[email protected]> 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/[email protected]/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/[email protected]/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 <[email protected]>
> > > > | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > > >
> > > > 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;
> > >
> > > So perhaps this is a defect in LSM that needs to be addressed?
> >
> > Note btw that some of the 32-bit ioctl commands are only handled in
> > the fs-specific compat_ioctl routines, e.g. ext4_compat_ioctl()
> > handles EXT4_IOC32_GETVERSION == FS_IOC32_GETVERSION and ditto for
> > _SETVERSION.
> >
> > >
> > >
> > > >
> > > >
> > > > vim +3647 security/selinux/hooks.c
> > > >
> > > > 3634
> > > > 3635 static int selinux_file_ioctl(struct file *file, unsigned int cmd,
> > > > 3636 unsigned long arg)
> > > > 3637 {
> > > > 3638 const struct cred *cred = current_cred();
> > > > 3639 int error = 0;
> > > > 3640
> > > > 3641 switch (cmd) {
> > > > 3642 case FIONREAD:
> > > > 3643 case FIBMAP:
> > > > 3644 case FIGETBSZ:
> > > > 3645 case FS_IOC_GETFLAGS:
> > > > 3646 case FS_IOC_GETVERSION:
> > > > > 3647 case FS_IOC32_GETFLAGS:
> > > > 3648 case FS_IOC32_GETVERSION:
> > > > 3649 error = file_has_perm(cred, file, FILE__GETATTR);
> > > > 3650 break;
> > > > 3651
> > > > 3652 case FS_IOC_SETFLAGS:
> > > > 3653 case FS_IOC_SETVERSION:
> > > > 3654 case FS_IOC32_SETFLAGS:
> > > > 3655 case FS_IOC32_SETVERSION:
> > > > 3656 error = file_has_perm(cred, file, FILE__SETATTR);
> > > > 3657 break;
> > > > 3658
> > > > 3659 /* sys_ioctl() checks */
> > > > 3660 case FIONBIO:
> > > > 3661 case FIOASYNC:
> > > > 3662 error = file_has_perm(cred, file, 0);
> > > > 3663 break;
> > > > 3664
> > > > 3665 case KDSKBENT:
> > > > 3666 case KDSKBSENT:
> > > > 3667 error = cred_has_capability(cred, CAP_SYS_TTY_CONFIG,
> > > > 3668 CAP_OPT_NONE, true);
> > > > 3669 break;
> > > > 3670
> > > > 3671 case FIOCLEX:
> > > > 3672 case FIONCLEX:
> > > > 3673 if (!selinux_policycap_ioctl_skip_cloexec())
> > > > 3674 error = ioctl_has_perm(cred, file, FILE__IOCTL, (u16) cmd);
> > > > 3675 break;
> > > > 3676
> > > > 3677 /* default case assumes that the command will go
> > > > 3678 * to the file's ioctl() function.
> > > > 3679 */
> > > > 3680 default:
> > > > 3681 error = ioctl_has_perm(cred, file, FILE__IOCTL, (u16) cmd);
> > > > 3682 }
> > > > 3683 return error;
> > > > 3684 }
> > > > 3685
>
> Hey Stephen,
>
> Thanks for looking into it a bit deeper! This seems a bit of a pickle.
> I can think of a few somewhat hacky ways to fix this.
>
> I can just set the flags to check `if FS_IOC32_*; set FS_IOC_*;`,
> which is the quickest but kinda hacky.
>
> I can go with the other plan of dropping the irrelevant bytes from the
> cmd code, so all codes will be read as u16. This effectively does the
> same thing, but may be unclear.
>
> I can also look into whether this can be solved at the LSM or a higher
> level. Perhaps the filesystems setting `if FS_IOC32_*; set FS_IOC_*;`
> is a hint that something else interesting is going wrong.
>
> I'll spend a little time thinking and investigating and get back with
> a more concrete solution. I'll also need to do a bit more robust
> testing; it built on my machine!
Likewise for me; I don't generally try building for 32-bit systems.
Remapping FS_IOC32_* to FS_IOC_* in selinux_file_ioctl() seems
reasonable to me although optimally that would be conditional on
whether selinux_file_ioctl() is being called from the compat ioctl
syscall (e.g. adding a flag to the LSM hook to indicate this or using
a separate hook for it). Otherwise we might misinterpret some other
ioctl on 64-bit.
If we didn't have compatibility requirements, it would be tempting to
just get rid of all the special case ioctl command handling in
selinux_file_ioctl() and let ioctl_has_perm() handle them all with the
extended ioctl permissions support. But that would require a SELinux
policy cap to switch it on conditionally for compatibility at least
and not sure anyone is willing to refactor their policies accordingly.
>
> Thanks!