Excerpts from Christophe Leroy's message of February 5, 2021 4:04 pm: > > > 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 <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. >> > > 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.
I don't see a problem with that. Thanks, Nick
