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

[Stephen Smalley]

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;

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


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