Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()

[Eric W. Biederman]

Christophe Leroy <christophe.leroy@xxxxxxxxxx> writes:

> Le 02/09/2021 à 20:43, 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_user() in order to use it within user access blocks.
>>>
>>> For that, also add an 'unsafe' version of clear_user().
>>
>> Looking at your use cases you need the 32bit compat version of this
>> as well.
>>
>> The 32bit compat version is too complicated to become a macro, so I
>> don't think you can make this work correctly for the 32bit compat case.
>
> When looking into patch 5/5 that you nacked, I think you missed the fact that we
> keep using copy_siginfo_to_user32() as it for the 32 bit compat case.

I did.  My mistake.

However that mistake was so easy I think it mirrors the comments others
have made that this looks like a maintenance hazard.

Is improving the performance of 32bit kernels interesting?
Is improving the performance of 32bit compat support interesting?

If performance one or either of those cases is interesting it looks like
we already have copy_siginfo_to_external32 the factor you would need
to build unsafe_copy_siginfo_to_user32.

So I am not going to say impossible but please make something
maintainable.  I unified all of the compat 32bit siginfo logic because
it simply did not get enough love and attention when it was implemented
per architecture.

In general I think that concern applies to this case as well.  We really
need an implementation that shares as much burden as possible with other
architectures.

Eric


>> Probably-Not-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
>>
>> Eric
>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
>>> ---
>>>   include/linux/signal.h  | 15 +++++++++++++++
>>>   include/linux/uaccess.h |  1 +
>>>   kernel/signal.c         |  5 -----
>>>   3 files changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/linux/signal.h b/include/linux/signal.h
>>> index 3454c7ff0778..659bd43daf10 100644
>>> --- a/include/linux/signal.h
>>> +++ b/include/linux/signal.h
>>> @@ -35,6 +35,21 @@ static inline void copy_siginfo_to_external(siginfo_t *to,
>>>   int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from);
>>>   int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from);
>>>   +static __always_inline char __user *si_expansion(const siginfo_t __user
>>> *info)
>>> +{
>>> +	return ((char __user *)info) + sizeof(struct kernel_siginfo);
>>> +}
>>> +
>>> +#define unsafe_copy_siginfo_to_user(to, from, label) do {		\
>>> +	siginfo_t __user *__ucs_to = to;				\
>>> +	const kernel_siginfo_t *__ucs_from = from;			\
>>> +	char __user *__ucs_expansion = si_expansion(__ucs_to);		\
>>> +									\
>>> +	unsafe_copy_to_user(__ucs_to, __ucs_from,			\
>>> +			    sizeof(struct kernel_siginfo), label);	\
>>> +	unsafe_clear_user(__ucs_expansion, SI_EXPANSION_SIZE, label);	\
>>> +} while (0)
>>> +
>>>   enum siginfo_layout {
>>>   	SIL_KILL,
>>>   	SIL_TIMER,
>>> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
>>> index c05e903cef02..37073caac474 100644
>>> --- a/include/linux/uaccess.h
>>> +++ b/include/linux/uaccess.h
>>> @@ -398,6 +398,7 @@ long strnlen_user_nofault(const void __user *unsafe_addr, long count);
>>>   #define unsafe_put_user(x,p,e) unsafe_op_wrap(__put_user(x,p),e)
>>>   #define unsafe_copy_to_user(d,s,l,e) unsafe_op_wrap(__copy_to_user(d,s,l),e)
>>>   #define unsafe_copy_from_user(d,s,l,e) unsafe_op_wrap(__copy_from_user(d,s,l),e)
>>> +#define unsafe_clear_user(d, l, e) unsafe_op_wrap(__clear_user(d, l), e)
>>>   static inline unsigned long user_access_save(void) { return 0UL; }
>>>   static inline void user_access_restore(unsigned long flags) { }
>>>   #endif
>>> diff --git a/kernel/signal.c b/kernel/signal.c
>>> index a3229add4455..83b5971e4304 100644
>>> --- a/kernel/signal.c
>>> +++ b/kernel/signal.c
>>> @@ -3261,11 +3261,6 @@ enum siginfo_layout siginfo_layout(unsigned sig, int si_code)
>>>   	return layout;
>>>   }
>>>   -static inline char __user *si_expansion(const siginfo_t __user *info)
>>> -{
>>> -	return ((char __user *)info) + sizeof(struct kernel_siginfo);
>>> -}
>>> -
>>>   int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
>>>   {
>>>   	char __user *expansion = si_expansion(to);