Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Eric W. Biederman" <ebiederm@xmission.com>
To:     Xin Li <xin3.li@intel.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        hpa@zytor.com, brgerst@gmail.com
References: <20230706052231.2183-1-xin3.li@intel.com>
Date:   Thu, 06 Jul 2023 09:05:22 -0500
In-Reply-To: <20230706052231.2183-1-xin3.li@intel.com> (Xin Li's message of
        "Wed, 5 Jul 2023 22:22:31 -0700")
Message-ID: <87v8exgmot.fsf@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH] x86/ia32: Do not modify the DPL bits for a null selector
Precedence: bulk

Xin Li <xin3.li@intel.com> writes:

> When a null selector is to be loaded into a segment register,
> reload_segments() sets its DPL bits to 3. Later when an IRET
> instruction loads it, it zeros the segment register. The two
> operations offset each other to actually effect a nop.
>
> Fix it by not modifying the DPL bits for a null selector.

Maybe this is the right thing but this needs some serious comments
about what is going on.

In particular how does sel <= 3 equate to a null selector?  Is that
defined somewhere?  At a minimum you should have static asserts to make
certain no one redefines the first 4 segment selectors as anything else,
if you want to refer to them by number instead of testing for specific
properties.

As written this looks like it requires an enormous amount of knowledge
about how other parts of the code works, to be comprehensible or to
change safely.  That level of non-local knowledge should be unnecessary.

Eric


> Signed-off-by: Xin Li <xin3.li@intel.com>
> ---
>  arch/x86/kernel/signal_32.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
> index 9027fc088f97..7796cf84fca2 100644
> --- a/arch/x86/kernel/signal_32.c
> +++ b/arch/x86/kernel/signal_32.c
> @@ -36,22 +36,27 @@
>  #ifdef CONFIG_IA32_EMULATION
>  #include <asm/ia32_unistd.h>
>  
> +static inline u16 usrseg(u16 sel)
> +{
> +	return sel <= 3 ? sel : sel | 3;
> +}
> +
>  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