| Message ID | 20200724001248.GC25522@altlinux.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs/nsfs.c: fix ioctl support of compat processes | expand |
On Fri, Jul 24, 2020 at 2:12 AM Dmitry V. Levin <ldv@altlinux.org> wrote: > > According to Documentation/driver-api/ioctl.rst, in order to support > 32-bit user space running on a 64-bit kernel, each subsystem or driver > that implements an ioctl callback handler must also implement the > corresponding compat_ioctl handler. The compat_ptr_ioctl() helper can > be used in place of a custom compat_ioctl file operation for drivers > that only take arguments that are pointers to compatible data > structures. > > In case of NS_* ioctls only NS_GET_OWNER_UID accepts an argument, and > this argument is a pointer to uid_t type, which is universally defined > to __kernel_uid32_t. This is potentially dangerous to rely on, as there are two parts that are mismatched: - user space does not see the kernel's uid_t definition, but has its own, which may be either the 16-bit or the 32-bit type. 32-bit uid_t was introduced with linux-2.3.39 in back in 2000. glibc was already using 32-bit uid_t at the time in user space, but uclibc only changed in 2003, and others may have been even later. - the ioctl command number is defined (incorrectly) as if there was no argument, so if there is any user space that happens to be built with a 16-bit uid_t, this does not get caught. Arnd > Reported-by: Ákos Uzonyi <uzonyi.akos@gmail.com> > Fixes: 6786741dbf99 ("nsfs: add ioctl to get an owning user namespace for ns file descriptor") > Cc: stable@vger.kernel.org # v4.9+ > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> > --- > fs/nsfs.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/nsfs.c b/fs/nsfs.c > index 800c1d0eb0d0..a00236bffa2c 100644 > --- a/fs/nsfs.c > +++ b/fs/nsfs.c > @@ -21,6 +21,7 @@ static long ns_ioctl(struct file *filp, unsigned int ioctl, > static const struct file_operations ns_file_operations = { > .llseek = no_llseek, > .unlocked_ioctl = ns_ioctl, > + .compat_ioctl = compat_ptr_ioctl, > }; > > static char *ns_dname(struct dentry *dentry, char *buffer, int buflen)
On Fri, Jul 24, 2020 at 11:20:26AM +0200, Arnd Bergmann wrote: > On Fri, Jul 24, 2020 at 2:12 AM Dmitry V. Levin <ldv@altlinux.org> wrote: > > > > According to Documentation/driver-api/ioctl.rst, in order to support > > 32-bit user space running on a 64-bit kernel, each subsystem or driver > > that implements an ioctl callback handler must also implement the > > corresponding compat_ioctl handler. The compat_ptr_ioctl() helper can > > be used in place of a custom compat_ioctl file operation for drivers > > that only take arguments that are pointers to compatible data > > structures. > > > > In case of NS_* ioctls only NS_GET_OWNER_UID accepts an argument, and > > this argument is a pointer to uid_t type, which is universally defined > > to __kernel_uid32_t. > > This is potentially dangerous to rely on, as there are two parts that > are mismatched: > > - user space does not see the kernel's uid_t definition, but has its own, > which may be either the 16-bit or the 32-bit type. 32-bit uid_t was > introduced with linux-2.3.39 in back in 2000. glibc was already > using 32-bit uid_t at the time in user space, but uclibc only changed > in 2003, and others may have been even later. > > - the ioctl command number is defined (incorrectly) as if there was no > argument, so if there is any user space that happens to be built with > a 16-bit uid_t, this does not get caught. Note that NS_GET_OWNER_UID is provided on 32-bit architectures, too, so this 16-bit vs 32-bit uid_t issue was exposed to userspace long time ago when NS_GET_OWNER_UID was introduced, and making NS_GET_OWNER_UID available for compat processes won't make any difference, as the mismatch is not between native and compat types, but rather between 16-bit and 32-bit uid_t types. I agree it would be correct to define NS_GET_OWNER_UID as _IOR(NSIO, 0x4, uid_t) instead of _IO(NSIO, 0x4), but nobody Cc'ed me on this topic when NS_GET_OWNER_UID was discussed, and that ship has long sailed. > > This change fixes compat strace --pidns-translation. > > > > Note: when backporting this patch to stable kernels, commit > > "compat_ioctl: add compat_ptr_ioctl()" is needed as well. > > > > Reported-by: Ákos Uzonyi <uzonyi.akos@gmail.com> > > Fixes: 6786741dbf99 ("nsfs: add ioctl to get an owning user namespace for ns file descriptor") > > Cc: stable@vger.kernel.org # v4.9+ > > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> > > --- > > fs/nsfs.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/fs/nsfs.c b/fs/nsfs.c > > index 800c1d0eb0d0..a00236bffa2c 100644 > > --- a/fs/nsfs.c > > +++ b/fs/nsfs.c > > @@ -21,6 +21,7 @@ static long ns_ioctl(struct file *filp, unsigned int ioctl, > > static const struct file_operations ns_file_operations = { > > .llseek = no_llseek, > > .unlocked_ioctl = ns_ioctl, > > + .compat_ioctl = compat_ptr_ioctl, > > }; > > > > static char *ns_dname(struct dentry *dentry, char *buffer, int buflen)
Michael, As the original author of NS_GET_OWNER_UID can you take a look at this? "Dmitry V. Levin" <ldv@altlinux.org> writes: > On Fri, Jul 24, 2020 at 11:20:26AM +0200, Arnd Bergmann wrote: >> On Fri, Jul 24, 2020 at 2:12 AM Dmitry V. Levin <ldv@altlinux.org> wrote: >> > >> > According to Documentation/driver-api/ioctl.rst, in order to support >> > 32-bit user space running on a 64-bit kernel, each subsystem or driver >> > that implements an ioctl callback handler must also implement the >> > corresponding compat_ioctl handler. The compat_ptr_ioctl() helper can >> > be used in place of a custom compat_ioctl file operation for drivers >> > that only take arguments that are pointers to compatible data >> > structures. >> > >> > In case of NS_* ioctls only NS_GET_OWNER_UID accepts an argument, and >> > this argument is a pointer to uid_t type, which is universally defined >> > to __kernel_uid32_t. >> >> This is potentially dangerous to rely on, as there are two parts that >> are mismatched: >> >> - user space does not see the kernel's uid_t definition, but has its own, >> which may be either the 16-bit or the 32-bit type. 32-bit uid_t was >> introduced with linux-2.3.39 in back in 2000. glibc was already >> using 32-bit uid_t at the time in user space, but uclibc only changed >> in 2003, and others may have been even later. >> >> - the ioctl command number is defined (incorrectly) as if there was no >> argument, so if there is any user space that happens to be built with >> a 16-bit uid_t, this does not get caught. > > Note that NS_GET_OWNER_UID is provided on 32-bit architectures, too, so > this 16-bit vs 32-bit uid_t issue was exposed to userspace long time ago > when NS_GET_OWNER_UID was introduced, and making NS_GET_OWNER_UID > available for compat processes won't make any difference, as the mismatch > is not between native and compat types, but rather between 16-bit and > 32-bit uid_t types. > > I agree it would be correct to define NS_GET_OWNER_UID as > _IOR(NSIO, 0x4, uid_t) instead of _IO(NSIO, 0x4), but nobody Cc'ed me > on this topic when NS_GET_OWNER_UID was discussed, and that ship has long > sailed. > >> > This change fixes compat strace --pidns-translation. >> > >> > Note: when backporting this patch to stable kernels, commit >> > "compat_ioctl: add compat_ptr_ioctl()" is needed as well. >> > >> > Reported-by: Ákos Uzonyi <uzonyi.akos@gmail.com> >> > Fixes: 6786741dbf99 ("nsfs: add ioctl to get an owning user namespace for ns file descriptor") >> > Cc: stable@vger.kernel.org # v4.9+ >> > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> >> > --- >> > fs/nsfs.c | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/fs/nsfs.c b/fs/nsfs.c >> > index 800c1d0eb0d0..a00236bffa2c 100644 >> > --- a/fs/nsfs.c >> > +++ b/fs/nsfs.c >> > @@ -21,6 +21,7 @@ static long ns_ioctl(struct file *filp, unsigned int ioctl, >> > static const struct file_operations ns_file_operations = { >> > .llseek = no_llseek, >> > .unlocked_ioctl = ns_ioctl, >> > + .compat_ioctl = compat_ptr_ioctl, >> > }; >> > >> > static char *ns_dname(struct dentry *dentry, char *buffer, int buflen) Thank you, Eric
Hi, On Fri, Jul 24, 2020 at 02:31:19PM -0500, Eric W. Biederman wrote: > Michael, > > As the original author of NS_GET_OWNER_UID can you take a look at this? This is a gentle reminder that my patch hasn't been applied, the problem reported by Ákos Uzonyi hasn't been fixed, and the example in ioctl_ns(2) manual page doesn't work when e.g. it's compiled with -m32 on a 64-bit kernel. > "Dmitry V. Levin" <ldv@altlinux.org> writes: > > On Fri, Jul 24, 2020 at 11:20:26AM +0200, Arnd Bergmann wrote: > >> On Fri, Jul 24, 2020 at 2:12 AM Dmitry V. Levin wrote: > >> > > >> > According to Documentation/driver-api/ioctl.rst, in order to support > >> > 32-bit user space running on a 64-bit kernel, each subsystem or driver > >> > that implements an ioctl callback handler must also implement the > >> > corresponding compat_ioctl handler. The compat_ptr_ioctl() helper can > >> > be used in place of a custom compat_ioctl file operation for drivers > >> > that only take arguments that are pointers to compatible data > >> > structures. > >> > > >> > In case of NS_* ioctls only NS_GET_OWNER_UID accepts an argument, and > >> > this argument is a pointer to uid_t type, which is universally defined > >> > to __kernel_uid32_t. > >> > >> This is potentially dangerous to rely on, as there are two parts that > >> are mismatched: > >> > >> - user space does not see the kernel's uid_t definition, but has its own, > >> which may be either the 16-bit or the 32-bit type. 32-bit uid_t was > >> introduced with linux-2.3.39 in back in 2000. glibc was already > >> using 32-bit uid_t at the time in user space, but uclibc only changed > >> in 2003, and others may have been even later. > >> > >> - the ioctl command number is defined (incorrectly) as if there was no > >> argument, so if there is any user space that happens to be built with > >> a 16-bit uid_t, this does not get caught. > > > > Note that NS_GET_OWNER_UID is provided on 32-bit architectures, too, so > > this 16-bit vs 32-bit uid_t issue was exposed to userspace long time ago > > when NS_GET_OWNER_UID was introduced, and making NS_GET_OWNER_UID > > available for compat processes won't make any difference, as the mismatch > > is not between native and compat types, but rather between 16-bit and > > 32-bit uid_t types. > > > > I agree it would be correct to define NS_GET_OWNER_UID as > > _IOR(NSIO, 0x4, uid_t) instead of _IO(NSIO, 0x4), but nobody Cc'ed me > > on this topic when NS_GET_OWNER_UID was discussed, and that ship has long > > sailed. > > > >> > This change fixes compat strace --pidns-translation. > >> > > >> > Note: when backporting this patch to stable kernels, commit > >> > "compat_ioctl: add compat_ptr_ioctl()" is needed as well. > >> > > >> > Reported-by: Ákos Uzonyi <uzonyi.akos@gmail.com> > >> > Fixes: 6786741dbf99 ("nsfs: add ioctl to get an owning user namespace for ns file descriptor") > >> > Cc: stable@vger.kernel.org # v4.9+ > >> > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> > >> > --- > >> > fs/nsfs.c | 1 + > >> > 1 file changed, 1 insertion(+) > >> > > >> > diff --git a/fs/nsfs.c b/fs/nsfs.c > >> > index 800c1d0eb0d0..a00236bffa2c 100644 > >> > --- a/fs/nsfs.c > >> > +++ b/fs/nsfs.c > >> > @@ -21,6 +21,7 @@ static long ns_ioctl(struct file *filp, unsigned int ioctl, > >> > static const struct file_operations ns_file_operations = { > >> > .llseek = no_llseek, > >> > .unlocked_ioctl = ns_ioctl, > >> > + .compat_ioctl = compat_ptr_ioctl, > >> > }; > >> > > >> > static char *ns_dname(struct dentry *dentry, char *buffer, int buflen) > > Thank you, > Eric
diff --git a/fs/nsfs.c b/fs/nsfs.c index 800c1d0eb0d0..a00236bffa2c 100644 --- a/fs/nsfs.c +++ b/fs/nsfs.c @@ -21,6 +21,7 @@ static long ns_ioctl(struct file *filp, unsigned int ioctl, static const struct file_operations ns_file_operations = { .llseek = no_llseek, .unlocked_ioctl = ns_ioctl, + .compat_ioctl = compat_ptr_ioctl, }; static char *ns_dname(struct dentry *dentry, char *buffer, int buflen)
According to Documentation/driver-api/ioctl.rst, in order to support 32-bit user space running on a 64-bit kernel, each subsystem or driver that implements an ioctl callback handler must also implement the corresponding compat_ioctl handler. The compat_ptr_ioctl() helper can be used in place of a custom compat_ioctl file operation for drivers that only take arguments that are pointers to compatible data structures. In case of NS_* ioctls only NS_GET_OWNER_UID accepts an argument, and this argument is a pointer to uid_t type, which is universally defined to __kernel_uid32_t. This change fixes compat strace --pidns-translation. Note: when backporting this patch to stable kernels, commit "compat_ioctl: add compat_ptr_ioctl()" is needed as well. Reported-by: Ákos Uzonyi <uzonyi.akos@gmail.com> Fixes: 6786741dbf99 ("nsfs: add ioctl to get an owning user namespace for ns file descriptor") Cc: stable@vger.kernel.org # v4.9+ Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> --- fs/nsfs.c | 1 + 1 file changed, 1 insertion(+)