Excerpts from Michael Ellerman's message of February 5, 2021 10:22 am: > Nicholas Piggin <npig...@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. Thanks, Nick
