| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <36970ddb-c0d8-43e4-a94e-0d9ea3d55ced@zytor.com>
Date: Thu, 13 Feb 2025 22:56:48 -0800
From: Xin Li <xin@...or.com>
To: linux-kernel@...r.kernel.org
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
andrew.cooper3@...rix.com, brgerst@...il.com, ebiederm@...ssion.com
Subject: Re: [PATCH v4 1/1] x86/ia32: Leave NULL selector values 0~3 as is
On 12/12/2024 10:44 AM, Xin Li wrote:
> 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?
Hi Andrew,
Are you okay to give a review-by to this patch?
Thanks!
Xin
>>
>> Do the same on 32-bit kernel as well.
>>
>> Signed-off-by: Xin Li (Intel) <xin@...or.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
>
>
Powered by blists - more mailing lists