Re: [PATCH v3 1/1] x86/ia32: Leave NULL selector values 0~3 as is

[Xin Li <xin@zytor.com>]

On 11/21/2024 11:43 AM, Andrew Cooper wrote:
> On 21/11/2024 5:54 pm, Xin Li (Intel) wrote:
>> As such, leave NULL selector values 0~3 as is.
>>
>> Do the same on 32-bit kernel as well.
>>
>> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> 
> As far as fixing up RPL goes, I think the patch is fine, and probably
> wants to be taken in roughly this form (new minor points below).
> 
> However, the pre-existing code is doing something entirely bizarre,
> which warrants further investigation, and maybe fixes.
> 
>> + * a nonzero NULL selector and waiting for it to drop to zero.
> 
> I know I wrote "drop to zero", but in hindsight, I think "become zero"
> would be better.

Sure.  They both look good to me, but I'm not a native English speaker,
so it doesn't count :-P.

> 
>>    Before FRED
>> + * there is nothing we can do to prevent such an information leak.
>> + *
>> + * ERETU, the only legit instruction to return to userspace from kernel
>> + * under FRED, by design does NOT zero any segment register to avoid this
>> + * problem behavior.
>> + *
>> + * As such, leave NULL selector values 0~3 as is.
>> + */
>> +static inline u16 usrseg(u16 sel)
> 
> I would suggest naming this fixup_rpl() which is a bit clearer as to its
> intent.

The rename makes sense.

> 
> However, I would also recommend u32 (or at least, unsigned int).
> 
> It's absolutely marginal, but you do get better code generation by
> avoiding u16 specifically where possible.
> 
> https://godbolt.org/z/MnnvW461f

Oh, you created a live sample, I appreciate it!

> 
>> +{
>> +	return sel <= 3 ? sel : sel | 3;
>> +}
>> +
>>   #ifdef CONFIG_IA32_EMULATION
>>   #include <asm/unistd_32_ia32.h>
>>   
>> @@ -41,17 +64,17 @@ static inline void reload_segments(struct sigcontext_32 *sc)
>>   	unsigned int cur;
>>   
>>   	savesegment(gs, cur);
>> -	if ((sc->gs | 0x03) != cur)
>> -		load_gs_index(sc->gs | 0x03);
>> +	if (usrseg(sc->gs) != cur)
>> +		load_gs_index(usrseg(sc->gs));
>>   	savesegment(fs, cur);
>> -	if ((sc->fs | 0x03) != cur)
>> -		loadsegment(fs, sc->fs | 0x03);
>> +	if (usrseg(sc->fs) != cur)
>> +		loadsegment(fs, usrseg(sc->fs));
>>   	savesegment(ds, cur);
>> -	if ((sc->ds | 0x03) != cur)
>> -		loadsegment(ds, sc->ds | 0x03);
>> +	if (usrseg(sc->ds) != cur)
>> +		loadsegment(ds, usrseg(sc->ds));
>>   	savesegment(es, cur);
>> -	if ((sc->es | 0x03) != cur)
>> -		loadsegment(es, sc->es | 0x03);
>> +	if (usrseg(sc->es) != cur)
>> +		loadsegment(es, usrseg(sc->es));
>>   }
>>   
>>   #define sigset32_t			compat_sigset_t
>> @@ -113,10 +136,10 @@ static bool ia32_restore_sigcontext(struct pt_regs *regs,
>>   	 */
>>   	reload_segments(&sc);
> 
> This is the singular caller of reload_segments(), and the comment out of
> context does not match the implementation.
> 
> It probably wants inlining just so all the segment juggling is in one place.

So move the comment (C&P below) above invoking reload_segments(&sc) into
the function definition?

	/*
	 * Reload fs and gs if they have changed in the signal
	 * handler.  This does not handle long fs/gs base changes in
	 * the handler, but does not clobber them at least in the
	 * normal case.
	 */

> 
>>   #else
>> -	loadsegment(gs, sc.gs);
>> -	regs->fs = sc.fs;
>> -	regs->es = sc.es;
>> -	regs->ds = sc.ds;
>> +	loadsegment(gs, usrseg(sc.gs));
>> +	regs->fs = usrseg(sc.fs);
>> +	regs->es = usrseg(sc.es);
>> +	regs->ds = usrseg(sc.ds);
>>   #endif
> 
> Why is GS handled specially?
> 
> Both, 1) Why is regs->gs the only value that doesn't an RPL-adjusted
> value, and 2) why do we need to reload it here?  We need to keep it as
> the per_cpu pointer anyway, and we're going to reload on exit-to-user,
> aren't we?

> Also, why do we have such wildly-different behaviours depending on
> IA32_EMULATION or not?

Maybe because 32-bit exit code skips popping gs?

And 64-bit exit code doesn't load segment registers as 32-bit does.

Thanks!
     Xin