| Message ID | cover.1689091394.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| Headers | show |
| Series | powerpc/objtool: uaccess validation for PPC32 (v4) | expand |
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? > 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?
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?
On Tue, 11 Jul 2023 18:08:26 +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=* > > It is almost mature, performs code analysis for all PPC32. > > [...] Applied to powerpc/fixes. [01/15] Revert "powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto" https://git.kernel.org/powerpc/c/b49e578b9314db051da0ad72bba24094193f9bd0 cheers
Michael Ellerman <patch-notifications@ellerman.id.au> writes: > On Tue, 11 Jul 2023 18:08:26 +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=* >> >> It is almost mature, performs code analysis for all PPC32. >> >> [...] > > Applied to powerpc/fixes. > > [01/15] Revert "powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto" > https://git.kernel.org/powerpc/c/b49e578b9314db051da0ad72bba24094193f9bd0 Sorry that's b4 getting confused, I actually applied the v5 that I sent: https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20230712134552.534955-1-mpe@ellerman.id.au/ cheers
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=* 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. Changes in v3: - Rebased on top of a merge of powerpc tree and tip/objtool/core tree - Simplified support for relative switch tables based on relocation type - Taken comments from Peter Christophe Leroy (15): Revert "powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto" objtool: Move back misplaced comment objtool: Allow an architecture to disable objtool on ASM files objtool: Fix JUMP_ENTRY_SIZE for bi-arch like powerpc objtool: Add INSN_RETURN_CONDITIONAL objtool: Add support for relative switch tables objtool: Merge mark_func_jump_tables() and add_func_jump_tables() objtool: Track general purpose register used for switch table base objtool: Find end of switch table directly objtool: When looking for switch tables also follow conditional and dynamic jumps objtool: .rodata.cst{2/4/8/16} are not switch tables objtool: Add support for more complex UACCESS control objtool: Prepare noreturns.h for more architectures powerpc/bug: Annotate reachable after warning trap powerpc: Implement UACCESS validation on PPC32 arch/Kconfig | 5 + arch/powerpc/Kconfig | 2 + arch/powerpc/include/asm/book3s/32/kup.h | 2 + arch/powerpc/include/asm/book3s/64/kup.h | 2 +- arch/powerpc/include/asm/bug.h | 77 ++------- arch/powerpc/include/asm/nohash/32/kup-8xx.h | 4 +- arch/powerpc/include/asm/nohash/kup-booke.h | 4 +- arch/powerpc/kernel/misc_32.S | 2 +- arch/powerpc/kernel/traps.c | 9 +- arch/powerpc/kexec/core_32.c | 4 +- arch/powerpc/mm/nohash/kup.c | 2 + include/linux/objtool.h | 14 ++ scripts/Makefile.build | 4 + tools/objtool/arch/powerpc/decode.c | 155 +++++++++++++++++- .../arch/powerpc/include/arch/noreturns.h | 11 ++ .../arch/powerpc/include/arch/special.h | 2 +- tools/objtool/arch/powerpc/special.c | 39 ++++- .../objtool/arch/x86/include/arch/noreturns.h | 20 +++ tools/objtool/arch/x86/special.c | 8 +- tools/objtool/check.c | 154 ++++++++++++----- tools/objtool/include/objtool/arch.h | 1 + tools/objtool/include/objtool/check.h | 6 +- tools/objtool/include/objtool/special.h | 3 +- tools/objtool/noreturns.h | 14 +- tools/objtool/special.c | 55 +++---- 25 files changed, 425 insertions(+), 174 deletions(-) create mode 100644 tools/objtool/arch/powerpc/include/arch/noreturns.h create mode 100644 tools/objtool/arch/x86/include/arch/noreturns.h