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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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

> 
>> 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.

Christophe