[PATCH] powerpc/signal64: Copy siginfo before changing regs->nip

[PATCH] powerpc/signal64: Copy siginfo before changing regs->nip

[Michael Ellerman]

In commit 96d7a4e06fab ("powerpc/signal64: Rewrite handle_rt_signal64()
to minimise uaccess switches") the 64-bit signal code was rearranged to
use user_write_access_begin/end().

As part of that change the call to copy_siginfo_to_user() was moved
later in the function, so that it could be done after the
user_write_access_end().

In particular it was moved after we modify regs->nip to point to the
signal trampoline. That means if copy_siginfo_to_user() fails we exit
handle_rt_signal64() with an error but with regs->nip modified, whereas
previously we would not modify regs->nip until the copy succeeded.

Returning an error from signal delivery but with regs->nip updated
leaves the process in a sort of half-delivered state. We do immediately
force a SEGV in signal_setup_done(), called from do_signal(), so the
process should never run in the half-delivered state.

However that SEGV is not delivered until we've gone around to
do_notify_resume() again, so it's possible some tracing could observe
the half-delivered state.

There are other cases where we fail signal delivery with regs partly
updated, eg. the write to newsp and SA_SIGINFO, but the latter at least
is very unlikely to fail as it reads back from the frame we just wrote
to.

Looking at other arches they seem to be more careful about leaving regs
unchanged until the copy operations have succeeded, and in general that
seems like good hygenie.

So although the current behaviour is not cleary buggy, it's also not
clearly correct. So move the call to copy_siginfo_to_user() up prior to
the modification of regs->nip, which is closer to the old behaviour, and
easier to reason about.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/signal_64.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index dca66481d0c2..f9e1f5428b9e 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -902,6 +902,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
 	user_write_access_end();
 
+	/* Save the siginfo outside of the unsafe block. */
+	if (copy_siginfo_to_user(&frame->info, &ksig->info))
+		goto badframe;
+
 	/* Make sure signal handler doesn't get spurious FP exceptions */
 	tsk->thread.fp_state.fpscr = 0;
 
@@ -915,11 +919,6 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 		regs->nip = (unsigned long) &frame->tramp[0];
 	}
 
-
-	/* Save the siginfo outside of the unsafe block. */
-	if (copy_siginfo_to_user(&frame->info, &ksig->info))
-		goto badframe;
-
 	/* Allocate a dummy caller frame for the signal handler. */
 	newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
 	err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);
-- 
2.25.1

Re: [PATCH] powerpc/signal64: Copy siginfo before changing regs->nip

[Nicholas Piggin]

Excerpts from Michael Ellerman's message of June 8, 2021 11:46 pm:
> In commit 96d7a4e06fab ("powerpc/signal64: Rewrite handle_rt_signal64()
> to minimise uaccess switches") the 64-bit signal code was rearranged to
> use user_write_access_begin/end().
> 
> As part of that change the call to copy_siginfo_to_user() was moved
> later in the function, so that it could be done after the
> user_write_access_end().
> 
> In particular it was moved after we modify regs->nip to point to the
> signal trampoline. That means if copy_siginfo_to_user() fails we exit
> handle_rt_signal64() with an error but with regs->nip modified, whereas
> previously we would not modify regs->nip until the copy succeeded.
> 
> Returning an error from signal delivery but with regs->nip updated
> leaves the process in a sort of half-delivered state. We do immediately
> force a SEGV in signal_setup_done(), called from do_signal(), so the
> process should never run in the half-delivered state.
> 
> However that SEGV is not delivered until we've gone around to
> do_notify_resume() again, so it's possible some tracing could observe
> the half-delivered state.
> 
> There are other cases where we fail signal delivery with regs partly
> updated, eg. the write to newsp and SA_SIGINFO, but the latter at least
> is very unlikely to fail as it reads back from the frame we just wrote
> to.
> 
> Looking at other arches they seem to be more careful about leaving regs
> unchanged until the copy operations have succeeded, and in general that
> seems like good hygenie.
> 
> So although the current behaviour is not cleary buggy, it's also not
> clearly correct. So move the call to copy_siginfo_to_user() up prior to
> the modification of regs->nip, which is closer to the old behaviour, and
> easier to reason about.

Good catch, should it still have a Fixes: tag though? Even if it's not
clearly buggy we want it to be patched.

Thanks,
Nick

> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/kernel/signal_64.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index dca66481d0c2..f9e1f5428b9e 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -902,6 +902,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
>  	user_write_access_end();
>  
> +	/* Save the siginfo outside of the unsafe block. */
> +	if (copy_siginfo_to_user(&frame->info, &ksig->info))
> +		goto badframe;
> +
>  	/* Make sure signal handler doesn't get spurious FP exceptions */
>  	tsk->thread.fp_state.fpscr = 0;
>  
> @@ -915,11 +919,6 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  		regs->nip = (unsigned long) &frame->tramp[0];
>  	}
>  
> -
> -	/* Save the siginfo outside of the unsafe block. */
> -	if (copy_siginfo_to_user(&frame->info, &ksig->info))
> -		goto badframe;
> -
>  	/* Allocate a dummy caller frame for the signal handler. */
>  	newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
>  	err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);
> -- 
> 2.25.1
> 
> 

Re: [PATCH] powerpc/signal64: Copy siginfo before changing regs->nip

[Nicholas Piggin]

Excerpts from Nicholas Piggin's message of June 14, 2021 10:47 am:
> Excerpts from Michael Ellerman's message of June 8, 2021 11:46 pm:
>> In commit 96d7a4e06fab ("powerpc/signal64: Rewrite handle_rt_signal64()
>> to minimise uaccess switches") the 64-bit signal code was rearranged to
>> use user_write_access_begin/end().
>> 
>> As part of that change the call to copy_siginfo_to_user() was moved
>> later in the function, so that it could be done after the
>> user_write_access_end().
>> 
>> In particular it was moved after we modify regs->nip to point to the
>> signal trampoline. That means if copy_siginfo_to_user() fails we exit
>> handle_rt_signal64() with an error but with regs->nip modified, whereas
>> previously we would not modify regs->nip until the copy succeeded.
>> 
>> Returning an error from signal delivery but with regs->nip updated
>> leaves the process in a sort of half-delivered state. We do immediately
>> force a SEGV in signal_setup_done(), called from do_signal(), so the
>> process should never run in the half-delivered state.
>> 
>> However that SEGV is not delivered until we've gone around to
>> do_notify_resume() again, so it's possible some tracing could observe
>> the half-delivered state.
>> 
>> There are other cases where we fail signal delivery with regs partly
>> updated, eg. the write to newsp and SA_SIGINFO, but the latter at least
>> is very unlikely to fail as it reads back from the frame we just wrote
>> to.
>> 
>> Looking at other arches they seem to be more careful about leaving regs
>> unchanged until the copy operations have succeeded, and in general that
>> seems like good hygenie.
>> 
>> So although the current behaviour is not cleary buggy, it's also not
>> clearly correct. So move the call to copy_siginfo_to_user() up prior to
>> the modification of regs->nip, which is closer to the old behaviour, and
>> easier to reason about.
> 
> Good catch, should it still have a Fixes: tag though? Even if it's not
> clearly buggy we want it to be patched.

Also...

>> 
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>>  arch/powerpc/kernel/signal_64.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
>> index dca66481d0c2..f9e1f5428b9e 100644
>> --- a/arch/powerpc/kernel/signal_64.c
>> +++ b/arch/powerpc/kernel/signal_64.c
>> @@ -902,6 +902,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>  	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
>>  	user_write_access_end();
>>  
>> +	/* Save the siginfo outside of the unsafe block. */
>> +	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>> +		goto badframe;
>> +
>>  	/* Make sure signal handler doesn't get spurious FP exceptions */
>>  	tsk->thread.fp_state.fpscr = 0;
>>  
>> @@ -915,11 +919,6 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>  		regs->nip = (unsigned long) &frame->tramp[0];
>>  	}
>>  
>> -
>> -	/* Save the siginfo outside of the unsafe block. */
>> -	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>> -		goto badframe;
>> -
>>  	/* Allocate a dummy caller frame for the signal handler. */
>>  	newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
>>  	err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);

Does the same reasoning apply to this one and the ELF V1 function
descriptor thing? It seems like you could move all of that block
up instead. With your other SA_SIGINFO get_user patch, there would
then be no possibility of error after you start modifying regs.

Thanks,
Nick

Re: [PATCH] powerpc/signal64: Copy siginfo before changing regs->nip

[Christophe Leroy]

Le 14/06/2021 à 03:29, Nicholas Piggin a écrit :
> Excerpts from Nicholas Piggin's message of June 14, 2021 10:47 am:
>> Excerpts from Michael Ellerman's message of June 8, 2021 11:46 pm:
>>> In commit 96d7a4e06fab ("powerpc/signal64: Rewrite handle_rt_signal64()
>>> to minimise uaccess switches") the 64-bit signal code was rearranged to
>>> use user_write_access_begin/end().
>>>
>>> As part of that change the call to copy_siginfo_to_user() was moved
>>> later in the function, so that it could be done after the
>>> user_write_access_end().
>>>
>>> In particular it was moved after we modify regs->nip to point to the
>>> signal trampoline. That means if copy_siginfo_to_user() fails we exit
>>> handle_rt_signal64() with an error but with regs->nip modified, whereas
>>> previously we would not modify regs->nip until the copy succeeded.
>>>
>>> Returning an error from signal delivery but with regs->nip updated
>>> leaves the process in a sort of half-delivered state. We do immediately
>>> force a SEGV in signal_setup_done(), called from do_signal(), so the
>>> process should never run in the half-delivered state.
>>>
>>> However that SEGV is not delivered until we've gone around to
>>> do_notify_resume() again, so it's possible some tracing could observe
>>> the half-delivered state.
>>>
>>> There are other cases where we fail signal delivery with regs partly
>>> updated, eg. the write to newsp and SA_SIGINFO, but the latter at least
>>> is very unlikely to fail as it reads back from the frame we just wrote
>>> to.
>>>
>>> Looking at other arches they seem to be more careful about leaving regs
>>> unchanged until the copy operations have succeeded, and in general that
>>> seems like good hygenie.
>>>
>>> So although the current behaviour is not cleary buggy, it's also not
>>> clearly correct. So move the call to copy_siginfo_to_user() up prior to
>>> the modification of regs->nip, which is closer to the old behaviour, and
>>> easier to reason about.
>>
>> Good catch, should it still have a Fixes: tag though? Even if it's not
>> clearly buggy we want it to be patched.
> 
> Also...
> 
>>>
>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>> ---
>>>   arch/powerpc/kernel/signal_64.c | 9 ++++-----
>>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
>>> index dca66481d0c2..f9e1f5428b9e 100644
>>> --- a/arch/powerpc/kernel/signal_64.c
>>> +++ b/arch/powerpc/kernel/signal_64.c
>>> @@ -902,6 +902,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>>   	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
>>>   	user_write_access_end();
>>>   
>>> +	/* Save the siginfo outside of the unsafe block. */
>>> +	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>>> +		goto badframe;
>>> +
>>>   	/* Make sure signal handler doesn't get spurious FP exceptions */
>>>   	tsk->thread.fp_state.fpscr = 0;
>>>   
>>> @@ -915,11 +919,6 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>>   		regs->nip = (unsigned long) &frame->tramp[0];
>>>   	}
>>>   
>>> -
>>> -	/* Save the siginfo outside of the unsafe block. */
>>> -	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>>> -		goto badframe;
>>> -
>>>   	/* Allocate a dummy caller frame for the signal handler. */
>>>   	newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
>>>   	err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);
> 
> Does the same reasoning apply to this one and the ELF V1 function
> descriptor thing? It seems like you could move all of that block
> up instead. With your other SA_SIGINFO get_user patch, there would
> then be no possibility of error after you start modifying regs.
> 

To move the above in the user access block, we need to open a larger window. At the time being the 
window opened only contains the 'frame'. 'newsp' points before the 'frame'.

Re: [PATCH] powerpc/signal64: Copy siginfo before changing regs->nip

[Nicholas Piggin]

Excerpts from Christophe Leroy's message of June 14, 2021 3:31 pm:
> 
> 
> Le 14/06/2021 à 03:29, Nicholas Piggin a écrit :
>> Excerpts from Nicholas Piggin's message of June 14, 2021 10:47 am:
>>> Excerpts from Michael Ellerman's message of June 8, 2021 11:46 pm:
>>>> In commit 96d7a4e06fab ("powerpc/signal64: Rewrite handle_rt_signal64()
>>>> to minimise uaccess switches") the 64-bit signal code was rearranged to
>>>> use user_write_access_begin/end().
>>>>
>>>> As part of that change the call to copy_siginfo_to_user() was moved
>>>> later in the function, so that it could be done after the
>>>> user_write_access_end().
>>>>
>>>> In particular it was moved after we modify regs->nip to point to the
>>>> signal trampoline. That means if copy_siginfo_to_user() fails we exit
>>>> handle_rt_signal64() with an error but with regs->nip modified, whereas
>>>> previously we would not modify regs->nip until the copy succeeded.
>>>>
>>>> Returning an error from signal delivery but with regs->nip updated
>>>> leaves the process in a sort of half-delivered state. We do immediately
>>>> force a SEGV in signal_setup_done(), called from do_signal(), so the
>>>> process should never run in the half-delivered state.
>>>>
>>>> However that SEGV is not delivered until we've gone around to
>>>> do_notify_resume() again, so it's possible some tracing could observe
>>>> the half-delivered state.
>>>>
>>>> There are other cases where we fail signal delivery with regs partly
>>>> updated, eg. the write to newsp and SA_SIGINFO, but the latter at least
>>>> is very unlikely to fail as it reads back from the frame we just wrote
>>>> to.
>>>>
>>>> Looking at other arches they seem to be more careful about leaving regs
>>>> unchanged until the copy operations have succeeded, and in general that
>>>> seems like good hygenie.
>>>>
>>>> So although the current behaviour is not cleary buggy, it's also not
>>>> clearly correct. So move the call to copy_siginfo_to_user() up prior to
>>>> the modification of regs->nip, which is closer to the old behaviour, and
>>>> easier to reason about.
>>>
>>> Good catch, should it still have a Fixes: tag though? Even if it's not
>>> clearly buggy we want it to be patched.
>> 
>> Also...
>> 
>>>>
>>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>> ---
>>>>   arch/powerpc/kernel/signal_64.c | 9 ++++-----
>>>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
>>>> index dca66481d0c2..f9e1f5428b9e 100644
>>>> --- a/arch/powerpc/kernel/signal_64.c
>>>> +++ b/arch/powerpc/kernel/signal_64.c
>>>> @@ -902,6 +902,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>>>   	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
>>>>   	user_write_access_end();
>>>>   
>>>> +	/* Save the siginfo outside of the unsafe block. */
>>>> +	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>>>> +		goto badframe;
>>>> +
>>>>   	/* Make sure signal handler doesn't get spurious FP exceptions */
>>>>   	tsk->thread.fp_state.fpscr = 0;
>>>>   
>>>> @@ -915,11 +919,6 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>>>   		regs->nip = (unsigned long) &frame->tramp[0];
>>>>   	}
>>>>   
>>>> -
>>>> -	/* Save the siginfo outside of the unsafe block. */
>>>> -	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>>>> -		goto badframe;
>>>> -
>>>>   	/* Allocate a dummy caller frame for the signal handler. */
>>>>   	newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
>>>>   	err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);
>> 
>> Does the same reasoning apply to this one and the ELF V1 function
>> descriptor thing? It seems like you could move all of that block
>> up instead. With your other SA_SIGINFO get_user patch, there would
>> then be no possibility of error after you start modifying regs.
>> 
> 
> To move the above in the user access block, we need to open a larger window. At the time being the 
> window opened only contains the 'frame'. 'newsp' points before the 'frame'.
> 

Only by 64/128 bytes though. Is that a problem? Not for 64s. Could it 
cause more overhead than it saves on other platforms?

For protection, it looks like all the important control data is in the 
signal frame anyway, this frame is just for stack unwinding?

Thanks,
Nick

Re: [PATCH] powerpc/signal64: Copy siginfo before changing regs->nip

[Christophe Leroy]

Le 14/06/2021 à 07:55, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of June 14, 2021 3:31 pm:
>>
>>
>> Le 14/06/2021 à 03:29, Nicholas Piggin a écrit :
>>> Excerpts from Nicholas Piggin's message of June 14, 2021 10:47 am:
>>>> Excerpts from Michael Ellerman's message of June 8, 2021 11:46 pm:
>>>>> In commit 96d7a4e06fab ("powerpc/signal64: Rewrite handle_rt_signal64()
>>>>> to minimise uaccess switches") the 64-bit signal code was rearranged to
>>>>> use user_write_access_begin/end().
>>>>>
>>>>> As part of that change the call to copy_siginfo_to_user() was moved
>>>>> later in the function, so that it could be done after the
>>>>> user_write_access_end().
>>>>>
>>>>> In particular it was moved after we modify regs->nip to point to the
>>>>> signal trampoline. That means if copy_siginfo_to_user() fails we exit
>>>>> handle_rt_signal64() with an error but with regs->nip modified, whereas
>>>>> previously we would not modify regs->nip until the copy succeeded.
>>>>>
>>>>> Returning an error from signal delivery but with regs->nip updated
>>>>> leaves the process in a sort of half-delivered state. We do immediately
>>>>> force a SEGV in signal_setup_done(), called from do_signal(), so the
>>>>> process should never run in the half-delivered state.
>>>>>
>>>>> However that SEGV is not delivered until we've gone around to
>>>>> do_notify_resume() again, so it's possible some tracing could observe
>>>>> the half-delivered state.
>>>>>
>>>>> There are other cases where we fail signal delivery with regs partly
>>>>> updated, eg. the write to newsp and SA_SIGINFO, but the latter at least
>>>>> is very unlikely to fail as it reads back from the frame we just wrote
>>>>> to.
>>>>>
>>>>> Looking at other arches they seem to be more careful about leaving regs
>>>>> unchanged until the copy operations have succeeded, and in general that
>>>>> seems like good hygenie.
>>>>>
>>>>> So although the current behaviour is not cleary buggy, it's also not
>>>>> clearly correct. So move the call to copy_siginfo_to_user() up prior to
>>>>> the modification of regs->nip, which is closer to the old behaviour, and
>>>>> easier to reason about.
>>>>
>>>> Good catch, should it still have a Fixes: tag though? Even if it's not
>>>> clearly buggy we want it to be patched.
>>>
>>> Also...
>>>
>>>>>
>>>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>>> ---
>>>>>    arch/powerpc/kernel/signal_64.c | 9 ++++-----
>>>>>    1 file changed, 4 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
>>>>> index dca66481d0c2..f9e1f5428b9e 100644
>>>>> --- a/arch/powerpc/kernel/signal_64.c
>>>>> +++ b/arch/powerpc/kernel/signal_64.c
>>>>> @@ -902,6 +902,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>>>>    	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
>>>>>    	user_write_access_end();
>>>>>    
>>>>> +	/* Save the siginfo outside of the unsafe block. */
>>>>> +	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>>>>> +		goto badframe;
>>>>> +
>>>>>    	/* Make sure signal handler doesn't get spurious FP exceptions */
>>>>>    	tsk->thread.fp_state.fpscr = 0;
>>>>>    
>>>>> @@ -915,11 +919,6 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>>>>    		regs->nip = (unsigned long) &frame->tramp[0];
>>>>>    	}
>>>>>    
>>>>> -
>>>>> -	/* Save the siginfo outside of the unsafe block. */
>>>>> -	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>>>>> -		goto badframe;
>>>>> -
>>>>>    	/* Allocate a dummy caller frame for the signal handler. */
>>>>>    	newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
>>>>>    	err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);
>>>
>>> Does the same reasoning apply to this one and the ELF V1 function
>>> descriptor thing? It seems like you could move all of that block
>>> up instead. With your other SA_SIGINFO get_user patch, there would
>>> then be no possibility of error after you start modifying regs.
>>>
>>
>> To move the above in the user access block, we need to open a larger window. At the time being the
>> window opened only contains the 'frame'. 'newsp' points before the 'frame'.
>>
> 
> Only by 64/128 bytes though. Is that a problem? Not for 64s. Could it
> cause more overhead than it saves on other platforms?

No it is not a problem at all, just need to not be forgotten, on ppc64 it may go unnoticed, on 32s 
it will blew up if we forget to enlarge the access window and the access involves a different 256M 
segment (Very unlikely for sure but ...)

> 
> For protection, it looks like all the important control data is in the
> signal frame anyway, this frame is just for stack unwinding?

That's my understanding as well.

Christophe

Re: [PATCH] powerpc/signal64: Copy siginfo before changing regs->nip

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Michael Ellerman's message of June 8, 2021 11:46 pm:
>> In commit 96d7a4e06fab ("powerpc/signal64: Rewrite handle_rt_signal64()
>> to minimise uaccess switches") the 64-bit signal code was rearranged to
>> use user_write_access_begin/end().
>> 
>> As part of that change the call to copy_siginfo_to_user() was moved
>> later in the function, so that it could be done after the
>> user_write_access_end().
>> 
>> In particular it was moved after we modify regs->nip to point to the
>> signal trampoline. That means if copy_siginfo_to_user() fails we exit
>> handle_rt_signal64() with an error but with regs->nip modified, whereas
>> previously we would not modify regs->nip until the copy succeeded.
>> 
>> Returning an error from signal delivery but with regs->nip updated
>> leaves the process in a sort of half-delivered state. We do immediately
>> force a SEGV in signal_setup_done(), called from do_signal(), so the
>> process should never run in the half-delivered state.
>> 
>> However that SEGV is not delivered until we've gone around to
>> do_notify_resume() again, so it's possible some tracing could observe
>> the half-delivered state.
>> 
>> There are other cases where we fail signal delivery with regs partly
>> updated, eg. the write to newsp and SA_SIGINFO, but the latter at least
>> is very unlikely to fail as it reads back from the frame we just wrote
>> to.
>> 
>> Looking at other arches they seem to be more careful about leaving regs
>> unchanged until the copy operations have succeeded, and in general that
>> seems like good hygenie.
>> 
>> So although the current behaviour is not cleary buggy, it's also not
>> clearly correct. So move the call to copy_siginfo_to_user() up prior to
>> the modification of regs->nip, which is closer to the old behaviour, and
>> easier to reason about.
>
> Good catch, should it still have a Fixes: tag though? Even if it's not
> clearly buggy we want it to be patched.

Yeah I'll add one.

cheers

Re: [PATCH] powerpc/signal64: Copy siginfo before changing regs->nip

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Nicholas Piggin's message of June 14, 2021 10:47 am:
>> Excerpts from Michael Ellerman's message of June 8, 2021 11:46 pm:
>>> In commit 96d7a4e06fab ("powerpc/signal64: Rewrite handle_rt_signal64()
>>> to minimise uaccess switches") the 64-bit signal code was rearranged to
>>> use user_write_access_begin/end().
>>> 
>>> As part of that change the call to copy_siginfo_to_user() was moved
>>> later in the function, so that it could be done after the
>>> user_write_access_end().
>>> 
>>> In particular it was moved after we modify regs->nip to point to the
>>> signal trampoline. That means if copy_siginfo_to_user() fails we exit
>>> handle_rt_signal64() with an error but with regs->nip modified, whereas
>>> previously we would not modify regs->nip until the copy succeeded.
>>> 
>>> Returning an error from signal delivery but with regs->nip updated
>>> leaves the process in a sort of half-delivered state. We do immediately
>>> force a SEGV in signal_setup_done(), called from do_signal(), so the
>>> process should never run in the half-delivered state.
>>> 
>>> However that SEGV is not delivered until we've gone around to
>>> do_notify_resume() again, so it's possible some tracing could observe
>>> the half-delivered state.
>>> 
>>> There are other cases where we fail signal delivery with regs partly
>>> updated, eg. the write to newsp and SA_SIGINFO, but the latter at least
>>> is very unlikely to fail as it reads back from the frame we just wrote
>>> to.
>>> 
>>> Looking at other arches they seem to be more careful about leaving regs
>>> unchanged until the copy operations have succeeded, and in general that
>>> seems like good hygenie.
>>> 
>>> So although the current behaviour is not cleary buggy, it's also not
>>> clearly correct. So move the call to copy_siginfo_to_user() up prior to
>>> the modification of regs->nip, which is closer to the old behaviour, and
>>> easier to reason about.
>> 
>> Good catch, should it still have a Fixes: tag though? Even if it's not
>> clearly buggy we want it to be patched.
>
> Also...
>
>>> 
>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>> ---
>>>  arch/powerpc/kernel/signal_64.c | 9 ++++-----
>>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
>>> index dca66481d0c2..f9e1f5428b9e 100644
>>> --- a/arch/powerpc/kernel/signal_64.c
>>> +++ b/arch/powerpc/kernel/signal_64.c
>>> @@ -902,6 +902,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>>  	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
>>>  	user_write_access_end();
>>>  
>>> +	/* Save the siginfo outside of the unsafe block. */
>>> +	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>>> +		goto badframe;
>>> +
>>>  	/* Make sure signal handler doesn't get spurious FP exceptions */
>>>  	tsk->thread.fp_state.fpscr = 0;
>>>  
>>> @@ -915,11 +919,6 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>>  		regs->nip = (unsigned long) &frame->tramp[0];
>>>  	}
>>>  
>>> -
>>> -	/* Save the siginfo outside of the unsafe block. */
>>> -	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>>> -		goto badframe;
>>> -
>>>  	/* Allocate a dummy caller frame for the signal handler. */
>>>  	newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
>>>  	err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);
>
> Does the same reasoning apply to this one and the ELF V1 function
> descriptor thing? It seems like you could move all of that block
> up instead. With your other SA_SIGINFO get_user patch, there would
> then be no possibility of error after you start modifying regs.

Yeah I think we should rework it further and eventually get to the point
were we leave regs untouched until we're guaranteed to return success.

It will need a bit more work though because of copy_siginfo_to_user().

cheers

Re: [PATCH] powerpc/signal64: Copy siginfo before changing regs->nip

[Nicholas Piggin]

Excerpts from Christophe Leroy's message of June 14, 2021 5:22 pm:
> 
> 
> Le 14/06/2021 à 07:55, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of June 14, 2021 3:31 pm:
>>>
>>>
>>> Le 14/06/2021 à 03:29, Nicholas Piggin a écrit :
>>>> Excerpts from Nicholas Piggin's message of June 14, 2021 10:47 am:
>>>>> Excerpts from Michael Ellerman's message of June 8, 2021 11:46 pm:
>>>>>> In commit 96d7a4e06fab ("powerpc/signal64: Rewrite handle_rt_signal64()
>>>>>> to minimise uaccess switches") the 64-bit signal code was rearranged to
>>>>>> use user_write_access_begin/end().
>>>>>>
>>>>>> As part of that change the call to copy_siginfo_to_user() was moved
>>>>>> later in the function, so that it could be done after the
>>>>>> user_write_access_end().
>>>>>>
>>>>>> In particular it was moved after we modify regs->nip to point to the
>>>>>> signal trampoline. That means if copy_siginfo_to_user() fails we exit
>>>>>> handle_rt_signal64() with an error but with regs->nip modified, whereas
>>>>>> previously we would not modify regs->nip until the copy succeeded.
>>>>>>
>>>>>> Returning an error from signal delivery but with regs->nip updated
>>>>>> leaves the process in a sort of half-delivered state. We do immediately
>>>>>> force a SEGV in signal_setup_done(), called from do_signal(), so the
>>>>>> process should never run in the half-delivered state.
>>>>>>
>>>>>> However that SEGV is not delivered until we've gone around to
>>>>>> do_notify_resume() again, so it's possible some tracing could observe
>>>>>> the half-delivered state.
>>>>>>
>>>>>> There are other cases where we fail signal delivery with regs partly
>>>>>> updated, eg. the write to newsp and SA_SIGINFO, but the latter at least
>>>>>> is very unlikely to fail as it reads back from the frame we just wrote
>>>>>> to.
>>>>>>
>>>>>> Looking at other arches they seem to be more careful about leaving regs
>>>>>> unchanged until the copy operations have succeeded, and in general that
>>>>>> seems like good hygenie.
>>>>>>
>>>>>> So although the current behaviour is not cleary buggy, it's also not
>>>>>> clearly correct. So move the call to copy_siginfo_to_user() up prior to
>>>>>> the modification of regs->nip, which is closer to the old behaviour, and
>>>>>> easier to reason about.
>>>>>
>>>>> Good catch, should it still have a Fixes: tag though? Even if it's not
>>>>> clearly buggy we want it to be patched.
>>>>
>>>> Also...
>>>>
>>>>>>
>>>>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>>>> ---
>>>>>>    arch/powerpc/kernel/signal_64.c | 9 ++++-----
>>>>>>    1 file changed, 4 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
>>>>>> index dca66481d0c2..f9e1f5428b9e 100644
>>>>>> --- a/arch/powerpc/kernel/signal_64.c
>>>>>> +++ b/arch/powerpc/kernel/signal_64.c
>>>>>> @@ -902,6 +902,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>>>>>    	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
>>>>>>    	user_write_access_end();
>>>>>>    
>>>>>> +	/* Save the siginfo outside of the unsafe block. */
>>>>>> +	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>>>>>> +		goto badframe;
>>>>>> +
>>>>>>    	/* Make sure signal handler doesn't get spurious FP exceptions */
>>>>>>    	tsk->thread.fp_state.fpscr = 0;
>>>>>>    
>>>>>> @@ -915,11 +919,6 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>>>>>    		regs->nip = (unsigned long) &frame->tramp[0];
>>>>>>    	}
>>>>>>    
>>>>>> -
>>>>>> -	/* Save the siginfo outside of the unsafe block. */
>>>>>> -	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>>>>>> -		goto badframe;
>>>>>> -
>>>>>>    	/* Allocate a dummy caller frame for the signal handler. */
>>>>>>    	newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
>>>>>>    	err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);
>>>>
>>>> Does the same reasoning apply to this one and the ELF V1 function
>>>> descriptor thing? It seems like you could move all of that block
>>>> up instead. With your other SA_SIGINFO get_user patch, there would
>>>> then be no possibility of error after you start modifying regs.
>>>>
>>>
>>> To move the above in the user access block, we need to open a larger window. At the time being the
>>> window opened only contains the 'frame'. 'newsp' points before the 'frame'.
>>>
>> 
>> Only by 64/128 bytes though. Is that a problem? Not for 64s. Could it
>> cause more overhead than it saves on other platforms?
> 
> No it is not a problem at all, just need to not be forgotten, on ppc64 it may go unnoticed, on 32s 
> it will blew up if we forget to enlarge the access window and the access involves a different 256M 
> segment (Very unlikely for sure but ...)

Okay, and it's a good point. Would be nice if there was some sanitizer 
that could check this to byte granularity.

Thanks,
Nick

>> For protection, it looks like all the important control data is in the
>> signal frame anyway, this frame is just for stack unwinding?
> 
> That's my understanding as well.

Re: [PATCH] powerpc/signal64: Copy siginfo before changing regs->nip

[Michael Ellerman]

On Tue, 8 Jun 2021 23:46:05 +1000, Michael Ellerman wrote:
> In commit 96d7a4e06fab ("powerpc/signal64: Rewrite handle_rt_signal64()
> to minimise uaccess switches") the 64-bit signal code was rearranged to
> use user_write_access_begin/end().
> 
> As part of that change the call to copy_siginfo_to_user() was moved
> later in the function, so that it could be done after the
> user_write_access_end().
> 
> [...]

Applied to powerpc/fixes.

[1/1] powerpc/signal64: Copy siginfo before changing regs->nip
      https://git.kernel.org/powerpc/c/e41d6c3f4f9b4804e53ca87aba8ee11ada606c77

cheers