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

[Xin Li]

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@xxxxxxxxx>
>
> 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