Christophe Leroy [off-list ref] writes:

Le 19/09/2022 à 14:37, Michael Ellerman a écrit :

quoted

Christophe Leroy [off-list ref] writes:

quoted

Le 16/09/2022 à 07:05, Samuel Holland a écrit :

quoted

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.

Unfortunately this isn't true when CONFIG_PREEMPT=y.

Argh, yes, I wrote the above with the assumption that we properly follow 
the main principles that no complex fonction is to be used while KUAP is 
open ... Which is apparently not true here. x86 would have detected it 
with objtool, but we don't have it yet in powerpc.

Yes and yes :/

quoted

We can switch away without an interrupt via:
   __copy_tofrom_user()
     -> __copy_tofrom_user_power7()
        -> exit_vmx_usercopy()
           -> preempt_enable()
              -> __preempt_schedule()
                 -> preempt_schedule()
                    -> preempt_schedule_common()
                       -> __schedule()

Should we use preempt_enable_no_resched() to avoid that ?

Good point :)

...

quoted

Still I think it might be our best option for an easy fix.

Wouldn't it be even easier and less abusive to use 
preemt_enable_no_resched() ? Or is there definitively a good reason to 
resched after a VMX copy while we don't with regular copies ?

I don't think it's important to reschedule there. I guess it means
another task that wants to preempt will have to wait a little longer,
but I doubt it's measurable.

One reason to do the KUAP lock is it also protects us from running
disable_kernel_altivec() with KUAP unlocked.

That in turn calls into msr_check_and_clear() and
__msr_check_and_clear(), none of which is a problem per-se, but we'd
need to make that all notrace to be 100% safe.

At the moment we're running that all with KUAP unlocked anyway, so using
preempt_enable_no_resched() would fix the actual bug and is much nicer
than my patch, so we should probably do that.

We can look at making disable_kernel_altivec() etc. notrace as a
follow-up.

cheers