linux-kernel - Re: [REVIEW][PATCH 03/26] signal/arm64: Use force_sig not force_sig

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190523102101.GW28398@e103592.cambridge.arm.com>
Date:   Thu, 23 May 2019 11:21:04 +0100
From:   Dave Martin <Dave.Martin@....com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linux Containers <containers@...ts.linux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        James Morse <James.Morse@....com>,
        Will Deacon <Will.Deacon@....com>
Subject: Re: [REVIEW][PATCH 03/26] signal/arm64: Use force_sig not
 force_sig_fault for SIGKILL

On Thu, May 23, 2019 at 01:38:53AM +0100, Eric W. Biederman wrote:
> It really only matters to debuggers but the 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.

I haven't fully understood the context for this, but why does it matter
what's in siginfo for SIGKILL?  My understanding is that userspace
(including ptrace) never gets to see it anyway for the SIGKILL case.

Here it feels like SIGKILL is logically a synchronous, thread-targeted
fault: we must ensure that no subsequent insn in current executes (just
like other fault signal).  In this case, I thought we fall back to
SIGKILL not because there is no fault, but because we failed to
properly diagnose or report the type of fault that occurred.

So maybe handling it consistently with other faults signals makes
sense.  The fact that delivery of this signal destroys the process
before anyone can look at the resulting siginfo feels like a
side-effect rather than something obviously wrong.

The siginfo is potentially useful diagnostic information, that we could
subsequently provide a means to access post-mortem.

I just dived in on this single patch, so I may be missing something more
fundamental, or just being pedantic...

Cheers
---Dave

> Cc: stable@...r.kernel.org
> Cc: Dave Martin <Dave.Martin@....com>
> Cc: James Morse <james.morse@....com>
> Cc: Will Deacon <will.deacon@....com>
> Fixes: af40ff687bc9 ("arm64: signal: Ensure si_code is valid for all fault signals")
> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>
> ---
>  arch/arm64/kernel/traps.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index ade32046f3fe..0feb17bdcaa0 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -282,6 +282,11 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
>  		current->thread.fault_address = 0;
>  		current->thread.fault_code = err;
>  
> +		if (signo == SIGKILL) {
> +			arm64_show_signal(signo, str);
> +			force_sig(signo, current);
> +			return;
> +		}
>  		arm64_force_sig_fault(signo, sicode, addr, str);
>  	} else {
>  		die(str, regs, err);
> -- 
> 2.21.0
>