[PATCH] SELinux: Check correct permissions for FS_IOC32_*

[PATCH] SELinux: Check correct permissions for FS_IOC32_*

[Alfred Piccioni]

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 emmits
32-bit flags (FS_IOC32_*) for certain ioctl operations. These flags are
being checked erroneoulsy, 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.

- Explicitily 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>
---
 security/selinux/hooks.c | 4 ++++
 1 file changed, 4 insertions(+)

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;
 

base-commit: 50a510a78287c15cee644f345ef8bac8977986a7
-- 
2.42.0.283.g2d96d420d3-goog

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

[kernel test robot]

Hi,

Thanks for your patch.

FYI: kernel test robot notices the stable kernel rule is not satisfied.

The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html/#option-1

Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree.
Subject: [PATCH] SELinux: Check correct permissions for FS_IOC32_*
Link: https://lore.kernel.org/stable/20230906102557.3432236-1-alpic%40google.com

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Alfred Piccioni]

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(+)

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;
 

base-commit: 50a510a78287c15cee644f345ef8bac8977986a7
-- 
2.42.0.283.g2d96d420d3-goog

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

[Stephen Smalley]

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(+)

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

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

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

[Stephen Smalley]

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

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

[Alfred Piccioni]

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!

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

[Stephen Smalley]

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!

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

[Mickaël Salaün]

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.

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@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).

-- 
paul-moore.com

[PATCH] SELinux: Introduce security_file_ioctl_compat hook

[Alfred Piccioni]

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 emmits
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
replaces security_file_ioctl if the CONFIG_COMPAT flag is on. All
current LSMs have been changed to hook into the compat flag.

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
---
 fs/ioctl.c                    |  3 +--
 fs/overlayfs/inode.c          |  4 ++++
 include/linux/lsm_hook_defs.h |  2 ++
 include/linux/security.h      |  7 +++++++
 security/security.c           | 17 +++++++++++++++++
 security/selinux/hooks.c      | 26 ++++++++++++++++++++++++++
 security/smack/smack_lsm.c    |  1 +
 security/tomoyo/tomoyo.c      |  1 +
 8 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index f5fd99d6b0d4..76cf22ac97d7 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -920,8 +920,7 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd,
 	if (!f.file)
 		return -EBADF;
 
-	/* RED-PEN how should LSM module know it's handling 32bit? */
-	error = security_file_ioctl(f.file, cmd, arg);
+	error = security_file_ioctl_compat(f.file, cmd, arg);
 	if (error)
 		goto out;
 
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 83ef66644c21..170687b5985b 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -751,7 +751,11 @@ static int ovl_security_fileattr(const struct path *realpath, struct fileattr *f
 	else
 		cmd = fa->fsx_valid ? FS_IOC_FSGETXATTR : FS_IOC_GETFLAGS;
 
+#ifdef CONFIG_COMPAT
+	err = security_file_ioctl_compat(file, cmd, 0);
+# else
 	err = security_file_ioctl(file, cmd, 0);
+#endif
 	fput(file);
 
 	return err;
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index ac962c4cb44b..626aa8cf930d 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -171,6 +171,8 @@ LSM_HOOK(int, 0, file_alloc_security, struct file *file)
 LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
 LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
 	 unsigned long arg)
+LSM_HOOK(int, 0, file_ioctl_compat, struct file *file, unsigned int cmd,
+	 unsigned long arg)
 LSM_HOOK(int, 0, mmap_addr, unsigned long addr)
 LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot,
 	 unsigned long prot, unsigned long flags)
diff --git a/include/linux/security.h b/include/linux/security.h
index 5f16eecde00b..22a82b7c59f1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -389,6 +389,7 @@ int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
 int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
+int security_file_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg);
 int security_mmap_file(struct file *file, unsigned long prot,
 			unsigned long flags);
 int security_mmap_addr(unsigned long addr);
@@ -987,6 +988,12 @@ static inline int security_file_ioctl(struct file *file, unsigned int cmd,
 	return 0;
 }
 
+static inline int security_file_ioctl_compat(struct file *file, unsigned int cmd,
+				      unsigned long arg)
+{
+	return 0;
+}
+
 static inline int security_mmap_file(struct file *file, unsigned long prot,
 				     unsigned long flags)
 {
diff --git a/security/security.c b/security/security.c
index 23b129d482a7..5c16ffc99b1e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2648,6 +2648,23 @@ int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 }
 EXPORT_SYMBOL_GPL(security_file_ioctl);
 
+/**
+ * security_file_ioctl_compat() - Check if an ioctl is allowed in 32-bit compat mode
+ * @file: associated file
+ * @cmd: ioctl cmd
+ * @arg: ioctl arguments
+ *
+ * Compat version of security_file_ioctl() that correctly handles 32-bit processes
+ * running on 64-bit kernels.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_file_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	return call_int_hook(file_ioctl_compat, 0, file, cmd, arg);
+}
+EXPORT_SYMBOL_GPL(security_file_ioctl_compat);
+
 static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
 {
 	/*
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.
+	switch (cmd) {
+	case FS_IOC32_GETFLAGS:
+		cmd = FS_IOC_GETFLAGS;
+		break;
+	case FS_IOC32_SETFLAGS:
+		cmd = FS_IOC_GETFLAGS;
+		break;
+	case FS_IOC32_GETVERSION:
+		cmd = FS_IOC_GETVERSION;
+		break;
+	case FS_IOC32_SETVERSION:
+		cmd = FS_IOC_SETVERSION;
+		break;
+	default:
+		break;
+	}
+
+	return selinux_file_ioctl(file, cmd, arg);
+}
+
 static int default_noexec __ro_after_init;
 
 static int file_map_prot_check(struct file *file, unsigned long prot, int shared)
@@ -7036,6 +7061,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(file_permission, selinux_file_permission),
 	LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
 	LSM_HOOK_INIT(file_ioctl, selinux_file_ioctl),
+	LSM_HOOK_INIT(file_ioctl_compat, selinux_file_ioctl_compat),
 	LSM_HOOK_INIT(mmap_file, selinux_mmap_file),
 	LSM_HOOK_INIT(mmap_addr, selinux_mmap_addr),
 	LSM_HOOK_INIT(file_mprotect, selinux_file_mprotect),
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 65130a791f57..1f1ea8529421 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4973,6 +4973,7 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
 
 	LSM_HOOK_INIT(file_alloc_security, smack_file_alloc_security),
 	LSM_HOOK_INIT(file_ioctl, smack_file_ioctl),
+	LSM_HOOK_INIT(file_ioctl_compat, smack_file_ioctl),
 	LSM_HOOK_INIT(file_lock, smack_file_lock),
 	LSM_HOOK_INIT(file_fcntl, smack_file_fcntl),
 	LSM_HOOK_INIT(mmap_file, smack_mmap_file),
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 25006fddc964..298d182759c2 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -568,6 +568,7 @@ static struct security_hook_list tomoyo_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(path_rename, tomoyo_path_rename),
 	LSM_HOOK_INIT(inode_getattr, tomoyo_inode_getattr),
 	LSM_HOOK_INIT(file_ioctl, tomoyo_file_ioctl),
+	LSM_HOOK_INIT(file_ioctl_compat, tomoyo_file_ioctl),
 	LSM_HOOK_INIT(path_chmod, tomoyo_path_chmod),
 	LSM_HOOK_INIT(path_chown, tomoyo_path_chown),
 	LSM_HOOK_INIT(path_chroot, tomoyo_path_chroot),

base-commit: 196e95aa8305aecafc4e1857b7d3eff200d953b6
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH] SELinux: Introduce security_file_ioctl_compat hook

[Stephen Smalley]

On Mon, Dec 18, 2023 at 7:43 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 emmits

s/emmits/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
> replaces security_file_ioctl if the CONFIG_COMPAT flag is on. All
> current LSMs have been changed to hook into the compat flag.

It doesn't (or shouldn't) replace security_file_ioctl, and the hook
doesn't appear to be conditional on CONFIG_COMPAT per se.
It is a new hook that is called from the compat ioctl syscall. The old
hook continues to be used from the regular ioctl syscall and
elsewhere.

> 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/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 83ef66644c21..170687b5985b 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -751,7 +751,11 @@ static int ovl_security_fileattr(const struct path *realpath, struct fileattr *f
>         else
>                 cmd = fa->fsx_valid ? FS_IOC_FSGETXATTR : FS_IOC_GETFLAGS;
>
> +#ifdef CONFIG_COMPAT
> +       err = security_file_ioctl_compat(file, cmd, 0);
> +# else
>         err = security_file_ioctl(file, cmd, 0);
> +#endif

I don't understand why you made this change, possibly a leftover of an
earlier version of the patch that tried to replace
security_file_ioctl() everywhere?

Re: [PATCH] SELinux: Introduce security_file_ioctl_compat hook

[Stephen Smalley]

On Mon, Dec 18, 2023 at 8:46 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Mon, Dec 18, 2023 at 7:43 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 emmits
>
> s/emmits/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
> > replaces security_file_ioctl if the CONFIG_COMPAT flag is on. All
> > current LSMs have been changed to hook into the compat flag.
>
> It doesn't (or shouldn't) replace security_file_ioctl, and the hook
> doesn't appear to be conditional on CONFIG_COMPAT per se.
> It is a new hook that is called from the compat ioctl syscall. The old
> hook continues to be used from the regular ioctl syscall and
> elsewhere.
>
> > 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/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index 83ef66644c21..170687b5985b 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -751,7 +751,11 @@ static int ovl_security_fileattr(const struct path *realpath, struct fileattr *f
> >         else
> >                 cmd = fa->fsx_valid ? FS_IOC_FSGETXATTR : FS_IOC_GETFLAGS;
> >
> > +#ifdef CONFIG_COMPAT
> > +       err = security_file_ioctl_compat(file, cmd, 0);
> > +# else
> >         err = security_file_ioctl(file, cmd, 0);
> > +#endif
>
> I don't understand why you made this change, possibly a leftover of an
> earlier version of the patch that tried to replace
> security_file_ioctl() everywhere?

By the way, for extra credit, you could augment the ioctl tests in the
selinux-testsuite to also exercise this new hook and confirm that it
works correctly. See
https://github.com/SELinuxProject/selinux-testsuite particularly
tests/ioctl and policy/test_ioctl.te. Feel free to ask for help on
that.

[PATCH] security: new security_file_ioctl_compat() hook

[Alfred Piccioni]

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 syscall. All current LSMs have been changed
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
---
 fs/ioctl.c                    |  3 +--
 include/linux/lsm_hook_defs.h |  2 ++
 include/linux/security.h      |  7 +++++++
 security/security.c           | 17 +++++++++++++++++
 security/selinux/hooks.c      | 28 ++++++++++++++++++++++++++++
 security/smack/smack_lsm.c    |  1 +
 security/tomoyo/tomoyo.c      |  1 +
 7 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index f5fd99d6b0d4..76cf22ac97d7 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -920,8 +920,7 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd,
 	if (!f.file)
 		return -EBADF;
 
-	/* RED-PEN how should LSM module know it's handling 32bit? */
-	error = security_file_ioctl(f.file, cmd, arg);
+	error = security_file_ioctl_compat(f.file, cmd, arg);
 	if (error)
 		goto out;
 
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index ac962c4cb44b..626aa8cf930d 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -171,6 +171,8 @@ LSM_HOOK(int, 0, file_alloc_security, struct file *file)
 LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
 LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
 	 unsigned long arg)
+LSM_HOOK(int, 0, file_ioctl_compat, struct file *file, unsigned int cmd,
+	 unsigned long arg)
 LSM_HOOK(int, 0, mmap_addr, unsigned long addr)
 LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot,
 	 unsigned long prot, unsigned long flags)
diff --git a/include/linux/security.h b/include/linux/security.h
index 5f16eecde00b..22a82b7c59f1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -389,6 +389,7 @@ int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
 int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
+int security_file_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg);
 int security_mmap_file(struct file *file, unsigned long prot,
 			unsigned long flags);
 int security_mmap_addr(unsigned long addr);
@@ -987,6 +988,12 @@ static inline int security_file_ioctl(struct file *file, unsigned int cmd,
 	return 0;
 }
 
+static inline int security_file_ioctl_compat(struct file *file, unsigned int cmd,
+				      unsigned long arg)
+{
+	return 0;
+}
+
 static inline int security_mmap_file(struct file *file, unsigned long prot,
 				     unsigned long flags)
 {
diff --git a/security/security.c b/security/security.c
index 23b129d482a7..5c16ffc99b1e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2648,6 +2648,23 @@ int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 }
 EXPORT_SYMBOL_GPL(security_file_ioctl);
 
+/**
+ * security_file_ioctl_compat() - Check if an ioctl is allowed in 32-bit compat mode
+ * @file: associated file
+ * @cmd: ioctl cmd
+ * @arg: ioctl arguments
+ *
+ * Compat version of security_file_ioctl() that correctly handles 32-bit processes
+ * running on 64-bit kernels.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_file_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	return call_int_hook(file_ioctl_compat, 0, file, cmd, arg);
+}
+EXPORT_SYMBOL_GPL(security_file_ioctl_compat);
+
 static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
 {
 	/*
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2aa0e219d721..c617ae21dba8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3731,6 +3731,33 @@ 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.
+	 */
+	switch (cmd) {
+	case FS_IOC32_GETFLAGS:
+		cmd = FS_IOC_GETFLAGS;
+		break;
+	case FS_IOC32_SETFLAGS:
+		cmd = FS_IOC_SETFLAGS;
+		break;
+	case FS_IOC32_GETVERSION:
+		cmd = FS_IOC_GETVERSION;
+		break;
+	case FS_IOC32_SETVERSION:
+		cmd = FS_IOC_SETVERSION;
+		break;
+	default:
+		break;
+	}
+
+	return selinux_file_ioctl(file, cmd, arg);
+}
+
 static int default_noexec __ro_after_init;
 
 static int file_map_prot_check(struct file *file, unsigned long prot, int shared)
@@ -7036,6 +7063,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(file_permission, selinux_file_permission),
 	LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
 	LSM_HOOK_INIT(file_ioctl, selinux_file_ioctl),
+	LSM_HOOK_INIT(file_ioctl_compat, selinux_file_ioctl_compat),
 	LSM_HOOK_INIT(mmap_file, selinux_mmap_file),
 	LSM_HOOK_INIT(mmap_addr, selinux_mmap_addr),
 	LSM_HOOK_INIT(file_mprotect, selinux_file_mprotect),
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 65130a791f57..1f1ea8529421 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4973,6 +4973,7 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
 
 	LSM_HOOK_INIT(file_alloc_security, smack_file_alloc_security),
 	LSM_HOOK_INIT(file_ioctl, smack_file_ioctl),
+	LSM_HOOK_INIT(file_ioctl_compat, smack_file_ioctl),
 	LSM_HOOK_INIT(file_lock, smack_file_lock),
 	LSM_HOOK_INIT(file_fcntl, smack_file_fcntl),
 	LSM_HOOK_INIT(mmap_file, smack_mmap_file),
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 25006fddc964..298d182759c2 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -568,6 +568,7 @@ static struct security_hook_list tomoyo_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(path_rename, tomoyo_path_rename),
 	LSM_HOOK_INIT(inode_getattr, tomoyo_inode_getattr),
 	LSM_HOOK_INIT(file_ioctl, tomoyo_file_ioctl),
+	LSM_HOOK_INIT(file_ioctl_compat, tomoyo_file_ioctl),
 	LSM_HOOK_INIT(path_chmod, tomoyo_path_chmod),
 	LSM_HOOK_INIT(path_chown, tomoyo_path_chown),
 	LSM_HOOK_INIT(path_chroot, tomoyo_path_chroot),

base-commit: 196e95aa8305aecafc4e1857b7d3eff200d953b6
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Alfred Piccioni]

Thanks for taking the time to review! Apologies for the number of
small mistakes.

> s/syscal/syscall/
> Might to consider checking using codespell to catch such things
> although it is imperfect.

Fixed, loaded codespell.

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

Changed to /**/ style comments. No particular preference on my side
for comment structure, just used to C++/Java style.

> Sorry, missed this the first time but cut-and-paste error above:
> s/GETFLAGS/SETFLAGS/

Egads. Fixed.

> 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").

Changed title to lower-case, prefixed with security, changed slightly
to fit in summary with new parentheses. Added [PATCH V3] to the
subject.

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

Added cc 'selinux@vger.kernel.org' and cc
'linux-kernel@vger.kernel.org'. Thanks!

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

I'm unsure. I looked through the BPF LSM and I can't see any way it's
picking up the file_ioctl hook to begin with. It appears to me
skimming through the code that it automagically picks it up, but I'm
not willing to bet the kernel on it.

Do you know who would be a good person to ask about this to make sure?

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Alfred Piccioni]

>> By the way, for extra credit, you could augment the ioctl tests in the
>> selinux-testsuite to also exercise this new hook and confirm that it
>> works correctly. See
>> https://github.com/SELinuxProject/selinux-testsuite particularly
>> tests/ioctl and policy/test_ioctl.te. Feel free to ask for help on
>> that.

> I do like extra credit. I'll take a look and see if it's something I
> can tackle. I'm primarily doing ad hoc checks on Android devices, so
> I'm unsure how easy it will be for me to run the suite. I'll get back
> to you shortly on that.

In response to myself, I unfortunately won't have time to do the
testing updates this year. If someone else wants to help, that'd be
great! Otherwise, I'll take a look next year after vacation and see if
I can take a crack at it. Thanks!

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Stephen Smalley]

On Tue, Dec 19, 2023 at 4:11 AM Alfred Piccioni <alpic@google.com> wrote:
>
> Thanks for taking the time to review! Apologies for the number of
> small mistakes.

NP.

> > 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").
>
> Changed title to lower-case, prefixed with security, changed slightly
> to fit in summary with new parentheses. Added [PATCH V3] to the
> subject.

Patch description still doesn't include the parentheses after each
function name but probably not worth re-spinning unless Paul says to
do so. I don't see the v3 in the subject line. Seemingly that in
combination with the fact that you replied to the original thread
confuses the b4 tool (b4.docs.kernel.org) such that b4 mbox/am/shazam
ends up selecting the v2 patch instead by default.

> > 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.
>
> Added cc 'selinux@vger.kernel.org' and cc
> 'linux-kernel@vger.kernel.org'. Thanks!

Just FYI, scripts/get_maintainer.pl /path/to/patch will provide an
over-approximation of who to include on the distribution for patches
based on MAINTAINERS and recent committers. That said, I generally
prune the set it provides. More art than science.

> > 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?
>
> I'm unsure. I looked through the BPF LSM and I can't see any way it's
> picking up the file_ioctl hook to begin with. It appears to me
> skimming through the code that it automagically picks it up, but I'm
> not willing to bet the kernel on it.
>
> Do you know who would be a good person to ask about this to make sure?

Looks like it inherited it via the lsm_hook_defs.h.
$ nm security/bpf/hooks.o | grep ioctl
                 U bpf_lsm_file_ioctl
                 U bpf_lsm_file_ioctl_compat

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Stephen Smalley]

On Tue, Dec 19, 2023 at 4:09 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 syscall. All current LSMs have been changed
> 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
> ---

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

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Eric Biggers]

On Tue, Dec 19, 2023 at 10:09:09AM +0100, Alfred Piccioni wrote:
> 
> base-commit: 196e95aa8305aecafc4e1857b7d3eff200d953b6
> -- 

Where can I get this base commit?

- Eric

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Paul Moore]

On Tue, Dec 19, 2023 at 4:09 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 syscall. All current LSMs have been changed
> 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
> ---
>  fs/ioctl.c                    |  3 +--
>  include/linux/lsm_hook_defs.h |  2 ++
>  include/linux/security.h      |  7 +++++++
>  security/security.c           | 17 +++++++++++++++++
>  security/selinux/hooks.c      | 28 ++++++++++++++++++++++++++++
>  security/smack/smack_lsm.c    |  1 +
>  security/tomoyo/tomoyo.c      |  1 +
>  7 files changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index f5fd99d6b0d4..76cf22ac97d7 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -920,8 +920,7 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd,
>         if (!f.file)
>                 return -EBADF;
>
> -       /* RED-PEN how should LSM module know it's handling 32bit? */
> -       error = security_file_ioctl(f.file, cmd, arg);
> +       error = security_file_ioctl_compat(f.file, cmd, arg);
>         if (error)
>                 goto out;

This is interesting ... if you look at the normal ioctl() syscall
definition in the kernel you see 'ioctl(unsigned int fd, unsigned int
cmd, unsigned long arg)' and if you look at the compat definition you
see 'ioctl(unsigned int fd, unsigned int cmd, compat_ulong_t arg)'.  I
was expecting the second parameter, @cmd, to be a long type in the
normal definition, but it is an int type in both cases.  It looks like
it has been that way long enough that it is correct, but I'm a little
lost ...

> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index ac962c4cb44b..626aa8cf930d 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -171,6 +171,8 @@ LSM_HOOK(int, 0, file_alloc_security, struct file *file)
>  LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
>  LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
>          unsigned long arg)
> +LSM_HOOK(int, 0, file_ioctl_compat, struct file *file, unsigned int cmd,
> +        unsigned long arg)
>  LSM_HOOK(int, 0, mmap_addr, unsigned long addr)
>  LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot,
>          unsigned long prot, unsigned long flags)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 5f16eecde00b..22a82b7c59f1 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -389,6 +389,7 @@ int security_file_permission(struct file *file, int mask);
>  int security_file_alloc(struct file *file);
>  void security_file_free(struct file *file);
>  int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
> +int security_file_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg);
>  int security_mmap_file(struct file *file, unsigned long prot,
>                         unsigned long flags);
>  int security_mmap_addr(unsigned long addr);
> @@ -987,6 +988,12 @@ static inline int security_file_ioctl(struct file *file, unsigned int cmd,
>         return 0;
>  }
>
> +static inline int security_file_ioctl_compat(struct file *file, unsigned int cmd,
> +                                     unsigned long arg)
> +{
> +       return 0;
> +}
> +
>  static inline int security_mmap_file(struct file *file, unsigned long prot,
>                                      unsigned long flags)
>  {
> diff --git a/security/security.c b/security/security.c
> index 23b129d482a7..5c16ffc99b1e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2648,6 +2648,23 @@ int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  }
>  EXPORT_SYMBOL_GPL(security_file_ioctl);
>
> +/**
> + * security_file_ioctl_compat() - Check if an ioctl is allowed in 32-bit compat mode
> + * @file: associated file
> + * @cmd: ioctl cmd
> + * @arg: ioctl arguments
> + *
> + * Compat version of security_file_ioctl() that correctly handles 32-bit processes
> + * running on 64-bit kernels.
> + *
> + * Return: Returns 0 if permission is granted.
> + */
> +int security_file_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +       return call_int_hook(file_ioctl_compat, 0, file, cmd, arg);
> +}
> +EXPORT_SYMBOL_GPL(security_file_ioctl_compat);
> +
>  static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
>  {
>         /*
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 2aa0e219d721..c617ae21dba8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3731,6 +3731,33 @@ 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.
> +        */
> +       switch (cmd) {
> +       case FS_IOC32_GETFLAGS:
> +               cmd = FS_IOC_GETFLAGS;
> +               break;
> +       case FS_IOC32_SETFLAGS:
> +               cmd = FS_IOC_SETFLAGS;
> +               break;
> +       case FS_IOC32_GETVERSION:
> +               cmd = FS_IOC_GETVERSION;
> +               break;
> +       case FS_IOC32_SETVERSION:
> +               cmd = FS_IOC_SETVERSION;
> +               break;
> +       default:
> +               break;
> +       }
> +
> +       return selinux_file_ioctl(file, cmd, arg);
> +}

Is it considered valid for a native 64-bit task to use 32-bit
FS_IO32_XXX flags?  If not, do we want to remove the FS_IO32_XXX flag
checks in selinux_file_ioctl()?

>  static int default_noexec __ro_after_init;
>
>  static int file_map_prot_check(struct file *file, unsigned long prot, int shared)
> @@ -7036,6 +7063,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
>         LSM_HOOK_INIT(file_permission, selinux_file_permission),
>         LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
>         LSM_HOOK_INIT(file_ioctl, selinux_file_ioctl),
> +       LSM_HOOK_INIT(file_ioctl_compat, selinux_file_ioctl_compat),
>         LSM_HOOK_INIT(mmap_file, selinux_mmap_file),
>         LSM_HOOK_INIT(mmap_addr, selinux_mmap_addr),
>         LSM_HOOK_INIT(file_mprotect, selinux_file_mprotect),
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 65130a791f57..1f1ea8529421 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4973,6 +4973,7 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
>
>         LSM_HOOK_INIT(file_alloc_security, smack_file_alloc_security),
>         LSM_HOOK_INIT(file_ioctl, smack_file_ioctl),
> +       LSM_HOOK_INIT(file_ioctl_compat, smack_file_ioctl),
>         LSM_HOOK_INIT(file_lock, smack_file_lock),
>         LSM_HOOK_INIT(file_fcntl, smack_file_fcntl),
>         LSM_HOOK_INIT(mmap_file, smack_mmap_file),
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 25006fddc964..298d182759c2 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -568,6 +568,7 @@ static struct security_hook_list tomoyo_hooks[] __ro_after_init = {
>         LSM_HOOK_INIT(path_rename, tomoyo_path_rename),
>         LSM_HOOK_INIT(inode_getattr, tomoyo_inode_getattr),
>         LSM_HOOK_INIT(file_ioctl, tomoyo_file_ioctl),
> +       LSM_HOOK_INIT(file_ioctl_compat, tomoyo_file_ioctl),
>         LSM_HOOK_INIT(path_chmod, tomoyo_path_chmod),
>         LSM_HOOK_INIT(path_chown, tomoyo_path_chown),
>         LSM_HOOK_INIT(path_chroot, tomoyo_path_chroot),

I agree that it looks like Smack and TOMOYO should be fine, but I
would like to hear from Casey and Tetsuo to confirm.

-- 
paul-moore.com

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Tetsuo Handa]

On 2023/12/23 10:23, Paul Moore wrote:
>> -       /* RED-PEN how should LSM module know it's handling 32bit? */
>> -       error = security_file_ioctl(f.file, cmd, arg);
>> +       error = security_file_ioctl_compat(f.file, cmd, arg);
>>         if (error)
>>                 goto out;
> 
> This is interesting ... if you look at the normal ioctl() syscall
> definition in the kernel you see 'ioctl(unsigned int fd, unsigned int
> cmd, unsigned long arg)' and if you look at the compat definition you
> see 'ioctl(unsigned int fd, unsigned int cmd, compat_ulong_t arg)'.  I
> was expecting the second parameter, @cmd, to be a long type in the
> normal definition, but it is an int type in both cases.  It looks like
> it has been that way long enough that it is correct, but I'm a little
> lost ...

Since @arg might be a pointer to some struct, @arg needs to use a long type.
But @cmd can remain 32bits for both 32bits/64bits kernels because @cmd is not
a pointer, can't it?

> I agree that it looks like Smack and TOMOYO should be fine, but I
> would like to hear from Casey and Tetsuo to confirm.

Fine for TOMOYO part, for TOMOYO treats @cmd as an integer.

Fwd: [PATCH] security: new security_file_ioctl_compat() hook

[Tetsuo Handa]

Does anything need to be updated for the BPF LSM or
does it auto-magically pick up new hooks?

-------- Forwarded Message --------
References: <20230906102557.3432236-1-alpic@google.com> <20231219090909.2827497-1-alpic@google.com>
In-Reply-To: <20231219090909.2827497-1-alpic@google.com>
From: Alfred Piccioni <alpic@google.com>
Date: Tue, 19 Dec 2023 10:10:38 +0100
Message-ID: <CALcwBGC9LzzdJeq3SWy9F3g5A32s5uSvJZae4j+rwNQqqLHCKg@mail.gmail.com>
Subject: Re: [PATCH] security: new security_file_ioctl_compat() hook
To: Paul Moore <paul@paul-moore.com>, Stephen Smalley <stephen.smalley.work@gmail.com>, Eric Paris <eparis@parisplace.org>
Cc: linux-security-module@vger.kernel.org, linux-fsdevel@vger.kernel.org, stable@vger.kernel.org, selinux@vger.kernel.org, linux-kernel@vger.kernel.org

Thanks for taking the time to review! Apologies for the number of
small mistakes.

> s/syscal/syscall/
> Might to consider checking using codespell to catch such things
> although it is imperfect.

Fixed, loaded codespell.

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

Changed to /**/ style comments. No particular preference on my side
for comment structure, just used to C++/Java style.

> Sorry, missed this the first time but cut-and-paste error above:
> s/GETFLAGS/SETFLAGS/

Egads. Fixed.

> 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").

Changed title to lower-case, prefixed with security, changed slightly
to fit in summary with new parentheses. Added [PATCH V3] to the
subject.

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

Added cc 'selinux@vger.kernel.org' and cc
'linux-kernel@vger.kernel.org'. Thanks!

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

I'm unsure. I looked through the BPF LSM and I can't see any way it's
picking up the file_ioctl hook to begin with. It appears to me
skimming through the code that it automagically picks it up, but I'm
not willing to bet the kernel on it.

Do you know who would be a good person to ask about this to make sure?

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Tetsuo Handa]

Adding BPF.

On 2023/12/19 18:10, Alfred Piccioni wrote:
>> 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?
> 
> I'm unsure. I looked through the BPF LSM and I can't see any way it's
> picking up the file_ioctl hook to begin with. It appears to me
> skimming through the code that it automagically picks it up, but I'm
> not willing to bet the kernel on it.

If BPF LSM silently picks up security_file_ioctl_compat() hook, I worry
that some existing BPF programs which check ioctl() using BPF LSM fail to
understand that such BPF programs need to be updated.

We basically don't care about out-of-tree kernel code. But does that rule
apply to BPF programs? Since BPF programs are out-of-tree, are BPF programs
which depend on BPF LSM considered as "we don't care about" rule?
Or is breakage of existing BPF programs considered as a regression?
(Note that this patch is CC:ed for stable kernels.)

Maybe BPF LSM should at least emit warning if the loaded BPF program defined
security_file_ioctl() hook and did not define security_file_ioctl_compat() hook?

We could use a struct where undefined hooks needs to be manually filled with
a dummy pointer, so that we can catch erroneously undefined hooks (detected by
being automatically filled with a NULL pointer) at load time?

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Eric Biggers]

On Fri, Dec 22, 2023 at 08:23:26PM -0500, Paul Moore wrote:
> 
> Is it considered valid for a native 64-bit task to use 32-bit
> FS_IO32_XXX flags?

No, that's not valid.

> If not, do we want to remove the FS_IO32_XXX flag
> checks in selinux_file_ioctl()?

I don't see any such flag checks in selinux_file_ioctl().

Is there something else you have in mind?

- Eric

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Casey Schaufler]

On 12/22/2023 5:23 PM, Paul Moore wrote:
> On Tue, Dec 19, 2023 at 4:09 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 syscall. All current LSMs have been changed
>> 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
>> ---
>>  fs/ioctl.c                    |  3 +--
>>  include/linux/lsm_hook_defs.h |  2 ++
>>  include/linux/security.h      |  7 +++++++
>>  security/security.c           | 17 +++++++++++++++++
>>  security/selinux/hooks.c      | 28 ++++++++++++++++++++++++++++
>>  security/smack/smack_lsm.c    |  1 +
>>  security/tomoyo/tomoyo.c      |  1 +
>>  7 files changed, 57 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ioctl.c b/fs/ioctl.c
>> index f5fd99d6b0d4..76cf22ac97d7 100644
>> --- a/fs/ioctl.c
>> +++ b/fs/ioctl.c
>> @@ -920,8 +920,7 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd,
>>         if (!f.file)
>>                 return -EBADF;
>>
>> -       /* RED-PEN how should LSM module know it's handling 32bit? */
>> -       error = security_file_ioctl(f.file, cmd, arg);
>> +       error = security_file_ioctl_compat(f.file, cmd, arg);
>>         if (error)
>>                 goto out;
> This is interesting ... if you look at the normal ioctl() syscall
> definition in the kernel you see 'ioctl(unsigned int fd, unsigned int
> cmd, unsigned long arg)' and if you look at the compat definition you
> see 'ioctl(unsigned int fd, unsigned int cmd, compat_ulong_t arg)'.  I
> was expecting the second parameter, @cmd, to be a long type in the
> normal definition, but it is an int type in both cases.  It looks like
> it has been that way long enough that it is correct, but I'm a little
> lost ...
>
>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>> index ac962c4cb44b..626aa8cf930d 100644
>> --- a/include/linux/lsm_hook_defs.h
>> +++ b/include/linux/lsm_hook_defs.h
>> @@ -171,6 +171,8 @@ LSM_HOOK(int, 0, file_alloc_security, struct file *file)
>>  LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
>>  LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
>>          unsigned long arg)
>> +LSM_HOOK(int, 0, file_ioctl_compat, struct file *file, unsigned int cmd,
>> +        unsigned long arg)
>>  LSM_HOOK(int, 0, mmap_addr, unsigned long addr)
>>  LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot,
>>          unsigned long prot, unsigned long flags)
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 5f16eecde00b..22a82b7c59f1 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -389,6 +389,7 @@ int security_file_permission(struct file *file, int mask);
>>  int security_file_alloc(struct file *file);
>>  void security_file_free(struct file *file);
>>  int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
>> +int security_file_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg);
>>  int security_mmap_file(struct file *file, unsigned long prot,
>>                         unsigned long flags);
>>  int security_mmap_addr(unsigned long addr);
>> @@ -987,6 +988,12 @@ static inline int security_file_ioctl(struct file *file, unsigned int cmd,
>>         return 0;
>>  }
>>
>> +static inline int security_file_ioctl_compat(struct file *file, unsigned int cmd,
>> +                                     unsigned long arg)
>> +{
>> +       return 0;
>> +}
>> +
>>  static inline int security_mmap_file(struct file *file, unsigned long prot,
>>                                      unsigned long flags)
>>  {
>> diff --git a/security/security.c b/security/security.c
>> index 23b129d482a7..5c16ffc99b1e 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2648,6 +2648,23 @@ int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>>  }
>>  EXPORT_SYMBOL_GPL(security_file_ioctl);
>>
>> +/**
>> + * security_file_ioctl_compat() - Check if an ioctl is allowed in 32-bit compat mode
>> + * @file: associated file
>> + * @cmd: ioctl cmd
>> + * @arg: ioctl arguments
>> + *
>> + * Compat version of security_file_ioctl() that correctly handles 32-bit processes
>> + * running on 64-bit kernels.
>> + *
>> + * Return: Returns 0 if permission is granted.
>> + */
>> +int security_file_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg)
>> +{
>> +       return call_int_hook(file_ioctl_compat, 0, file, cmd, arg);
>> +}
>> +EXPORT_SYMBOL_GPL(security_file_ioctl_compat);
>> +
>>  static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
>>  {
>>         /*
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 2aa0e219d721..c617ae21dba8 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -3731,6 +3731,33 @@ 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.
>> +        */
>> +       switch (cmd) {
>> +       case FS_IOC32_GETFLAGS:
>> +               cmd = FS_IOC_GETFLAGS;
>> +               break;
>> +       case FS_IOC32_SETFLAGS:
>> +               cmd = FS_IOC_SETFLAGS;
>> +               break;
>> +       case FS_IOC32_GETVERSION:
>> +               cmd = FS_IOC_GETVERSION;
>> +               break;
>> +       case FS_IOC32_SETVERSION:
>> +               cmd = FS_IOC_SETVERSION;
>> +               break;
>> +       default:
>> +               break;
>> +       }
>> +
>> +       return selinux_file_ioctl(file, cmd, arg);
>> +}
> Is it considered valid for a native 64-bit task to use 32-bit
> FS_IO32_XXX flags?  If not, do we want to remove the FS_IO32_XXX flag
> checks in selinux_file_ioctl()?
>
>>  static int default_noexec __ro_after_init;
>>
>>  static int file_map_prot_check(struct file *file, unsigned long prot, int shared)
>> @@ -7036,6 +7063,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
>>         LSM_HOOK_INIT(file_permission, selinux_file_permission),
>>         LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
>>         LSM_HOOK_INIT(file_ioctl, selinux_file_ioctl),
>> +       LSM_HOOK_INIT(file_ioctl_compat, selinux_file_ioctl_compat),
>>         LSM_HOOK_INIT(mmap_file, selinux_mmap_file),
>>         LSM_HOOK_INIT(mmap_addr, selinux_mmap_addr),
>>         LSM_HOOK_INIT(file_mprotect, selinux_file_mprotect),
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 65130a791f57..1f1ea8529421 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -4973,6 +4973,7 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
>>
>>         LSM_HOOK_INIT(file_alloc_security, smack_file_alloc_security),
>>         LSM_HOOK_INIT(file_ioctl, smack_file_ioctl),
>> +       LSM_HOOK_INIT(file_ioctl_compat, smack_file_ioctl),
>>         LSM_HOOK_INIT(file_lock, smack_file_lock),
>>         LSM_HOOK_INIT(file_fcntl, smack_file_fcntl),
>>         LSM_HOOK_INIT(mmap_file, smack_mmap_file),
>> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
>> index 25006fddc964..298d182759c2 100644
>> --- a/security/tomoyo/tomoyo.c
>> +++ b/security/tomoyo/tomoyo.c
>> @@ -568,6 +568,7 @@ static struct security_hook_list tomoyo_hooks[] __ro_after_init = {
>>         LSM_HOOK_INIT(path_rename, tomoyo_path_rename),
>>         LSM_HOOK_INIT(inode_getattr, tomoyo_inode_getattr),
>>         LSM_HOOK_INIT(file_ioctl, tomoyo_file_ioctl),
>> +       LSM_HOOK_INIT(file_ioctl_compat, tomoyo_file_ioctl),
>>         LSM_HOOK_INIT(path_chmod, tomoyo_path_chmod),
>>         LSM_HOOK_INIT(path_chown, tomoyo_path_chown),
>>         LSM_HOOK_INIT(path_chroot, tomoyo_path_chroot),
> I agree that it looks like Smack and TOMOYO should be fine, but I
> would like to hear from Casey and Tetsuo to confirm.

Smack should be OK.

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Paul Moore]

On Sat, Dec 23, 2023 at 5:49 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> On 2023/12/23 10:23, Paul Moore wrote:
> >> -       /* RED-PEN how should LSM module know it's handling 32bit? */
> >> -       error = security_file_ioctl(f.file, cmd, arg);
> >> +       error = security_file_ioctl_compat(f.file, cmd, arg);
> >>         if (error)
> >>                 goto out;
> >
> > This is interesting ... if you look at the normal ioctl() syscall
> > definition in the kernel you see 'ioctl(unsigned int fd, unsigned int
> > cmd, unsigned long arg)' and if you look at the compat definition you
> > see 'ioctl(unsigned int fd, unsigned int cmd, compat_ulong_t arg)'.  I
> > was expecting the second parameter, @cmd, to be a long type in the
> > normal definition, but it is an int type in both cases.  It looks like
> > it has been that way long enough that it is correct, but I'm a little
> > lost ...
>
> Since @arg might be a pointer to some struct, @arg needs to use a long type.
> But @cmd can remain 32bits for both 32bits/64bits kernels because @cmd is not
> a pointer, can't it?

I'm not worried about @arg, I'm worried about @cmd, the second
parameter to the syscall.  I was looking at the manpage and it is
specified as an unsigned long, which would be a size mismatch on a
64-bit system, although now that I'm reading further into the manpage
I see that the command is specified as a 32-bit value so an int
shouldn't be a problem.  I'm guessing the unsigned long type persists
from the days before 64-bit systems.

> > I agree that it looks like Smack and TOMOYO should be fine, but I
> > would like to hear from Casey and Tetsuo to confirm.
>
> Fine for TOMOYO part, for TOMOYO treats @cmd as an integer.

Great, thank you.

-- 
paul-moore.com

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Paul Moore]

On Sat, Dec 23, 2023 at 10:34 AM Eric Biggers <ebiggers@kernel.org> wrote:
> On Fri, Dec 22, 2023 at 08:23:26PM -0500, Paul Moore wrote:
> > Is it considered valid for a native 64-bit task to use 32-bit
> > FS_IO32_XXX flags?
>
> No, that's not valid.

Excellent, thank you.

> > If not, do we want to remove the FS_IO32_XXX flag
> > checks in selinux_file_ioctl()?
>
> I don't see any such flag checks in selinux_file_ioctl().

Neither do I ... I'm not sure what I was looking at when I made that
comment, I'm going to chalk that up to a bit of holiday fog.  Sorry
for the noise.

> Is there something else you have in mind?

Nope.

-- 
paul-moore.com

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Paul Moore]

On Sun, Dec 24, 2023 at 3:00 PM Paul Moore <paul@paul-moore.com> wrote:
> On Sat, Dec 23, 2023 at 10:34 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > On Fri, Dec 22, 2023 at 08:23:26PM -0500, Paul Moore wrote:
> > > Is it considered valid for a native 64-bit task to use 32-bit
> > > FS_IO32_XXX flags?
> >
> > No, that's not valid.
>
> Excellent, thank you.
>
> > > If not, do we want to remove the FS_IO32_XXX flag
> > > checks in selinux_file_ioctl()?
> >
> > I don't see any such flag checks in selinux_file_ioctl().
>
> Neither do I ... I'm not sure what I was looking at when I made that
> comment, I'm going to chalk that up to a bit of holiday fog.  Sorry
> for the noise.

Ah ha, I think I found the problem - the tools I use to pull in
patches for review seemed to have grabbed an old version of the patch
that *did* as the 32-bit ioctl commands to selinux_file_ioctl().

https://lore.kernel.org/selinux/20230906102557.3432236-1-alpic@google.com/

-- 
paul-moore.com

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Paul Moore]

On Tue, Dec 19, 2023 at 4:09 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 syscall. All current LSMs have been changed
> 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
> ---
>  fs/ioctl.c                    |  3 +--
>  include/linux/lsm_hook_defs.h |  2 ++
>  include/linux/security.h      |  7 +++++++
>  security/security.c           | 17 +++++++++++++++++
>  security/selinux/hooks.c      | 28 ++++++++++++++++++++++++++++
>  security/smack/smack_lsm.c    |  1 +
>  security/tomoyo/tomoyo.c      |  1 +
>  7 files changed, 57 insertions(+), 2 deletions(-)

I made some minor style tweaks around line length and alignment, but
otherwise this looked good to me.  Thanks all!

While I agree this is definitely stable kernel material, given where
we are at in the current kernel cycle, and with the end-of-year
holidays in full swing, I'm going to merge this into lsm/dev and send
it up to Linus during the next merge window.  The stable tag will
remain intact, so it will end up trickling down into the stable
kernels, it will just take an extra week or so (which I think will be
good from a testing perspective).

-- 
paul-moore.com

Re: [PATCH] security: new security_file_ioctl_compat() hook

[Eric Biggers]

On Sun, Dec 24, 2023 at 03:53:16PM -0500, Paul Moore wrote:
> On Tue, Dec 19, 2023 at 4:09 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 syscall. All current LSMs have been changed
> > 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
> > ---
> >  fs/ioctl.c                    |  3 +--
> >  include/linux/lsm_hook_defs.h |  2 ++
> >  include/linux/security.h      |  7 +++++++
> >  security/security.c           | 17 +++++++++++++++++
> >  security/selinux/hooks.c      | 28 ++++++++++++++++++++++++++++
> >  security/smack/smack_lsm.c    |  1 +
> >  security/tomoyo/tomoyo.c      |  1 +
> >  7 files changed, 57 insertions(+), 2 deletions(-)
> 
> I made some minor style tweaks around line length and alignment, but
> otherwise this looked good to me.  Thanks all!
> 

Reviewed-by: Eric Biggers <ebiggers@google.com>

(I reviewed the version in branch "next" of
https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git)

- Eric