linux-kernel - Re: [PATCH] x86/unwind/orc: fix inactive tasks with sp in sp

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] [day] [month] [year] [list]

Date:   Wed, 14 Oct 2020 07:08:52 +0200
From:   Jiri Slaby <jslaby@...e.cz>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     linux-kernel@...r.kernel.org, Miroslav Benes <mbenes@...e.cz>
Subject: Re: [PATCH] x86/unwind/orc: fix inactive tasks with sp in sp

On 07. 10. 20, 16:54, Josh Poimboeuf wrote:
> -ENOPARSE on $SUBJECT.
> 
> Also please address it to x86@...nel.org, I think the tip maintainers
> can pick up the fix directly.

Hmm, weird, I must have sent an older version as my current patch in the 
tree has:
Cc: Miroslav Benes <mbenes@...e.cz>
Cc: Josh Poimboeuf <jpoimboe@...hat.com>
Cc: Thomas Gleixner <tglx@...utronix.de>
Cc: Ingo Molnar <mingo@...hat.com>
Cc: Borislav Petkov <bp@...en8.de>
Cc: "H. Peter Anvin" <hpa@...or.com>
Cc: x86@...nel.org

> Also it might be a good idea to Cc the live-patching mailing list, I
> presume this causes a livepatch stall?
> 
> On Wed, Oct 07, 2020 at 10:19:09AM +0200, Jiri Slaby wrote:
>> gcc-10 optimizes the scheduler code differently than its predecessors,
>> depending on DEBUG_SECTION_MISMATCH=y config -- the config sets
>> -fno-inline-functions-called-once.
> 
> Weird.  Was GCC ignoring this flag before?

gcc 7 generated the earlier mentioned prologue (push bp; mov sp,bp). So 
we extract stack pointer from bp. gcc 10 no longer generates the 
prologue in some of the standalone functions. That's the difference. So 
we started extracting stack pointer from sp which contains more than we 
expect.

And the problem also (obviously) dismisses when gcc (even 10) inlines 
the function.

>> @@ -663,7 +656,13 @@ void __unwind_start(struct unwind_state *state, struct task_struct *task,
>>   	} else {
>>   		struct inactive_task_frame *frame = (void *)task->thread.sp;
>>   
>> -		state->sp = task->thread.sp;
>> +		/*
>> +		 * @ret_addr is in __schedule _before_ the @frame is pushed to
>> +		 * the stack, but @thread.sp is saved in __switch_to_asm only
>> +		 * _after_ saving the @frame, so subtract the @frame size, i.e.
>> +		 * add it to @thread.sp.
>> +		 */
>> +		state->sp = task->thread.sp + sizeof(*frame);
> 
> IMO, the code speaks for itself and the comment may be superfluous.
> 
> Otherwise it looks good to me.  Thanks for fixing it!

OK, will resend the correct and fixed version.

thanks,
-- 
js
suse labs