linux-kernel - Re: [PATCH v10 3/6] arm64: add uaccess to machine check safe

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]

Date: Tue, 30 Jan 2024 21:41:28 +0800
From: Tong Tiangen <tongtiangen@...wei.com>
To: Mark Rutland <mark.rutland@....com>
CC: Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
	James Morse <james.morse@....com>, Robin Murphy <robin.murphy@....com>,
	Andrey Ryabinin <ryabinin.a.a@...il.com>, Alexander Potapenko
	<glider@...gle.com>, Alexander Viro <viro@...iv.linux.org.uk>, Andrey
 Konovalov <andreyknvl@...il.com>, Dmitry Vyukov <dvyukov@...gle.com>,
	Vincenzo Frascino <vincenzo.frascino@....com>, Andrew Morton
	<akpm@...ux-foundation.org>, Michael Ellerman <mpe@...erman.id.au>, Nicholas
 Piggin <npiggin@...il.com>, Christophe Leroy <christophe.leroy@...roup.eu>,
	Aneesh Kumar K.V <aneesh.kumar@...nel.org>, "Naveen N. Rao"
	<naveen.n.rao@...ux.ibm.com>, Thomas Gleixner <tglx@...utronix.de>, Ingo
 Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
	<dave.hansen@...ux.intel.com>, <x86@...nel.org>, "H. Peter Anvin"
	<hpa@...or.com>, <linux-arm-kernel@...ts.infradead.org>,
	<linux-mm@...ck.org>, <linuxppc-dev@...ts.ozlabs.org>,
	<linux-kernel@...r.kernel.org>, <kasan-dev@...glegroups.com>,
	<wangkefeng.wang@...wei.com>, Guohanjun <guohanjun@...wei.com>
Subject: Re: [PATCH v10 3/6] arm64: add uaccess to machine check safe



在 2024/1/30 20:01, Mark Rutland 写道:
> On Tue, Jan 30, 2024 at 07:14:35PM +0800, Tong Tiangen wrote:
>> 在 2024/1/30 1:43, Mark Rutland 写道:
>>> On Mon, Jan 29, 2024 at 09:46:49PM +0800, Tong Tiangen wrote:
>>> Further, this change will also silently fixup unexpected kernel faults if we
>>> pass bad kernel pointers to copy_{to,from}_user, which will hide real bugs.
>>
>> I think this is better than the panic kernel, because the real bugs
>> belongs to the user process. Even if the wrong pointer is
>> transferred, the page corresponding to the wrong pointer has a memroy
>> error.
> 
> I think you have misunderstood my point; I'm talking about the case of a bad
> kernel pointer *without* a memory error.
> 
> For example, consider some buggy code such as:
> 
> 	void __user *uptr = some_valid_user_pointer;
> 	void *kptr = NULL; // or any other bad pointer
> 
> 	ret = copy_to_user(uptr, kptr, size);
> 	if (ret)
> 		return -EFAULT;
> 
> Before this patch, when copy_to_user() attempted to load from NULL it would
> fault, there would be no fixup handler for the LDR, and the kernel would die(),
> reporting the bad kernel access.
> 
> After this patch (which adds fixup handlers to all the LDR*s in
> copy_to_user()), the fault (which is *not* a memory error) would be handled by
> the fixup handler, and copy_to_user() would return an error without *any*
> indication of the horrible kernel bug.
> 
> This will hide kernel bugs, which will make those harder to identify and fix,
> and will also potentially make it easier to exploit the kernel: if the user
> somehow gains control of the kernel pointer, they can rely on the fixup handler
> returning an error, and can scan through memory rather than dying as soon as
> they pas a bad pointer.

I should understand what you mean. I'll think about this and reply.

Many thanks.
Tong.

> 
>> In addition, the panic information contains necessary information
>> for users to check.
> 
> There is no panic() in the case I am describing.
> 
>>> So NAK to this change as-is; likewise for the addition of USER() to other ldr*
>>> macros in copy_from_user.S and the addition of USER() str* macros in
>>> copy_to_user.S.
>>>
>>> If we want to handle memory errors on some kaccesses, we need a new EX_TYPE_*
>>> separate from the usual EX_TYPE_KACESS_ERR_ZERO that means "handle memory
>>> errors, but treat other faults as fatal". That should come with a rationale and
>>> explanation of why it's actually useful.
>>
>> This makes sense. Add kaccess types that can be processed properly.
>>
>>>
>>> [...]
>>>
>>>> diff --git a/arch/arm64/mm/extable.c b/arch/arm64/mm/extable.c
>>>> index 478e639f8680..28ec35e3d210 100644
>>>> --- a/arch/arm64/mm/extable.c
>>>> +++ b/arch/arm64/mm/extable.c
>>>> @@ -85,10 +85,10 @@ bool fixup_exception_mc(struct pt_regs *regs)
>>>>    	if (!ex)
>>>>    		return false;
>>>> -	/*
>>>> -	 * This is not complete, More Machine check safe extable type can
>>>> -	 * be processed here.
>>>> -	 */
>>>> +	switch (ex->type) {
>>>> +	case EX_TYPE_UACCESS_ERR_ZERO:
>>>> +		return ex_handler_uaccess_err_zero(ex, regs);
>>>> +	}
>>>
>>> Please fold this part into the prior patch, and start ogf with *only* handling
>>> errors on accesses already marked with EX_TYPE_UACCESS_ERR_ZERO. I think that
>>> change would be relatively uncontroversial, and it would be much easier to
>>> build atop that.
>>
>> OK, the two patches will be merged in the next release.
> 
> Thanks.
> 
> Mark.
> .