[REVIEW,PATCHv2,03/26] signal/arm64: Use force_sig not force_sig_fault for SIGKILL

Submitted by Eric W. Biederman on May 23, 2019, 4:11 p.m.

Details

Message ID 875zq1gnh4.fsf_-_@xmission.com
State New
Series "signal: Remove task argument from force_sig_info"
Headers show

Commit Message

Eric W. Biederman May 23, 2019, 4:11 p.m.
I don't think this is userspace visible but SIGKILL does not have
any si_codes that use the fault member of the siginfo union.  Correct
this the simple way and call force_sig instead of force_sig_fault when
the signal is SIGKILL.

The two know places where synchronous SIGKILL are generated are
do_bad_area and fpsimd_save.  The call paths to force_sig_fault are:
do_bad_area
  arm64_force_sig_fault
    force_sig_fault
force_signal_inject
  arm64_notify_die
    arm64_force_sig_fault
       force_sig_fault

Which means correcting this in arm64_force_sig_fault is enough
to ensure the arm64 code is not misusing the generic code, which
could lead to maintenance problems later.

Cc: stable@vger.kernel.org
Cc: Dave Martin <Dave.Martin@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Fixes: af40ff687bc9 ("arm64: signal: Ensure si_code is valid for all fault signals")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---

I have also made the corresponding changes to:
09/26 signal: Remove task parameter from force_sig
21/26 signal: Remove the task parameter from force_sig_fault
But I will leave off reposting those as for now as the changes
are obvious.

arch/arm64/kernel/traps.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index ade32046f3fe..e45d5b440fb1 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -256,7 +256,10 @@  void arm64_force_sig_fault(int signo, int code, void __user *addr,
 			   const char *str)
 {
 	arm64_show_signal(signo, str);
-	force_sig_fault(signo, code, addr, current);
+	if (signo == SIGKILL)
+		force_sig(SIGKILL, current);
+	else
+		force_sig_fault(signo, code, addr, current);
 }
 
 void arm64_force_sig_mceerr(int code, void __user *addr, short lsb,

Comments

Will Deacon May 23, 2019, 4:15 p.m.
On Thu, May 23, 2019 at 11:11:19AM -0500, Eric W. Biederman wrote:
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index ade32046f3fe..e45d5b440fb1 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -256,7 +256,10 @@ void arm64_force_sig_fault(int signo, int code, void __user *addr,
>  			   const char *str)
>  {
>  	arm64_show_signal(signo, str);
> -	force_sig_fault(signo, code, addr, current);
> +	if (signo == SIGKILL)
> +		force_sig(SIGKILL, current);
> +	else
> +		force_sig_fault(signo, code, addr, current);
>  }

Acked-by: Will Deacon <will.deacon@arm.com>

Are you planning to send this series on, or would you like me to pick this
into the arm64 tree?

Will
Eric W. Biederman May 23, 2019, 8:59 p.m.
Will Deacon <will.deacon@arm.com> writes:

> On Thu, May 23, 2019 at 11:11:19AM -0500, Eric W. Biederman wrote:
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index ade32046f3fe..e45d5b440fb1 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -256,7 +256,10 @@ void arm64_force_sig_fault(int signo, int code, void __user *addr,
>>  			   const char *str)
>>  {
>>  	arm64_show_signal(signo, str);
>> -	force_sig_fault(signo, code, addr, current);
>> +	if (signo == SIGKILL)
>> +		force_sig(SIGKILL, current);
>> +	else
>> +		force_sig_fault(signo, code, addr, current);
>>  }
>
> Acked-by: Will Deacon <will.deacon@arm.com>
>
> Are you planning to send this series on, or would you like me to pick this
> into the arm64 tree?

I am planning on taking this through siginfo tree, unless it causes
problems.

The rest of my patchset this is a part of is a clean up to remove
the task pointer which is always current from all of the force_sig
calls.

Eric
Will Deacon May 24, 2019, 10 a.m.
On Thu, May 23, 2019 at 03:59:20PM -0500, Eric W. Biederman wrote:
> Will Deacon <will.deacon@arm.com> writes:
> 
> > On Thu, May 23, 2019 at 11:11:19AM -0500, Eric W. Biederman wrote:
> >> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> >> index ade32046f3fe..e45d5b440fb1 100644
> >> --- a/arch/arm64/kernel/traps.c
> >> +++ b/arch/arm64/kernel/traps.c
> >> @@ -256,7 +256,10 @@ void arm64_force_sig_fault(int signo, int code, void __user *addr,
> >>  			   const char *str)
> >>  {
> >>  	arm64_show_signal(signo, str);
> >> -	force_sig_fault(signo, code, addr, current);
> >> +	if (signo == SIGKILL)
> >> +		force_sig(SIGKILL, current);
> >> +	else
> >> +		force_sig_fault(signo, code, addr, current);
> >>  }
> >
> > Acked-by: Will Deacon <will.deacon@arm.com>
> >
> > Are you planning to send this series on, or would you like me to pick this
> > into the arm64 tree?
> 
> I am planning on taking this through siginfo tree, unless it causes
> problems.

Okey doke, it would just be nice to see this patch land in 5.2, that's
all.

Will
Eric W. Biederman May 24, 2019, 10:36 p.m.
Will Deacon <will.deacon@arm.com> writes:

> On Thu, May 23, 2019 at 03:59:20PM -0500, Eric W. Biederman wrote:
>> Will Deacon <will.deacon@arm.com> writes:
>> 
>> > On Thu, May 23, 2019 at 11:11:19AM -0500, Eric W. Biederman wrote:
>> >> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> >> index ade32046f3fe..e45d5b440fb1 100644
>> >> --- a/arch/arm64/kernel/traps.c
>> >> +++ b/arch/arm64/kernel/traps.c
>> >> @@ -256,7 +256,10 @@ void arm64_force_sig_fault(int signo, int code, void __user *addr,
>> >>  			   const char *str)
>> >>  {
>> >>  	arm64_show_signal(signo, str);
>> >> -	force_sig_fault(signo, code, addr, current);
>> >> +	if (signo == SIGKILL)
>> >> +		force_sig(SIGKILL, current);
>> >> +	else
>> >> +		force_sig_fault(signo, code, addr, current);
>> >>  }
>> >
>> > Acked-by: Will Deacon <will.deacon@arm.com>
>> >
>> > Are you planning to send this series on, or would you like me to pick this
>> > into the arm64 tree?
>> 
>> I am planning on taking this through siginfo tree, unless it causes
>> problems.
>
> Okey doke, it would just be nice to see this patch land in 5.2, that's
> all.

As this does not appear to have any real world consequences I am aiming
at 5.3.  If someone else would like to take it and feed it to Linus
sooner I won't object.

Eric
Will Deacon May 29, 2019, 3:12 p.m.
On Fri, May 24, 2019 at 05:36:41PM -0500, Eric W. Biederman wrote:
> Will Deacon <will.deacon@arm.com> writes:
> 
> > On Thu, May 23, 2019 at 03:59:20PM -0500, Eric W. Biederman wrote:
> >> Will Deacon <will.deacon@arm.com> writes:
> >> 
> >> > On Thu, May 23, 2019 at 11:11:19AM -0500, Eric W. Biederman wrote:
> >> >> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> >> >> index ade32046f3fe..e45d5b440fb1 100644
> >> >> --- a/arch/arm64/kernel/traps.c
> >> >> +++ b/arch/arm64/kernel/traps.c
> >> >> @@ -256,7 +256,10 @@ void arm64_force_sig_fault(int signo, int code, void __user *addr,
> >> >>  			   const char *str)
> >> >>  {
> >> >>  	arm64_show_signal(signo, str);
> >> >> -	force_sig_fault(signo, code, addr, current);
> >> >> +	if (signo == SIGKILL)
> >> >> +		force_sig(SIGKILL, current);
> >> >> +	else
> >> >> +		force_sig_fault(signo, code, addr, current);
> >> >>  }
> >> >
> >> > Acked-by: Will Deacon <will.deacon@arm.com>
> >> >
> >> > Are you planning to send this series on, or would you like me to pick this
> >> > into the arm64 tree?
> >> 
> >> I am planning on taking this through siginfo tree, unless it causes
> >> problems.
> >
> > Okey doke, it would just be nice to see this patch land in 5.2, that's
> > all.
> 
> As this does not appear to have any real world consequences I am aiming
> at 5.3.  If someone else would like to take it and feed it to Linus
> sooner I won't object.

Thanks. I've picked this patch up as part of the arm64 fixes I plan to send
for -rc3.

Will
Eric W. Biederman May 29, 2019, 3:34 p.m.
Will Deacon <will.deacon@arm.com> writes:

> On Fri, May 24, 2019 at 05:36:41PM -0500, Eric W. Biederman wrote:
>> Will Deacon <will.deacon@arm.com> writes:
>> 
>> > On Thu, May 23, 2019 at 03:59:20PM -0500, Eric W. Biederman wrote:
>> >> Will Deacon <will.deacon@arm.com> writes:
>> >> 
>> >> > On Thu, May 23, 2019 at 11:11:19AM -0500, Eric W. Biederman wrote:
>> >> >> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> >> >> index ade32046f3fe..e45d5b440fb1 100644
>> >> >> --- a/arch/arm64/kernel/traps.c
>> >> >> +++ b/arch/arm64/kernel/traps.c
>> >> >> @@ -256,7 +256,10 @@ void arm64_force_sig_fault(int signo, int code, void __user *addr,
>> >> >>  			   const char *str)
>> >> >>  {
>> >> >>  	arm64_show_signal(signo, str);
>> >> >> -	force_sig_fault(signo, code, addr, current);
>> >> >> +	if (signo == SIGKILL)
>> >> >> +		force_sig(SIGKILL, current);
>> >> >> +	else
>> >> >> +		force_sig_fault(signo, code, addr, current);
>> >> >>  }
>> >> >
>> >> > Acked-by: Will Deacon <will.deacon@arm.com>
>> >> >
>> >> > Are you planning to send this series on, or would you like me to pick this
>> >> > into the arm64 tree?
>> >> 
>> >> I am planning on taking this through siginfo tree, unless it causes
>> >> problems.
>> >
>> > Okey doke, it would just be nice to see this patch land in 5.2, that's
>> > all.
>> 
>> As this does not appear to have any real world consequences I am aiming
>> at 5.3.  If someone else would like to take it and feed it to Linus
>> sooner I won't object.
>
> Thanks. I've picked this patch up as part of the arm64 fixes I plan to send
> for -rc3.

Sounds good.

We might have a trivial conflict between our branches as I am also
including this in my for-next branch, as I have further patches that go
on to remove the task argument from force_sig and force_sig_fault.

But I don't think it is anything to worry about.

Eric