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

[Xin Li <xin@zytor.com>]

On 11/26/2024 10:45 AM, 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 become 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.

Hi Andrew,

Do you have any more comments?

Thanks!
     Xin

> 
> Do the same on 32-bit kernel as well.
> 
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> ---
> 
> Changes since v3:
> * Rename usrseg() to fixup_rpl() to match its intent (Andrew Cooper).
> * A few comment improvements (Andrew Cooper).
> 
> 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 | 62 +++++++++++++++++++++++++------------
>   1 file changed, 43 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
> index ef654530bf5a..1e275268d256 100644
> --- a/arch/x86/kernel/signal_32.c
> +++ b/arch/x86/kernel/signal_32.c
> @@ -33,25 +33,55 @@
>   #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 become zero.  Before FRED
> + * there is nothing software 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 fixup_rpl(u16 sel)
> +{
> +	return sel <= 3 ? sel : sel | 3;
> +}
> +
>   #ifdef CONFIG_IA32_EMULATION
>   #include <asm/unistd_32_ia32.h>
>   
>   static inline void reload_segments(struct sigcontext_32 *sc)
>   {
> -	unsigned int cur;
> +	u16 cur;
>   
> +	/*
> +	 * 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.
> +	 */
>   	savesegment(gs, cur);
> -	if ((sc->gs | 0x03) != cur)
> -		load_gs_index(sc->gs | 0x03);
> +	if (fixup_rpl(sc->gs) != cur)
> +		load_gs_index(fixup_rpl(sc->gs));
>   	savesegment(fs, cur);
> -	if ((sc->fs | 0x03) != cur)
> -		loadsegment(fs, sc->fs | 0x03);
> +	if (fixup_rpl(sc->fs) != cur)
> +		loadsegment(fs, fixup_rpl(sc->fs));
> +
>   	savesegment(ds, cur);
> -	if ((sc->ds | 0x03) != cur)
> -		loadsegment(ds, sc->ds | 0x03);
> +	if (fixup_rpl(sc->ds) != cur)
> +		loadsegment(ds, fixup_rpl(sc->ds));
>   	savesegment(es, cur);
> -	if ((sc->es | 0x03) != cur)
> -		loadsegment(es, sc->es | 0x03);
> +	if (fixup_rpl(sc->es) != cur)
> +		loadsegment(es, fixup_rpl(sc->es));
>   }
>   
>   #define sigset32_t			compat_sigset_t
> @@ -105,18 +135,12 @@ static bool ia32_restore_sigcontext(struct pt_regs *regs,
>   	regs->orig_ax = -1;
>   
>   #ifdef CONFIG_IA32_EMULATION
> -	/*
> -	 * 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.
> -	 */
>   	reload_segments(&sc);
>   #else
> -	loadsegment(gs, sc.gs);
> -	regs->fs = sc.fs;
> -	regs->es = sc.es;
> -	regs->ds = sc.ds;
> +	loadsegment(gs, fixup_rpl(sc.gs));
> +	regs->fs = fixup_rpl(sc.fs);
> +	regs->es = fixup_rpl(sc.es);
> +	regs->ds = fixup_rpl(sc.ds);
>   #endif
>   
>   	return fpu__restore_sig(compat_ptr(sc.fpstate), 1);
> 
> base-commit: 6ff908de1eafb53f31db75d929b7566a87847d2d