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

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

[Xin Li (Intel)]

The first GDT descriptor is reserved as 'NULL descriptor'.  As bits 0
and 1 of a segment selector, i.e., the RPL bits, are NOT used to index
GDT, selector values 0~3 all point to the NULL descriptor, thus values
0, 1, 2 and 3 are all valid NULL selector values.

When a NULL selector value is to be loaded into a segment register,
reload_segments() sets its RPL bits.  Later IRET zeros ES, FS, GS, and
DS segment registers if any of them is found to have any nonzero NULL
selector value.  The two operations offset each other to actually effect
a nop.

Besides, zeroing of RPL in NULL selector values is an information leak
in pre-FRED systems as userspace can spot any interrupt/exception by
loading a nonzero NULL selector, and waiting for it to drop to zero.
But there is nothing software can do to prevent it before FRED.

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.

Do the same on 32-bit kernel as well.

Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---

Changes since v2:
* No, don't zero non-zero NULL selector values, essentially revert
  to v1 (Andrew Cooper).

Changes since v1:
* Normalize non-zero NULL selector values to 0 (Eric W. Biederman).
* Apply the same normalization logic in a 32bit kernel (Eric W.
  Biederman).
---
 arch/x86/kernel/signal_32.c | 47 +++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
index ef654530bf5a..23879f14aa51 100644
--- a/arch/x86/kernel/signal_32.c
+++ b/arch/x86/kernel/signal_32.c
@@ -33,6 +33,29 @@
 #include <asm/smap.h>
 #include <asm/gsseg.h>
 
+/*
+ * The first GDT descriptor is reserved as 'NULL descriptor'.  As bits 0
+ * and 1 of a segment selector, i.e., the RPL bits, are NOT used to index
+ * GDT, selector values 0~3 all point to the NULL descriptor, thus values
+ * 0, 1, 2 and 3 are all valid NULL selector values.
+ *
+ * However IRET zeros ES, FS, GS, and DS segment registers if any of them
+ * is found to have any nonzero NULL selector value, which can be used by
+ * userspace in pre-FRED systems to spot any interrupt/exception by loading
+ * a nonzero NULL selector and waiting for it to drop to zero.  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)
+{
+	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);
 #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
 
 	return fpu__restore_sig(compat_ptr(sc.fpstate), 1);

base-commit: 25f324dbc68cd6ff972f5ab3bb07f1bf6200f0e0
-- 
2.47.0

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

[Andrew Cooper]

On 21/11/2024 5:54 pm, Xin Li (Intel) wrote:
> The first GDT descriptor is reserved as 'NULL descriptor'.  As bits 0
> and 1 of a segment selector, i.e., the RPL bits, are NOT used to index
> GDT, selector values 0~3 all point to the NULL descriptor, thus values
> 0, 1, 2 and 3 are all valid NULL selector values.
>
> When a NULL selector value is to be loaded into a segment register,
> reload_segments() sets its RPL bits.  Later IRET zeros ES, FS, GS, and
> DS segment registers if any of them is found to have any nonzero NULL
> selector value.  The two operations offset each other to actually effect
> a nop.
>
> Besides, zeroing of RPL in NULL selector values is an information leak
> in pre-FRED systems as userspace can spot any interrupt/exception by
> loading a nonzero NULL selector, and waiting for it to drop to zero.
> But there is nothing software can do to prevent it before FRED.
>
> 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.
>
> 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.

> diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
> index ef654530bf5a..23879f14aa51 100644
> --- a/arch/x86/kernel/signal_32.c
> +++ b/arch/x86/kernel/signal_32.c
> @@ -33,6 +33,29 @@
>  #include <asm/smap.h>
>  #include <asm/gsseg.h>
>  
> +/*
> + * The first GDT descriptor is reserved as 'NULL descriptor'.  As bits 0
> + * and 1 of a segment selector, i.e., the RPL bits, are NOT used to index
> + * GDT, selector values 0~3 all point to the NULL descriptor, thus values
> + * 0, 1, 2 and 3 are all valid NULL selector values.
> + *
> + * However IRET zeros ES, FS, GS, and DS segment registers if any of them
> + * is found to have any nonzero NULL selector value, which can be used by
> + * userspace in pre-FRED systems to spot any interrupt/exception by loading
> + * 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.

>   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.

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

> +{
> +	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.

>  #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?

~Andrew

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

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

[H. Peter Anvin]

In this case you want to keep u16, though, because you are otherwise comparing with the wrong value!