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

[Michael Ellerman]

Nicholas Piggin <npiggin at gmail.com> writes:
> 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 :
...
>>>>> +
>>>>> +	/*
>>>>> +	 * 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.

I don't think it's a very attractive gadget, it's not just a plain blr,
it does a full stack frame tear down before the return. And there's no
LR reloads anywhere very close.

Obviously it depends on what the compiler decides to do, it's possible
it could be a usable gadget. But there are other places that are more
attractive I think, eg:

c00000000061d768:	a6 03 3d 7d 	mtspr   29,r9
c00000000061d76c:	2c 01 00 4c 	isync
c00000000061d770:	00 00 00 60 	nop
c00000000061d774:	30 00 21 38 	addi    r1,r1,48
c00000000061d778:	20 00 80 4e 	blr


So I don't think we should redesign the code *purely* because we're
worried about interrupt_exit_kernel_prepare() being a useful gadget. If
we can come up with a way to restructure it that reads well and is
maintainable, and also reduces the chance of it being a good gadget then
sure.

cheers