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

Le 13/09/2021 à 17:54, Eric W. Biederman a écrit :

Christophe Leroy [off-list ref] writes:

quoted

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@infradead.org/T/ (local) ?

 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