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

From: Xin Li
Date: Fri Nov 22 2024 - 02:52:16 EST


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