Le 17/09/2022 à 20:38, Samuel Holland a écrit :
> On 9/17/22 03:16, Christophe Leroy wrote:
>> Le 16/09/2022 à 07:05, Samuel Holland a écrit :
>>> With CONFIG_PREEMPT=y (involuntary preemption enabled), it is possible
>>> to switch away from a task inside copy_{from,to}_user. This left the CPU
>>> with userspace access enabled until after the next IRQ or privilege
>>> level switch, when AMR/IAMR got reset to AMR_KU[AE]P_BLOCKED. Then, when
>>> switching back to the original task, the userspace access would fault:
>>
>> This is not supposed to happen. You never switch away from a task
>> magically. Task switch will always happen in an interrupt, that means
>> copy_{from,to}_user() get interrupted.
>
> That makes sense, the interrupt handler is responsible for saving the
> KUAP status. It looks like neither DEFINE_INTERRUPT_HANDLER_RAW nor any
> of its users (performance_monitor_exception(), do_slb_fault()) do that.
As far as I can see, that's done earlier, in exceptions-64s.S.
Look for kuap_save_amr_and_lock.
Now, it may be possible that one of the exceptions pathes misses it.
> Yet they still call one of the interrupt_return variants, which restores
> the status from the stack.
>
>> Whenever an interrupt is taken, kuap_save_amr_and_lock() macro is used
>> to save KUAP status into the stack then lock KUAP access. At interrupt
>> exit, kuap_kernel_restore() macro or function is used to restore KUAP
>> access from the stack. At the time the task switch happens, KUAP access
>> is expected to be locked. During task switch, the stack is switched so
>> the KUAP status is taken back from the new task's stack.
>
> What if another task calls schedule() from kernel process context, and
> the scheduler switches to a task that had been preempted inside
> copy_{from,to}_user()? Then there is no interrupt involved, and I don't
> see where kuap_kernel_restore() would get called.
Yes there is interrupt involved. That task, if it has been preempted
inside copy_from_user(), it must have been through an interrupt, likely
a timer interrupt. So switching back to that task means doing an
interrupt return in the context of that task. That's when KUAP status
should be restored.
>
>> Your fix suggests that there is some path where the KUAP status is not
>> properly saved and/or restored. Did you try running with
>> CONFIG_PPC_KUAP_DEBUG ? It should warn whenever a KUAP access is left
>> unlocked.
>>
>>>
>>> Kernel attempted to write user page (3fff7ab68190) - exploit attempt?
>>> (uid: 65536)
>>> ------------[ cut here ]------------
>>> Bug: Write fault blocked by KUAP!
>>> WARNING: CPU: 56 PID: 4939 at arch/powerpc/mm/fault.c:228
>>> ___do_page_fault+0x7b4/0xaa0
>>> CPU: 56 PID: 4939 Comm: git Tainted: G W
>>> 5.19.8-00005-gba424747260d #1
>>> NIP: c0000000000555e4 LR: c0000000000555e0 CTR: c00000000079d9d0
>>> REGS: c00000008f507370 TRAP: 0700 Tainted: G W
>>> (5.19.8-00005-gba424747260d)
>>> MSR: 9000000000021033 <SF,HV,ME,IR,DR,RI,LE> CR: 28042222 XER:
>>> 20040000
>>> CFAR: c000000000123780 IRQMASK: 3
>>> NIP [c0000000000555e4] ___do_page_fault+0x7b4/0xaa0
>>> LR [c0000000000555e0] ___do_page_fault+0x7b0/0xaa0
>>> Call Trace:
>>> [c00000008f507610] [c0000000000555e0] ___do_page_fault+0x7b0/0xaa0
>>> (unreliable)
>>> [c00000008f5076c0] [c000000000055938] do_page_fault+0x68/0x130
>>> [c00000008f5076f0] [c000000000008914]
>>> data_access_common_virt+0x194/0x1f0
>>> --- interrupt: 300 at __copy_tofrom_user_base+0x9c/0x5a4
>>
>> ...
>>
>>>
>>> Fix this by saving and restoring the kernel-side AMR/IAMR values when
>>> switching tasks.
>>
>> As explained above, KUAP access should be locked at that time, so saving
>> and restoring it should not have any effect. If it does, it means
>> something goes wrong somewhere else.
>>
>>>
>>> Fixes: 890274c2dc4c ("powerpc/64s: Implement KUAP for Radix MMU")
>>> Signed-off-by: Samuel Holland <sam...@sholland.org>
>>> ---
>>> I have no idea if this is the right change to make, and it could be
>>> optimized, but my system has been stable with this patch for 5 days now.
>>>
>>> Without the patch, I hit the bug every few minutes when my load average
>>> is <1, and I hit it immediately if I try to do a parallel kernel build.
>>
>> Great, then can you make a try with CONFIG_PPC_KUAP_DEBUG ?
>
> Yes, I will try this out in the next few days.
>
Thanks
Christophe
