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

[Christophe Leroy]

Le 05/02/2021 à 03:16, Nicholas Piggin a écrit :
> Excerpts from Michael Ellerman's message of February 5, 2021 10:22 am:
>> 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.
> 
> Okay. That would be good if we can keep it in C, the pkeys + kuap combo
> is fairly complicated and we might want to something cleverer with it,
> so that would make it even more difficult in asm.
> 

Ok.

For ppc32, I prefer to keep it in assembly for the time being and move everything from ASM to C at 
once after porting syscall and interrupts to C and wrappers.

Hope this is OK for you.

Christophe