linux-kernel - Re: [PATCH v3 2/3] MIPS: Remove noreturn attribute for die()

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <efc0eacb-195a-9587-8315-15a31ae9bedd@loongson.cn>
Date:   Sat, 19 Aug 2023 10:44:01 +0800
From:   Tiezhu Yang <yangtiezhu@...ngson.cn>
To:     "Maciej W. Rozycki" <macro@...am.me.uk>
Cc:     Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
        loongson-kernel@...ts.loongnix.cn
Subject: Re: [PATCH v3 2/3] MIPS: Remove noreturn attribute for die()



On 08/18/2023 10:41 AM, Maciej W. Rozycki wrote:
> On Mon, 14 Aug 2023, Tiezhu Yang wrote:
>
>> If notify_die() returns NOTIFY_STOP, honor the return value from the
>> handler chain invocation in die() as, through a debugger, the fault
>> may have been fixed. It makes sense even if ignoring the event will
>> make the system unstable, by allowing access through a debugger it
>> has been compromised already anyway. So we can remove the noreturn
>> attribute for die() to make our port consistent with x86, arm64,
>> riscv and csky.
>
>  I find it weird that you say that it is specifically the removal of the
> `noreturn' attribute that makes our port consistent with the other ones
> (and make it the change heading too).  I don't think you need to mention
> the removal of `noreturn' even as you can see it in the code itself and
> it's a natural consequence of the change proper.  How about:
>
> "
> MIPS: Do not kill the task in die() if notify_die() returns NOTIFY_STOP
>
> If notify_die() returns NOTIFY_STOP, honor the return value from the
> handler chain invocation in die() and return without killing the task
> as, through a debugger, the fault may have been fixed. It makes sense
> even if ignoring the event will make the system unstable: by allowing
> access through a debugger it has been compromised already anyway. It
> makes our port consistent with x86, arm64, riscv and csky.
> "
>
> then (notice the use of a colon rather than a comma changing the meaning
> of the sentence above)?

OK, it looks better.

>
>> Commit 20c0d2d44029 ("[PATCH] i386: pass proper trap numbers to die
>> chain handlers") may be the earliest of similar changes.
>>
>> Link: https://lore.kernel.org/all/alpine.DEB.2.21.2308132148500.8596@angie.orcam.me.uk/
>
>  I think you meant:
>
> Link: https://lore.kernel.org/r/43DDF02E.76F0.0078.0@novell.com/
>
> didn't you?

Yes, I will update it in v4.

>
>> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
>> index 7a34674..4f5140f 100644
>> --- a/arch/mips/kernel/traps.c
>> +++ b/arch/mips/kernel/traps.c
>> @@ -391,16 +391,15 @@ void show_registers(struct pt_regs *regs)
>>
>>  static DEFINE_RAW_SPINLOCK(die_lock);
>>
>> -void __noreturn die(const char *str, struct pt_regs *regs)
>> +void die(const char *str, struct pt_regs *regs)
>>  {
>>  	static int die_counter;
>> -	int sig = SIGSEGV;
>> +	int ret;
>>
>>  	oops_enter();
>>
>> -	if (notify_die(DIE_OOPS, str, regs, 0, current->thread.trap_nr,
>> -		       SIGSEGV) == NOTIFY_STOP)
>> -		sig = 0;
>> +	ret = notify_die(DIE_OOPS, str, regs, 0,
>> +			 current->thread.trap_nr, SIGSEGV);
>>
>>  	console_verbose();
>>  	raw_spin_lock_irq(&die_lock);
>> @@ -422,7 +421,8 @@ void __noreturn die(const char *str, struct pt_regs *regs)
>>  	if (regs && kexec_should_crash(current))
>>  		crash_kexec(regs);
>>
>> -	make_task_dead(sig);
>> +	if (ret != NOTIFY_STOP)
>> +		make_task_dead(SIGSEGV);
>
>  It doesn't appear to me we should panic or execute the crash kernel if
> the oops is to be suppressed.  Can we just do what the x86 port does, that
> is return if !sig after the call to `oops_exit'?

Yes, I think so, I will add a separate patch to do this.

>
>  Also I note that the individual ports aren't exactly consistent here with
> respect to each other, so maybe that's something you might want to post a
> combined follow-up clean-up patch series for too?
>

Maybe do it someday if possible.

Thanks,
Tiezhu