[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <877ca9zskd.fsf@gmail.com>
Date: Tue, 15 Oct 2024 13:49:46 +0530
From: Ritesh Harjani (IBM) <ritesh.list@...il.com>
To: Christophe Leroy <christophe.leroy@...roup.eu>, linuxppc-dev@...ts.ozlabs.org
Cc: kasan-dev@...glegroups.com, linux-mm@...ck.org, Marco Elver <elver@...gle.com>, Alexander Potapenko <glider@...gle.com>, Heiko Carstens <hca@...ux.ibm.com>, Michael Ellerman <mpe@...erman.id.au>, Nicholas Piggin <npiggin@...il.com>, Madhavan Srinivasan <maddy@...ux.ibm.com>, Hari Bathini <hbathini@...ux.ibm.com>, "Aneesh Kumar K . V" <aneesh.kumar@...nel.org>, Donet Tom <donettom@...ux.vnet.ibm.com>, Pavithra Prakash <pavrampu@...ux.vnet.ibm.com>, LKML <linux-kernel@...r.kernel.org>, Disha Goel <disgoel@...ux.ibm.com>
Subject: Re: [RFC RESEND v2 02/13] powerpc: mm: Fix kfence page fault reporting
Christophe Leroy <christophe.leroy@...roup.eu> writes:
> Le 15/10/2024 à 03:33, Ritesh Harjani (IBM) a écrit :
>> copy_from_kernel_nofault() can be called when doing read of /proc/kcore.
>> /proc/kcore can have some unmapped kfence objects which when read via
>> copy_from_kernel_nofault() can cause page faults. Since *_nofault()
>> functions define their own fixup table for handling fault, use that
>> instead of asking kfence to handle such faults.
>>
>> Hence we search the exception tables for the nip which generated the
>> fault. If there is an entry then we let the fixup table handler handle the
>> page fault by returning an error from within ___do_page_fault().
>>
>> This can be easily triggered if someone tries to do dd from /proc/kcore.
>> dd if=/proc/kcore of=/dev/null bs=1M
>>
>> <some example false negatives>
>> ===============================
>> BUG: KFENCE: invalid read in copy_from_kernel_nofault+0xb0/0x1c8
>> Invalid read at 0x000000004f749d2e:
>> copy_from_kernel_nofault+0xb0/0x1c8
>> 0xc0000000057f7950
>> read_kcore_iter+0x41c/0x9ac
>> proc_reg_read_iter+0xe4/0x16c
>> vfs_read+0x2e4/0x3b0
>> ksys_read+0x88/0x154
>> system_call_exception+0x124/0x340
>> system_call_common+0x160/0x2c4
>>
>> BUG: KFENCE: use-after-free read in copy_from_kernel_nofault+0xb0/0x1c8
>> Use-after-free read at 0x000000008fbb08ad (in kfence-#0):
>> copy_from_kernel_nofault+0xb0/0x1c8
>> 0xc0000000057f7950
>> read_kcore_iter+0x41c/0x9ac
>> proc_reg_read_iter+0xe4/0x16c
>> vfs_read+0x2e4/0x3b0
>> ksys_read+0x88/0x154
>> system_call_exception+0x124/0x340
>> system_call_common+0x160/0x2c4
>>
>> Guessing the fix should go back to when we first got kfence on PPC32.
>>
>> Fixes: 90cbac0e995d ("powerpc: Enable KFENCE for PPC32")
>> Reported-by: Disha Goel <disgoel@...ux.ibm.com>
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>
>> ---
>> arch/powerpc/mm/fault.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 81c77ddce2e3..fa825198f29f 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -439,9 +439,17 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
>> /*
>> * The kernel should never take an execute fault nor should it
>> * take a page fault to a kernel address or a page fault to a user
>> - * address outside of dedicated places
>> + * address outside of dedicated places.
>> + *
>> + * Rather than kfence reporting false negatives, let the fixup table
>> + * handler handle the page fault by returning SIGSEGV, if the fault
>> + * has come from functions like copy_from_kernel_nofault().
>> */
>> if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address, is_write))) {
>> +
>> + if (search_exception_tables(instruction_pointer(regs)))
>> + return SIGSEGV;
>
> This is a heavy operation. It should at least be done only when KFENCE
> is built-in.
>
> kfence_handle_page_fault() bails out immediately when
> is_kfence_address() returns false, and is_kfence_address() returns
> always false when KFENCE is not built-in.
>
> So you could check that before calling the heavy weight
> search_exception_tables().
>
> if (is_kfence_address(address) &&
> !search_exception_tables(instruction_pointer(regs)) &&
> kfence_handle_page_fault(address, is_write, regs))
> return 0;
>
Yes, thanks for the input. I agree with above. I will take that in v3.
I will wait for sometime for any review comments on other patches before
spinning a v3, though.
>
>
> > + return SIGSEGV;
>
>> +
>> if (kfence_handle_page_fault(address, is_write, regs))
>> return 0;
>>
-ritesh
Powered by blists - more mailing lists