Le 12/07/2023 à 16:23, Peter Zijlstra a écrit : > On Tue, Jul 11, 2023 at 06:08:26PM +0200, Christophe Leroy wrote: >> This series adds UACCESS validation for PPC32. It includes >> a dozen of changes to objtool core. >> >> It applies on top of series "Cleanup/Optimise KUAP (v3)" >> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=363368&state=* > > That contains: > > +static __always_inline void uaccess_begin_32s(unsigned long addr) > +{ > + unsigned long tmp; > + > + asm volatile(ASM_MMU_FTR_IFSET( > + "mfsrin %0, %1;" > + "rlwinm %0, %0, 0, %2;" > + "mtsrin %0, %1;" > + "isync", "", %3) > + : "=&r"(tmp) > + : "r"(addr), "i"(~SR_KS), "i"(MMU_FTR_KUAP) > + : "memory"); > +} > + > +static __always_inline void uaccess_end_32s(unsigned long addr) > +{ > + unsigned long tmp; > + > + asm volatile(ASM_MMU_FTR_IFSET( > + "mfsrin %0, %1;" > + "oris %0, %0, %2;" > + "mtsrin %0, %1;" > + "isync", "", %3) > + : "=&r"(tmp) > + : "r"(addr), "i"(SR_KS >> 16), "i"(MMU_FTR_KUAP) > + : "memory"); > +} > > And I am a bit puzzled by the isync placement of uaccess_end, should > that not start with the isync, to ensure completion of the uaccess > region before disabling it? > > Or is that not the purpose of the isync?
Good question. The C function is: static __always_inline void kuap_lock_one(unsigned long addr) { mtsr(mfsr(addr) | SR_KS, addr); isync(); /* Context sync required after mtsr() */ } static __always_inline void kuap_unlock_one(unsigned long addr) { mtsr(mfsr(addr) & ~SR_KS, addr); isync(); /* Context sync required after mtsr() */ } So uaccess_begin_32s() and uaccess_end_32s() are built from that. Looking at the history I have never put an isync up front even in the very first implementation back in 2019 in commit a68c31fc01ef ("powerpc/32s: Implement Kernel Userspace Access Protection") Well just before that commit we have commit 31ed2b13c48d ("powerpc/32s: Implement Kernel Userspace Execution Prevention.") which for sure doesn't require the isync according to the "Programming Environments Manual for 32-Bit Implementations of the PowerPC™ Architecture". But for data access the same manual says, in the previous table, that isync is required both before and after mtsr. So, did I miss something at that time ? I can't remember but for sure we have been in this situation from the begining, and nobody has reported any problem with that yet. So not sure what to do here, as it may have an impact on performance. Will handle it in a followup patch if deamed necessary. > >> It is almost mature, performs code analysis for all PPC32. >> >> In this version objtool switch table lookup has been enhanced to >> handle nested switch tables. >> >> Most object files are correctly decoded, only a few >> 'unreachable instruction' warnings remain due to more complex >> fonctions which include back and forth jumps or branches. >> >> It allowed to detect some UACCESS mess in a few files. They've been >> fixed through other patches. >> >> Changes in v4: >> - Split series in two parts, the powerpc uaccess rework is submitted >> separately, see >> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=363368&state=* >> - Support of UACCESS on all PPC32 including book3s/32 which was missing in >> v3. >> - More elaborated switch tables lookup. >> - Patches 2, 7, 8, 9, 10, 11 are new >> - Patch 11 in series v3 is now removed. > > The patches look eminently reasonable to me; Josh, could you please have > a look?