Re: [PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()
From: Christophe Leroy
Date: Mon Sep 13 2021 - 13:01:25 EST
Le 13/09/2021 à 17:54, Eric W. Biederman a écrit :
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)
As far as I understood, it is forbidden to call functions within user
access blocks.
On powerpc it doesn't matter (yet), but as far as I understand x86 as a
tool called "objtool" to enforce that.
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.
Ah ? I didn't know that.
Maybe we can make as exception for memset(). Or we can hard-code a
zeroizing loop.
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?
See see
https://lore.kernel.org/lkml/20190318155142.025214872@xxxxxxxxxxxxx/T/ ?
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.
Yes that's what I did, make sure everything is inlined. Or maybe I
misunderstood something ?
Christophe