Messages in this thread | | | Subject | Re: [PATCH 1/2] signal: Factor copy_siginfo_to_external32 from copy_siginfo_to_user32 | From | Christophe Leroy <> | Date | Sat, 18 Apr 2020 10:05:19 +0200 |
| |
Le 17/04/2020 à 23:09, Eric W. Biederman a écrit : > > To remove the use of set_fs in the coredump code there needs to be a > way to convert a kernel siginfo to a userspace compat siginfo. > > Call that function copy_siginfo_to_compat and factor it out of > copy_siginfo_to_user32.
I find it a pitty to do that.
The existing function could have been easily converted to using user_access_begin() + user_access_end() and use unsafe_put_user() to copy to userspace to avoid copying through a temporary structure on the stack.
With your change, it becomes impossible to do that.
Is that really an issue to use that set_fs() in the coredump code ?
Christophe
> > The existence of x32 complicates this code. On x32 SIGCHLD uses 64bit > times for utime and stime. As only SIGCHLD is affected and SIGCHLD > never causes a coredump I have avoided handling that case. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > include/linux/compat.h | 1 + > kernel/signal.c | 108 +++++++++++++++++++++++------------------ > 2 files changed, 63 insertions(+), 46 deletions(-) > > diff --git a/include/linux/compat.h b/include/linux/compat.h > index 0480ba4db592..4962b254e550 100644 > --- a/include/linux/compat.h > +++ b/include/linux/compat.h > @@ -402,6 +402,7 @@ long compat_get_bitmap(unsigned long *mask, const compat_ulong_t __user *umask, > unsigned long bitmap_size); > long compat_put_bitmap(compat_ulong_t __user *umask, unsigned long *mask, > unsigned long bitmap_size); > +void copy_siginfo_to_external32(struct compat_siginfo *to, const struct kernel_siginfo *from); > int copy_siginfo_from_user32(kernel_siginfo_t *to, const struct compat_siginfo __user *from); > int copy_siginfo_to_user32(struct compat_siginfo __user *to, const kernel_siginfo_t *from); > int get_compat_sigevent(struct sigevent *event, > diff --git a/kernel/signal.c b/kernel/signal.c > index e58a6c619824..578f196898cb 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -3235,90 +3235,106 @@ int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from) > } > > #ifdef CONFIG_COMPAT > -int copy_siginfo_to_user32(struct compat_siginfo __user *to, > - const struct kernel_siginfo *from) > -#if defined(CONFIG_X86_X32_ABI) || defined(CONFIG_IA32_EMULATION) > +void copy_siginfo_to_external32(struct compat_siginfo *to, > + const struct kernel_siginfo *from) > { > - return __copy_siginfo_to_user32(to, from, in_x32_syscall()); > -} > -int __copy_siginfo_to_user32(struct compat_siginfo __user *to, > - const struct kernel_siginfo *from, bool x32_ABI) > -#endif > -{ > - struct compat_siginfo new; > - memset(&new, 0, sizeof(new)); > + /* > + * This function does not work properly for SIGCHLD on x32, > + * but it does not need to as SIGCHLD never causes a coredump. > + */ > + memset(to, 0, sizeof(*to)); > > - new.si_signo = from->si_signo; > - new.si_errno = from->si_errno; > - new.si_code = from->si_code; > + to->si_signo = from->si_signo; > + to->si_errno = from->si_errno; > + to->si_code = from->si_code; > switch(siginfo_layout(from->si_signo, from->si_code)) { > case SIL_KILL: > - new.si_pid = from->si_pid; > - new.si_uid = from->si_uid; > + to->si_pid = from->si_pid; > + to->si_uid = from->si_uid; > break; > case SIL_TIMER: > - new.si_tid = from->si_tid; > - new.si_overrun = from->si_overrun; > - new.si_int = from->si_int; > + to->si_tid = from->si_tid; > + to->si_overrun = from->si_overrun; > + to->si_int = from->si_int; > break; > case SIL_POLL: > - new.si_band = from->si_band; > - new.si_fd = from->si_fd; > + to->si_band = from->si_band; > + to->si_fd = from->si_fd; > break; > case SIL_FAULT: > - new.si_addr = ptr_to_compat(from->si_addr); > + to->si_addr = ptr_to_compat(from->si_addr); > #ifdef __ARCH_SI_TRAPNO > - new.si_trapno = from->si_trapno; > + to->si_trapno = from->si_trapno; > #endif > break; > case SIL_FAULT_MCEERR: > - new.si_addr = ptr_to_compat(from->si_addr); > + to->si_addr = ptr_to_compat(from->si_addr); > #ifdef __ARCH_SI_TRAPNO > - new.si_trapno = from->si_trapno; > + to->si_trapno = from->si_trapno; > #endif > - new.si_addr_lsb = from->si_addr_lsb; > + to->si_addr_lsb = from->si_addr_lsb; > break; > case SIL_FAULT_BNDERR: > - new.si_addr = ptr_to_compat(from->si_addr); > + to->si_addr = ptr_to_compat(from->si_addr); > #ifdef __ARCH_SI_TRAPNO > - new.si_trapno = from->si_trapno; > + to->si_trapno = from->si_trapno; > #endif > - new.si_lower = ptr_to_compat(from->si_lower); > - new.si_upper = ptr_to_compat(from->si_upper); > + to->si_lower = ptr_to_compat(from->si_lower); > + to->si_upper = ptr_to_compat(from->si_upper); > break; > case SIL_FAULT_PKUERR: > - new.si_addr = ptr_to_compat(from->si_addr); > + to->si_addr = ptr_to_compat(from->si_addr); > #ifdef __ARCH_SI_TRAPNO > - new.si_trapno = from->si_trapno; > + to->si_trapno = from->si_trapno; > #endif > - new.si_pkey = from->si_pkey; > + to->si_pkey = from->si_pkey; > break; > case SIL_CHLD: > - new.si_pid = from->si_pid; > - new.si_uid = from->si_uid; > - new.si_status = from->si_status; > + to->si_pid = from->si_pid; > + to->si_uid = from->si_uid; > + to->si_status = from->si_status; > + to->si_utime = from->si_utime; > + to->si_stime = from->si_stime; > #ifdef CONFIG_X86_X32_ABI > if (x32_ABI) { > - new._sifields._sigchld_x32._utime = from->si_utime; > - new._sifields._sigchld_x32._stime = from->si_stime; > + to->_sifields._sigchld_x32._utime = from->si_utime; > + to->_sifields._sigchld_x32._stime = from->si_stime; > } else > #endif > { > - new.si_utime = from->si_utime; > - new.si_stime = from->si_stime; > } > break; > case SIL_RT: > - new.si_pid = from->si_pid; > - new.si_uid = from->si_uid; > - new.si_int = from->si_int; > + to->si_pid = from->si_pid; > + to->si_uid = from->si_uid; > + to->si_int = from->si_int; > break; > case SIL_SYS: > - new.si_call_addr = ptr_to_compat(from->si_call_addr); > - new.si_syscall = from->si_syscall; > - new.si_arch = from->si_arch; > + to->si_call_addr = ptr_to_compat(from->si_call_addr); > + to->si_syscall = from->si_syscall; > + to->si_arch = from->si_arch; > break; > } > +} > + > +int copy_siginfo_to_user32(struct compat_siginfo __user *to, > + const struct kernel_siginfo *from) > +#if defined(CONFIG_X86_X32_ABI) || defined(CONFIG_IA32_EMULATION) > +{ > + return __copy_siginfo_to_user32(to, from, in_x32_syscall()); > +} > +int __copy_siginfo_to_user32(struct compat_siginfo __user *to, > + const struct kernel_siginfo *from, bool x32_ABI) > +#endif > +{ > + struct compat_siginfo new; > + copy_siginfo_to_external32(&new, from); > +#ifdef CONFIG_X86_X32_ABI > + if (x32_ABI && from->si_signo == SIGCHLD) { > + new._sifields._sigchld_x32._utime = from->si_utime; > + new._sifields._sigchld_x32._stime = from->si_stime; > + } > +#endif > > if (copy_to_user(to, &new, sizeof(struct compat_siginfo))) > return -EFAULT; >
| |