linux-kernel - Re: [PATCH v1 4/5] powerpc/fault: Avoid heavy search_exception

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]

Message-Id: <1600048261.m5q9cmngxb.astroid@bobo.none>
Date:   Mon, 14 Sep 2020 12:23:01 +1000
From:   Nicholas Piggin <npiggin@...il.com>
To:     "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Michael Ellerman <mpe@...erman.id.au>,
        Paul Mackerras <paulus@...ba.org>
Cc:     linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v1 4/5] powerpc/fault: Avoid heavy
 search_exception_tables() verification

Excerpts from Christophe Leroy's message of September 9, 2020 4:20 pm:
> 
> 
> Le 09/09/2020 à 08:04, Aneesh Kumar K.V a écrit :
>> Christophe Leroy <christophe.leroy@...roup.eu> writes:
>> 
>>> search_exception_tables() is an heavy operation, we have to avoid it.
>>> When KUAP is selected, we'll know the fault has been blocked by KUAP.
>>> Otherwise, it behaves just as if the address was already in the TLBs
>>> and no fault was generated.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
>>> ---
>>>   arch/powerpc/mm/fault.c | 20 +++++---------------
>>>   1 file changed, 5 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>>> index 525e0c2b5406..edde169ba3a6 100644
>>> --- a/arch/powerpc/mm/fault.c
>>> +++ b/arch/powerpc/mm/fault.c
>>> @@ -214,24 +214,14 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
>>>   	if (address >= TASK_SIZE)
>>>   		return true;
>>>   
>>> -	if (!is_exec && (error_code & DSISR_PROTFAULT) &&
>>> -	    !search_exception_tables(regs->nip)) {
>>> +	// Read/write fault blocked by KUAP is bad, it can never succeed.
>>> +	if (bad_kuap_fault(regs, address, is_write)) {
>>>   		pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
>>> -				    address,
>>> -				    from_kuid(&init_user_ns, current_uid()));
>>> -	}
>>> -
>>> -	// Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
>>> -	if (!search_exception_tables(regs->nip))
>>> -		return true;
>> 
>> We still need to keep this ? Without that we detect the lack of
>> exception tables pretty late.
> 
> Is that a problem at all to detect the lack of exception tables late ?
> That case is very unlikely and will lead to failure anyway. So, is it 
> worth impacting performance of the likely case which will always have an 
> exception table and where we expect the exception to run as fast as 
> possible ?
> 
> The other architectures I have looked at (arm64 and x86) only have the 
> exception table search together with the down_read_trylock(&mm->mmap_sem).

Yeah I don't see how it'd be a problem. User could arrange for page 
table to already be at this address and avoid the fault so it's not the 
right way to stop an attacker, KUAP is.

Thanks,
Nick