Re: [PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()

From: Eric W. Biederman
Date: Mon Sep 13 2021 - 11:55:04 EST

Next message: Joerg Roedel: "[PATCH v2 00/12] x86/sev: KEXEC/KDUMP support for SEV-ES guests"
Previous message: yongw . pur: "[PATCH v1] vmpressure: wake up work only when there is registration event"
In reply to: Christophe Leroy: "[PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()"
Next in thread: Christophe Leroy: "Re: [PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Christophe Leroy <christophe.leroy@xxxxxxxxxx> writes:

> In the same spirit as commit fb05121fd6a2 ("signal: Add
> unsafe_get_compat_sigset()"), implement an 'unsafe' version of
> copy_siginfo_to_user32() in order to use it within user access blocks.
>
> To do so, we need inline version of copy_siginfo_to_external32() as we
> don't want any function call inside user access blocks.

I don't understand. What is wrong with?

#define unsafe_copy_siginfo_to_user32(to, from, label) do { \
struct compat_siginfo __user *__ucs_to = to; \
const struct kernel_siginfo *__ucs_from = from; \
struct compat_siginfo __ucs_new; \
\
copy_siginfo_to_external32(&__ucs_new, __ucs_from); \
unsafe_copy_to_user(__ucs_to, &__ucs_new, \
sizeof(struct compat_siginfo), label); \
} while (0)

Your replacement of "memset(to, 0, sizeof(*to))" with
"struct compat_siginfo __ucs_new = {0}". is actively unsafe as the
compiler is free not to initialize any holes in the structure to 0 in
the later case.

Is there something about the unsafe macros that I am not aware of that
makes it improper to actually call C functions? Is that a requirement
for the instructions that change the user space access behavior?

>From the looks of this change all that you are doing is making it so
that all of copy_siginfo_to_external32 is being inlined. If that is not
a hard requirement of the instructions it seems like the wrong thing to
do here. copy_siginfo_to_external32 has not failures so it does not need
to be inlined so you can jump to the label.

Eric

>
> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> ---
> include/linux/compat.h | 83 +++++++++++++++++++++++++++++-
> include/linux/signal.h | 58 +++++++++++++++++++++
> kernel/signal.c | 114 +----------------------------------------
> 3 files changed, 141 insertions(+), 114 deletions(-)
>
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 8e0598c7d1d1..68823f4b86ee 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -412,6 +412,19 @@ int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
> #ifndef copy_siginfo_to_user32
> #define copy_siginfo_to_user32 __copy_siginfo_to_user32
> #endif
> +
> +#ifdef CONFIG_COMPAT
> +#define unsafe_copy_siginfo_to_user32(to, from, label) do { \
> + struct compat_siginfo __user *__ucs_to = to; \
> + const struct kernel_siginfo *__ucs_from = from; \
> + struct compat_siginfo __ucs_new = {0}; \
> + \
> + __copy_siginfo_to_external32(&__ucs_new, __ucs_from); \
> + unsafe_copy_to_user(__ucs_to, &__ucs_new, \
> + sizeof(struct compat_siginfo), label); \
> +} while (0)
> +#endif
> +
> int get_compat_sigevent(struct sigevent *event,
> const struct compat_sigevent __user *u_event);
>
> @@ -992,15 +1005,81 @@ static inline bool in_compat_syscall(void) { return false; }
> * appropriately converted them already.
> */
> #ifndef compat_ptr
> -static inline void __user *compat_ptr(compat_uptr_t uptr)
> +static __always_inline void __user *compat_ptr(compat_uptr_t uptr)
> {
> return (void __user *)(unsigned long)uptr;
> }
> #endif
>
> -static inline compat_uptr_t ptr_to_compat(void __user *uptr)
> +static __always_inline compat_uptr_t ptr_to_compat(void __user *uptr)
> {
> return (u32)(unsigned long)uptr;
> }
>
> +static __always_inline void
> +__copy_siginfo_to_external32(struct compat_siginfo *to,
> + const struct kernel_siginfo *from)
> +{
> + 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:
> + to->si_pid = from->si_pid;
> + to->si_uid = from->si_uid;
> + break;
> + case SIL_TIMER:
> + to->si_tid = from->si_tid;
> + to->si_overrun = from->si_overrun;
> + to->si_int = from->si_int;
> + break;
> + case SIL_POLL:
> + to->si_band = from->si_band;
> + to->si_fd = from->si_fd;
> + break;
> + case SIL_FAULT:
> + to->si_addr = ptr_to_compat(from->si_addr);
> + break;
> + case SIL_FAULT_TRAPNO:
> + to->si_addr = ptr_to_compat(from->si_addr);
> + to->si_trapno = from->si_trapno;
> + break;
> + case SIL_FAULT_MCEERR:
> + to->si_addr = ptr_to_compat(from->si_addr);
> + to->si_addr_lsb = from->si_addr_lsb;
> + break;
> + case SIL_FAULT_BNDERR:
> + to->si_addr = ptr_to_compat(from->si_addr);
> + to->si_lower = ptr_to_compat(from->si_lower);
> + to->si_upper = ptr_to_compat(from->si_upper);
> + break;
> + case SIL_FAULT_PKUERR:
> + to->si_addr = ptr_to_compat(from->si_addr);
> + to->si_pkey = from->si_pkey;
> + break;
> + case SIL_FAULT_PERF_EVENT:
> + to->si_addr = ptr_to_compat(from->si_addr);
> + to->si_perf_data = from->si_perf_data;
> + to->si_perf_type = from->si_perf_type;
> + break;
> + case SIL_CHLD:
> + 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;
> + break;
> + case SIL_RT:
> + to->si_pid = from->si_pid;
> + to->si_uid = from->si_uid;
> + to->si_int = from->si_int;
> + break;
> + case SIL_SYS:
> + to->si_call_addr = ptr_to_compat(from->si_call_addr);
> + to->si_syscall = from->si_syscall;
> + to->si_arch = from->si_arch;
> + break;
> + }
> +}
> +
> #endif /* _LINUX_COMPAT_H */
> diff --git a/include/linux/signal.h b/include/linux/signal.h
> index 70ea7e741427..637260bc193d 100644
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -65,6 +65,64 @@ enum siginfo_layout {
> SIL_SYS,
> };
>
> +static const struct {
> + unsigned char limit, layout;
> +} sig_sicodes[] = {
> + [SIGILL] = { NSIGILL, SIL_FAULT },
> + [SIGFPE] = { NSIGFPE, SIL_FAULT },
> + [SIGSEGV] = { NSIGSEGV, SIL_FAULT },
> + [SIGBUS] = { NSIGBUS, SIL_FAULT },
> + [SIGTRAP] = { NSIGTRAP, SIL_FAULT },
> +#if defined(SIGEMT)
> + [SIGEMT] = { NSIGEMT, SIL_FAULT },
> +#endif
> + [SIGCHLD] = { NSIGCHLD, SIL_CHLD },
> + [SIGPOLL] = { NSIGPOLL, SIL_POLL },
> + [SIGSYS] = { NSIGSYS, SIL_SYS },
> +};
> +
> +static __always_inline enum
> +siginfo_layout __siginfo_layout(unsigned sig, int si_code)
> +{
> + enum siginfo_layout layout = SIL_KILL;
> +
> + if ((si_code > SI_USER) && (si_code < SI_KERNEL)) {
> + if ((sig < ARRAY_SIZE(sig_sicodes)) &&
> + (si_code <= sig_sicodes[sig].limit)) {
> + layout = sig_sicodes[sig].layout;
> + /* Handle the exceptions */
> + if ((sig == SIGBUS) &&
> + (si_code >= BUS_MCEERR_AR) && (si_code <= BUS_MCEERR_AO))
> + layout = SIL_FAULT_MCEERR;
> + else if ((sig == SIGSEGV) && (si_code == SEGV_BNDERR))
> + layout = SIL_FAULT_BNDERR;
> +#ifdef SEGV_PKUERR
> + else if ((sig == SIGSEGV) && (si_code == SEGV_PKUERR))
> + layout = SIL_FAULT_PKUERR;
> +#endif
> + else if ((sig == SIGTRAP) && (si_code == TRAP_PERF))
> + layout = SIL_FAULT_PERF_EVENT;
> + else if (IS_ENABLED(CONFIG_SPARC) &&
> + (sig == SIGILL) && (si_code == ILL_ILLTRP))
> + layout = SIL_FAULT_TRAPNO;
> + else if (IS_ENABLED(CONFIG_ALPHA) &&
> + ((sig == SIGFPE) ||
> + ((sig == SIGTRAP) && (si_code == TRAP_UNK))))
> + layout = SIL_FAULT_TRAPNO;
> + }
> + else if (si_code <= NSIGPOLL)
> + layout = SIL_POLL;
> + } else {
> + if (si_code == SI_TIMER)
> + layout = SIL_TIMER;
> + else if (si_code == SI_SIGIO)
> + layout = SIL_POLL;
> + else if (si_code < 0)
> + layout = SIL_RT;
> + }
> + return layout;
> +}
> +
> enum siginfo_layout siginfo_layout(unsigned sig, int si_code);
>
> /*
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 23f168730b7e..0d402bdb174e 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3249,22 +3249,6 @@ COMPAT_SYSCALL_DEFINE2(rt_sigpending, compat_sigset_t __user *, uset,
> }
> #endif
>
> -static const struct {
> - unsigned char limit, layout;
> -} sig_sicodes[] = {
> - [SIGILL] = { NSIGILL, SIL_FAULT },
> - [SIGFPE] = { NSIGFPE, SIL_FAULT },
> - [SIGSEGV] = { NSIGSEGV, SIL_FAULT },
> - [SIGBUS] = { NSIGBUS, SIL_FAULT },
> - [SIGTRAP] = { NSIGTRAP, SIL_FAULT },
> -#if defined(SIGEMT)
> - [SIGEMT] = { NSIGEMT, SIL_FAULT },
> -#endif
> - [SIGCHLD] = { NSIGCHLD, SIL_CHLD },
> - [SIGPOLL] = { NSIGPOLL, SIL_POLL },
> - [SIGSYS] = { NSIGSYS, SIL_SYS },
> -};
> -
> static bool known_siginfo_layout(unsigned sig, int si_code)
> {
> if (si_code == SI_KERNEL)
> @@ -3286,42 +3270,7 @@ static bool known_siginfo_layout(unsigned sig, int si_code)
>
> enum siginfo_layout siginfo_layout(unsigned sig, int si_code)
> {
> - enum siginfo_layout layout = SIL_KILL;
> - if ((si_code > SI_USER) && (si_code < SI_KERNEL)) {
> - if ((sig < ARRAY_SIZE(sig_sicodes)) &&
> - (si_code <= sig_sicodes[sig].limit)) {
> - layout = sig_sicodes[sig].layout;
> - /* Handle the exceptions */
> - if ((sig == SIGBUS) &&
> - (si_code >= BUS_MCEERR_AR) && (si_code <= BUS_MCEERR_AO))
> - layout = SIL_FAULT_MCEERR;
> - else if ((sig == SIGSEGV) && (si_code == SEGV_BNDERR))
> - layout = SIL_FAULT_BNDERR;
> -#ifdef SEGV_PKUERR
> - else if ((sig == SIGSEGV) && (si_code == SEGV_PKUERR))
> - layout = SIL_FAULT_PKUERR;
> -#endif
> - else if ((sig == SIGTRAP) && (si_code == TRAP_PERF))
> - layout = SIL_FAULT_PERF_EVENT;
> - else if (IS_ENABLED(CONFIG_SPARC) &&
> - (sig == SIGILL) && (si_code == ILL_ILLTRP))
> - layout = SIL_FAULT_TRAPNO;
> - else if (IS_ENABLED(CONFIG_ALPHA) &&
> - ((sig == SIGFPE) ||
> - ((sig == SIGTRAP) && (si_code == TRAP_UNK))))
> - layout = SIL_FAULT_TRAPNO;
> - }
> - else if (si_code <= NSIGPOLL)
> - layout = SIL_POLL;
> - } else {
> - if (si_code == SI_TIMER)
> - layout = SIL_TIMER;
> - else if (si_code == SI_SIGIO)
> - layout = SIL_POLL;
> - else if (si_code < 0)
> - layout = SIL_RT;
> - }
> - return layout;
> + return __siginfo_layout(sig, si_code);
> }
>
> int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
> @@ -3389,66 +3338,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to,
> {
> memset(to, 0, sizeof(*to));
>
> - 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:
> - to->si_pid = from->si_pid;
> - to->si_uid = from->si_uid;
> - break;
> - case SIL_TIMER:
> - to->si_tid = from->si_tid;
> - to->si_overrun = from->si_overrun;
> - to->si_int = from->si_int;
> - break;
> - case SIL_POLL:
> - to->si_band = from->si_band;
> - to->si_fd = from->si_fd;
> - break;
> - case SIL_FAULT:
> - to->si_addr = ptr_to_compat(from->si_addr);
> - break;
> - case SIL_FAULT_TRAPNO:
> - to->si_addr = ptr_to_compat(from->si_addr);
> - to->si_trapno = from->si_trapno;
> - break;
> - case SIL_FAULT_MCEERR:
> - to->si_addr = ptr_to_compat(from->si_addr);
> - to->si_addr_lsb = from->si_addr_lsb;
> - break;
> - case SIL_FAULT_BNDERR:
> - to->si_addr = ptr_to_compat(from->si_addr);
> - to->si_lower = ptr_to_compat(from->si_lower);
> - to->si_upper = ptr_to_compat(from->si_upper);
> - break;
> - case SIL_FAULT_PKUERR:
> - to->si_addr = ptr_to_compat(from->si_addr);
> - to->si_pkey = from->si_pkey;
> - break;
> - case SIL_FAULT_PERF_EVENT:
> - to->si_addr = ptr_to_compat(from->si_addr);
> - to->si_perf_data = from->si_perf_data;
> - to->si_perf_type = from->si_perf_type;
> - break;
> - case SIL_CHLD:
> - 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;
> - break;
> - case SIL_RT:
> - to->si_pid = from->si_pid;
> - to->si_uid = from->si_uid;
> - to->si_int = from->si_int;
> - break;
> - case SIL_SYS:
> - to->si_call_addr = ptr_to_compat(from->si_call_addr);
> - to->si_syscall = from->si_syscall;
> - to->si_arch = from->si_arch;
> - break;
> - }
> + __copy_siginfo_to_external32(to, from);
> }
>
> int __copy_siginfo_to_user32(struct compat_siginfo __user *to,

Next message: Joerg Roedel: "[PATCH v2 00/12] x86/sev: KEXEC/KDUMP support for SEV-ES guests"
Previous message: yongw . pur: "[PATCH v1] vmpressure: wake up work only when there is registration event"
In reply to: Christophe Leroy: "[PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()"
Next in thread: Christophe Leroy: "Re: [PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]