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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 13/09/2021 à 17:54, Eric W. Biederman a écrit :
> 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)

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/ ?

> 
>  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