fs/nsfs.c: fix ioctl support of compat processes
PATCH 1/1 fs/nsfs.c: fix ioctl support of compat processes
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.
--- fs/nsfs.c | 1 + 1 file changed, 1 insertion(+)
fs/nsfs.c +1 -0
@@ -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)
Signature · 1 line
ldv
Quoted 9 lines · Dmitry V. Levin
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 correspondingcompat_ioctlhandler. Thecompat_ptr_ioctl()helper can be used in place of a customcompat_ioctlfile operation for drivers that only take arguments that are pointers to compatible data structures.
Quoted 3 lines
In case of NS_* ioctls only
NS_GET_OWNER_UIDaccepts an argument, and this argument is a pointer touid_ttype, 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
Quoted 20 lines
Reported-by Ákos Uzonyi <[email protected]>Fixes 6786741dbf99 ("nsfs: add ioctl to get an owning user namespace for ns file descriptor")Cc [email protected] # v4.9+Signed-off-by Dmitry V. Levin <[email protected]>--- fs/nsfs.c | 1 + 1 file changed, 1 insertion(+)fs/nsfs.c +1
-0@@ -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)
Quoted 23 lines · Arnd Bergmann
Quoted 9 lines · Dmitry V. Levin
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 correspondingcompat_ioctlhandler. Thecompat_ptr_ioctl()helper can be used in place of a customcompat_ioctlfile operation for drivers that only take arguments that are pointers to compatible data structures.Quoted 3 lines
In case of NS_* ioctls only
NS_GET_OWNER_UIDaccepts an argument, and this argument is a pointer touid_ttype, 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_tdefinition, but has its own, which may be either the 16-bit or the 32-bit type. 32-bituid_twas introduced with linux-2.3.39 in back in 2000. glibc was already using 32-bituid_tat the time in user space, but uclibc only changed in 2003, and others may have been even later.
Quoted 3 lines
- 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.
Quoted 25 lines
Quoted 25 lines
This change fixes compat strace --pidns-translation.
Note: when backporting this patch to stable kernels, commit "
compat_ioctl: addcompat_ptr_ioctl()" is needed as well.Reported-by Ákos Uzonyi <[email protected]>Fixes 6786741dbf99 ("nsfs: add ioctl to get an owning user namespace for ns file descriptor")Cc [email protected] # v4.9+Signed-off-by Dmitry V. Levin <[email protected]>--- fs/nsfs.c | 1 + 1 file changed, 1 insertion(+)fs/nsfs.c +1
-0@@ -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)
Signature · 1 line
ldv
Michael,
As the original author of NS_GET_OWNER_UID can you take a look at this?
Quoted 64 lines · Dmitry V. Levin
Quoted 23 lines · Arnd Bergmann
Quoted 9 lines · Dmitry V. Levin
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 correspondingcompat_ioctlhandler. Thecompat_ptr_ioctl()helper can be used in place of a customcompat_ioctlfile operation for drivers that only take arguments that are pointers to compatible data structures.Quoted 3 lines
In case of NS_* ioctls only
NS_GET_OWNER_UIDaccepts an argument, and this argument is a pointer touid_ttype, 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_tdefinition, but has its own, which may be either the 16-bit or the 32-bit type. 32-bituid_twas introduced with linux-2.3.39 in back in 2000. glibc was already using 32-bituid_tat the time in user space, but uclibc only changed in 2003, and others may have been even later.Quoted 3 lines
- 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_UIDis provided on 32-bit architectures, too, so this 16-bit vs 32-bituid_tissue was exposed to userspace long time ago whenNS_GET_OWNER_UIDwas introduced, and makingNS_GET_OWNER_UIDavailable 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-bituid_ttypes.I agree it would be correct to define
NS_GET_OWNER_UIDas_IOR(NSIO, 0x4, uid_t)instead of_IO(NSIO, 0x4), but nobody Cc'ed me on this topic whenNS_GET_OWNER_UIDwas discussed, and that ship has long sailed.Quoted 24 lines
Quoted 24 lines
This change fixes compat strace --pidns-translation.
Note: when backporting this patch to stable kernels, commit "
compat_ioctl: addcompat_ptr_ioctl()" is needed as well.Reported-by Ákos Uzonyi <[email protected]>Fixes 6786741dbf99 ("nsfs: add ioctl to get an owning user namespace for ns file descriptor")Cc [email protected] # v4.9+Signed-off-by Dmitry V. Levin <[email protected]>--- fs/nsfs.c | 1 + 1 file changed, 1 insertion(+)fs/nsfs.c +1
-0@@ -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, };
Quoted 1 line
Quoted 1 line
fs/nsfs.c static long ns_ioctl(struct file *filp, unsigned int ioctl, · line 27
Quoted 1 line
fs/nsfs.c +0
-0static char *ns_dname (struct dentry *dentry,char *buffer,int buflen)
Thank you,
Eric
Hi,
Quoted 3 lines · Eric W. Biederman
Michael,
As the original author of
NS_GET_OWNER_UIDcan 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.
Quoted 69 lines
Quoted 64 lines · Dmitry V. Levin
Quoted 23 lines · Arnd Bergmann
Quoted 9 lines · Dmitry V. Levin
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 correspondingcompat_ioctlhandler. Thecompat_ptr_ioctl()helper can be used in place of a customcompat_ioctlfile operation for drivers that only take arguments that are pointers to compatible data structures.Quoted 3 lines
In case of NS_* ioctls only
NS_GET_OWNER_UIDaccepts an argument, and this argument is a pointer touid_ttype, 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_tdefinition, but has its own, which may be either the 16-bit or the 32-bit type. 32-bituid_twas introduced with linux-2.3.39 in back in 2000. glibc was already using 32-bituid_tat the time in user space, but uclibc only changed in 2003, and others may have been even later.Quoted 3 lines
- 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_UIDis provided on 32-bit architectures, too, so this 16-bit vs 32-bituid_tissue was exposed to userspace long time ago whenNS_GET_OWNER_UIDwas introduced, and makingNS_GET_OWNER_UIDavailable 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-bituid_ttypes.I agree it would be correct to define
NS_GET_OWNER_UIDas_IOR(NSIO, 0x4, uid_t)instead of_IO(NSIO, 0x4), but nobody Cc'ed me on this topic whenNS_GET_OWNER_UIDwas discussed, and that ship has long sailed.Quoted 24 lines
Quoted 24 lines
This change fixes compat strace --pidns-translation.
Note: when backporting this patch to stable kernels, commit "
compat_ioctl: addcompat_ptr_ioctl()" is needed as well.Reported-by Ákos Uzonyi <[email protected]>Fixes 6786741dbf99 ("nsfs: add ioctl to get an owning user namespace for ns file descriptor")Cc [email protected] # v4.9+Signed-off-by Dmitry V. Levin <[email protected]>--- fs/nsfs.c | 1 + 1 file changed, 1 insertion(+)fs/nsfs.c +1
-0@@ -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, };Quoted 1 line
Quoted 1 line
fs/nsfs.c static long ns_ioctl(struct file *filp, unsigned int ioctl, · line 27
Quoted 1 line
fs/nsfs.c +0
-0static char *ns_dname (struct dentry *dentry,char *buffer,int buflen)Thank you,
Eric
Signature · 1 line
ldv