Excerpts from Christophe Leroy's message of February 4, 2021 6:03 pm:
>
>
> Le 04/02/2021 à 04:27, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of February 4, 2021 2:25 am:
>>>
>>>
>>> Le 25/02/2020 à 18:35, Nicholas Piggin a écrit :
>>>> Implement the bulk of interrupt return logic in C. The asm return code
>>>> must handle a few cases: restoring full GPRs, and emulating stack store.
>>>>
>>>
>>>
>>>> +notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs,
>>>> unsigned long msr)
>>>> +{
>>>> + unsigned long *ti_flagsp = ¤t_thread_info()->flags;
>>>> + unsigned long flags;
>>>> +
>>>> + if (IS_ENABLED(CONFIG_PPC_BOOK3S) && unlikely(!(regs->msr & MSR_RI)))
>>>> + unrecoverable_exception(regs);
>>>> + BUG_ON(regs->msr & MSR_PR);
>>>> + BUG_ON(!FULL_REGS(regs));
>>>> +
>>>> + local_irq_save(flags);
>>>> +
>>>> + if (regs->softe == IRQS_ENABLED) {
>>>> + /* Returning to a kernel context with local irqs enabled. */
>>>> + WARN_ON_ONCE(!(regs->msr & MSR_EE));
>>>> +again:
>>>> + if (IS_ENABLED(CONFIG_PREEMPT)) {
>>>> + /* Return to preemptible kernel context */
>>>> + if (unlikely(*ti_flagsp & _TIF_NEED_RESCHED)) {
>>>> + if (preempt_count() == 0)
>>>> + preempt_schedule_irq();
>>>> + }
>>>> + }
>>>> +
>>>> + trace_hardirqs_on();
>>>> + __hard_EE_RI_disable();
>>>> + if (unlikely(lazy_irq_pending())) {
>>>> + __hard_RI_enable();
>>>> + irq_soft_mask_set(IRQS_ALL_DISABLED);
>>>> + trace_hardirqs_off();
>>>> + local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
>>>> + /*
>>>> + * Can't local_irq_enable in case we are in interrupt
>>>> + * context. Must replay directly.
>>>> + */
>>>> + replay_soft_interrupts();
>>>> + irq_soft_mask_set(flags);
>>>> + /* Took an interrupt, may have more exit work to do. */
>>>> + goto again;
>>>> + }
>>>> + local_paca->irq_happened = 0;
>>>> + irq_soft_mask_set(IRQS_ENABLED);
>>>> + } else {
>>>> + /* Returning to a kernel context with local irqs disabled. */
>>>> + trace_hardirqs_on();
>>>> + __hard_EE_RI_disable();
>>>> + if (regs->msr & MSR_EE)
>>>> + local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
>>>> + }
>>>> +
>>>> +
>>>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>>>> + local_paca->tm_scratch = regs->msr;
>>>> +#endif
>>>> +
>>>> + /*
>>>> + * We don't need to restore AMR on the way back to userspace for KUAP.
>>>> + * The value of AMR only matters while we're in the kernel.
>>>> + */
>>>> + kuap_restore_amr(regs);
>>>
>>> Is that correct to restore KUAP state here ? Shouldn't we have it at lower
>>> level in assembly ?
>>>
>>> Isn't there a risk that someone manages to call
>>> interrupt_exit_kernel_prepare() or the end of it in
>>> a way or another, and get the previous KUAP state restored by this way ?
>>
>> I'm not sure if there much more risk if it's here rather than the
>> instruction being in another place in the code.
>>
>> There's a lot of user access around the kernel too if you want to find a
>> gadget to unlock KUAP then I suppose there is a pretty large attack
>> surface.
>
> My understanding is that user access scope is strictly limited, for instance
> we enforce the
> begin/end of user access to be in the same function, and we refrain from
> calling any other function
> inside the user access window. x86 even have 'objtool' to enforce it at build
> time. So in theory
> there is no way to get out of the function while user access is open.
>
> Here with the interrupt exit function it is free beer. You have a place where
> you re-open user
> access and return with a simple blr. So that's open bar. If someone manages
> to just call the
> interrupt exit function, then user access remains open
Hmm okay maybe that's a good point.
>>> Also, it looks a bit strange to have kuap_save_amr_and_lock() done at
>>> lowest level in assembly, and
>>> kuap_restore_amr() done in upper level. That looks unbalanced.
>>
>> I'd like to bring the entry assembly into C.
>>
>
> I really think it's not a good idea. We'll get better control and readability
> by keeping it at the
> lowest possible level in assembly.
>
> x86 only save and restore SMAC state in assembly.
Okay we could go the other way and move the unlock into asm then.
Thanks,
Nick
