From: ebiederm@xmission.com (Eric W. Biederman)
To: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
Michael Ellerman <mpe@ellerman.id.au>,
hch@infradead.org, linux-kernel@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()
Date: Mon, 13 Sep 2021 10:54:35 -0500 [thread overview]
Message-ID: <87r1dspj2c.fsf@disp2133> (raw)
In-Reply-To: <e1b94e52688cd99ed4a3ab86170cd9ec48849291.1631537060.git.christophe.leroy@csgroup.eu> (Christophe Leroy's message of "Mon, 13 Sep 2021 17:19:08 +0200")
Christophe Leroy <christophe.leroy@csgroup.eu> 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@csgroup.eu>
> ---
> 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 prev parent reply other threads:[~2021-09-13 15:55 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-13 15:19 [PATCH RESEND v3 1/6] powerpc/signal64: Access function descriptor with user access block Christophe Leroy
2021-09-13 15:19 ` [PATCH RESEND v3 2/6] powerpc/signal: Include the new stack frame inside the " Christophe Leroy
2021-09-13 15:19 ` [PATCH RESEND v3 3/6] signal: Add unsafe_copy_siginfo_to_user() Christophe Leroy
2021-09-13 15:19 ` [PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32() Christophe Leroy
2021-09-13 15:54 ` Eric W. Biederman [this message]
2021-09-13 17:01 ` Christophe Leroy
2021-09-13 15:19 ` [PATCH RESEND v3 5/6] powerpc/uaccess: Add unsafe_clear_user() Christophe Leroy
2021-09-13 15:19 ` [PATCH RESEND v3 6/6] powerpc/signal: Use unsafe_copy_siginfo_to_user() Christophe Leroy
2021-09-13 15:57 ` Eric W. Biederman
2021-09-13 16:21 ` Eric W. Biederman
2021-09-13 17:19 ` Christophe Leroy
2021-09-13 19:11 ` Eric W. Biederman
2021-09-14 14:00 ` Christophe Leroy
2021-09-13 17:14 ` Christophe Leroy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r1dspj2c.fsf@disp2133 \
--to=ebiederm@xmission.com \
--cc=benh@kernel.crashing.org \
--cc=christophe.leroy@csgroup.eu \
--cc=hch@infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=paulus@samba.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).