Re: [PATCH V5 03/50] x86/traps: Remove stack-protector from traps.c

From: Lai Jiangshan <laijs@linux.alibaba.com>
To: Peter Zijlstra <peterz@infradead.org>,
	Lai Jiangshan <jiangshanlai@gmail.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Javier Martinez Canillas <javierm@redhat.com>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	Brijesh Singh <brijesh.singh@amd.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Juergen Gross <jgross@suse.com>
Subject: Re: [PATCH V5 03/50] x86/traps: Remove stack-protector from traps.c
Date: Fri, 19 Nov 2021 09:38:17 +0800	[thread overview]
Message-ID: <d74f249a-5dba-e45b-0779-65fad1b63ba6@linux.alibaba.com> (raw)
In-Reply-To: <20211118195504.GM174703@worktop.programming.kicks-ass.net>

On 2021/11/19 03:55, Peter Zijlstra wrote:
> On Wed, Nov 10, 2021 at 07:56:49PM +0800, Lai Jiangshan wrote:
>> From: Lai Jiangshan <laijs@linux.alibaba.com>
>>
>> When stack-protector is enabled, the compiler adds some instrument code
>> at the beginning and the end of some functions. Many functions in traps.c
>> are non-instrumentable.  Moreover, stack-protector code in the beginning
>> of the affected function accesses the canary that might be watched by
>> hardware breakpoints which also violate the non-instrumentable
>> nature of some functions and might cause infinite recursive #DB because
>> the canary is accessed before resetting the dr7.
>>
>> So it is better to remove stack-protector from traps.c.
>>
>> It is also prepared for later patches that move some entry code into
>> traps.c, some of which can NOT use percpu register until gsbase is
>> properly switched.  And stack-protector depends on the percpu register
>> to work.
>>
>> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
>> ---
>>   arch/x86/kernel/Makefile | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>> index 2ff3e600f426..8ac45801ba8b 100644
>> --- a/arch/x86/kernel/Makefile
>> +++ b/arch/x86/kernel/Makefile
>> @@ -50,6 +50,7 @@ KCOV_INSTRUMENT		:= n
>>   
>>   CFLAGS_head$(BITS).o	+= -fno-stack-protector
>>   CFLAGS_cc_platform.o	+= -fno-stack-protector
>> +CFLAGS_traps.o		+= -fno-stack-protector
> 
> Well, there's a lot more noinstr than just in traps. 

Although it is stupid to put hardware break point on the stack canary,
it is fatal only in traps.c when the canary is accessed before
resetting the dr7 in #DB handler.

And this only happens when the administer of the system is deliberately
hurting the system, so the fix is not strongly required in this problem.

The best way is to disallow hw_breakpoint to watch the stack canary.

The later patch (patch39) puts __entry_code into traps.c which makes
no_stack_protector is strongly required, so this patch just simply puts
the -fno-stack-protector on traps.c.

> There's also real C
> code in traps. This isn't really a solution.

This patch focuses "hardware break point on the stack canary" only.

It is not a full solution [for other unhappiness when noistr is watching
by stack protector].

I will switch to disallow hw_breakpoint to watch the stack canary.

> 
> I think GCC has recently grown __attribute__((no_stack_protector)),
> which should be added to noinstr (GCC-11 and above).
> 
> Additionally we could add code to objtool to detect this problem.
> 