diff mbox series

[V2] SELinux: Check correct permissions for FS_IOC32_*

Message ID 20230906115928.3749928-1-alpic@google.com (mailing list archive)
State Changes Requested
Delegated to: Paul Moore
Headers show
Series [V2] SELinux: Check correct permissions for FS_IOC32_* | expand

Commit Message

Alfred Piccioni Sept. 6, 2023, 11:59 a.m. UTC 
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.

Two possible solutions exist:

- Trim parameter "cmd" to a u16 so that only the last two bytes are
  checked in the case statement.

- Explicitly add the FS_IOC32_* codes to the case statement.

Solution 2 was chosen because it is a minimal explicit change. Solution
1 is a more elegant change, but is less explicit, as the switch
statement appears to only check the FS_IOC_* codes upon first reading.

Fixes: 0b24dcb7f2f7 ("Revert "selinux: simplify ioctl checking"")
Signed-off-by: Alfred Piccioni <alpic@google.com>
Cc: stable@vger.kernel.org
---
V1->V2: Cleaned up some typos and added tag for -stable tree inclusion.

 security/selinux/hooks.c | 4 ++++
 1 file changed, 4 insertions(+)


base-commit: 50a510a78287c15cee644f345ef8bac8977986a7

Comments

Stephen Smalley Sept. 6, 2023, 5:49 p.m. UTC | #1 
On Wed, Sep 6, 2023 at 7:59 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.
>
> Two possible solutions exist:
>
> - Trim parameter "cmd" to a u16 so that only the last two bytes are
>   checked in the case statement.
>
> - Explicitly add the FS_IOC32_* codes to the case statement.
>
> Solution 2 was chosen because it is a minimal explicit change. Solution
> 1 is a more elegant change, but is less explicit, as the switch
> statement appears to only check the FS_IOC_* codes upon first reading.
>
> Fixes: 0b24dcb7f2f7 ("Revert "selinux: simplify ioctl checking"")
> Signed-off-by: Alfred Piccioni <alpic@google.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Stephen Smalley <stephen.smalley.work@gmail.com>

> ---
> V1->V2: Cleaned up some typos and added tag for -stable tree inclusion.
>
>  security/selinux/hooks.c | 4 ++++
>  1 file changed, 4 insertions(+)
kernel test robot Sept. 8, 2023, 10:54 p.m. UTC | #2 
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@intel.com/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@intel.com/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@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309090600.NSyo7d2q-lkp@intel.com/

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:
         |         ^~~~


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
Stephen Smalley Sept. 11, 2023, 1:19 p.m. UTC | #3 
On Fri, Sep 8, 2023 at 6:54 PM kernel test robot <lkp@intel.com> 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@intel.com/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@intel.com/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@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202309090600.NSyo7d2q-lkp@intel.com/
>
> 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
Stephen Smalley Sept. 11, 2023, 1:49 p.m. UTC | #4 
On Mon, Sep 11, 2023 at 9:19 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Sep 8, 2023 at 6:54 PM kernel test robot <lkp@intel.com> 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@intel.com/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@intel.com/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@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202309090600.NSyo7d2q-lkp@intel.com/
> >
> > 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
Alfred Piccioni Sept. 12, 2023, 9 a.m. UTC | #5 
On Mon, Sep 11, 2023 at 3:49 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Mon, Sep 11, 2023 at 9:19 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Fri, Sep 8, 2023 at 6:54 PM kernel test robot <lkp@intel.com> 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@intel.com/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@intel.com/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@intel.com>
> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202309090600.NSyo7d2q-lkp@intel.com/
> > >
> > > 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!
Stephen Smalley Sept. 12, 2023, noon UTC | #6 
On Tue, Sep 12, 2023 at 5:00 AM Alfred Piccioni <alpic@google.com> wrote:
>
> On Mon, Sep 11, 2023 at 3:49 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Mon, Sep 11, 2023 at 9:19 AM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Fri, Sep 8, 2023 at 6:54 PM kernel test robot <lkp@intel.com> 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@intel.com/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@intel.com/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@intel.com>
> > > > | Closes: https://lore.kernel.org/oe-kbuild-all/202309090600.NSyo7d2q-lkp@intel.com/
> > > >
> > > > 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!
Mickaël Salaün Sept. 12, 2023, 3:46 p.m. UTC | #7 
On Tue, Sep 12, 2023 at 08:00:12AM -0400, Stephen Smalley wrote:
> On Tue, Sep 12, 2023 at 5:00 AM Alfred Piccioni <alpic@google.com> wrote:
> >
> > On Mon, Sep 11, 2023 at 3:49 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Mon, Sep 11, 2023 at 9:19 AM Stephen Smalley
> > > <stephen.smalley.work@gmail.com> wrote:
> > > >
> > > > On Fri, Sep 8, 2023 at 6:54 PM kernel test robot <lkp@intel.com> 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@intel.com/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@intel.com/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@intel.com>
> > > > > | Closes: https://lore.kernel.org/oe-kbuild-all/202309090600.NSyo7d2q-lkp@intel.com/
> > > > >
> > > > > 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.

I think adding a boolean argument to the LSM hook makes sense. LSMs
might decide to handle it or not, at their own pace.

> 
> 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.
Paul Moore Sept. 13, 2023, 3:52 a.m. UTC | #8 
On Mon, Sep 11, 2023 at 9:19 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Fri, Sep 8, 2023 at 6:54 PM kernel test robot <lkp@intel.com> 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@intel.com/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@intel.com/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@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202309090600.NSyo7d2q-lkp@intel.com/
> >
> > 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).
diff mbox series

Patch

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d06e350fedee..bba83f437a1d 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3644,11 +3644,15 @@  static int selinux_file_ioctl(struct file *file, unsigned int cmd,
 	case FIGETBSZ:
 	case FS_IOC_GETFLAGS:
 	case FS_IOC_GETVERSION:
+	case FS_IOC32_GETFLAGS:
+	case FS_IOC32_GETVERSION:
 		error = file_has_perm(cred, file, FILE__GETATTR);
 		break;
 
 	case FS_IOC_SETFLAGS:
 	case FS_IOC_SETVERSION:
+	case FS_IOC32_SETFLAGS:
+	case FS_IOC32_SETVERSION:
 		error = file_has_perm(cred, file, FILE__SETATTR);
 		break;