Re: [PATCH v4 00/15] powerpc/objtool: uaccess validation for PPC32 (v4)

[Christophe Leroy]

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?