[PATCH v3 28/32] powerpc/64s: interrupt implement exit logic in C

[Nicholas Piggin]

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 = &current_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