linux-kernel - Re: [PATCH v2 7/7] x86/syscall/32: Add comment to conditional

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <B6BD4B23-1EF8-48CD-A668-BA1D5BA7489E@zytor.com>
Date: Fri, 14 Mar 2025 11:30:57 -0700
From: "H. Peter Anvin" <hpa@...or.com>
To: Brian Gerst <brgerst@...il.com>, Sohil Mehta <sohil.mehta@...el.com>
CC: Ingo Molnar <mingo@...nel.org>, Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>, Andy Lutomirski <luto@...nel.org>,
        Juergen Gross <jgross@...e.com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        X86-kernel <x86@...nel.org>,
        LKML Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 7/7] x86/syscall/32: Add comment to conditional

On March 14, 2025 10:43:34 AM PDT, Brian Gerst <brgerst@...il.com> wrote:
>On Fri, Mar 14, 2025 at 1:19 PM Sohil Mehta <sohil.mehta@...el.com> wrote:
>>
>> On 3/14/2025 8:12 AM, Brian Gerst wrote:
>> > Add a CONFIG_X86_FRED comment, since this conditional is nested.
>> >
>> > Signed-off-by: Brian Gerst <brgerst@...il.com>
>> > Suggested-by: Sohil Mehta <sohil.mehta@...el.com>
>> > ---
>> >  arch/x86/entry/syscall_32.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/x86/entry/syscall_32.c b/arch/x86/entry/syscall_32.c
>> > index 993d72504fc5..2b15ea17bb7c 100644
>> > --- a/arch/x86/entry/syscall_32.c
>> > +++ b/arch/x86/entry/syscall_32.c
>> > @@ -238,7 +238,8 @@ DEFINE_FREDENTRY_RAW(int80_emulation)
>> >       instrumentation_end();
>> >       syscall_exit_to_user_mode(regs);
>> >  }
>> > -#endif
>> > +#endif /* CONFIG_X86_FRED */
>> > +
>> >  #else /* CONFIG_IA32_EMULATION */
>> >
>> >  /* Handles int $0x80 on a 32bit kernel */
>>
>> Also, there seem to be a couple of adjacent CONFIG_IA32_EMULATION
>> sections in the file. It might be better to bring them together under
>> one section in this patch or a follow-up. Something like below:
>>
>> > diff --git a/arch/x86/entry/syscall_32.c b/arch/x86/entry/syscall_32.c
>> > index 2b15ea17bb7c..a84f9d3c1695 100644
>> > --- a/arch/x86/entry/syscall_32.c
>> > +++ b/arch/x86/entry/syscall_32.c
>> > @@ -57,16 +57,6 @@ static __always_inline int syscall_32_enter(struct pt_regs *regs)
>> >         return (int)regs->orig_ax;
>> >  }
>> >
>> > -#ifdef CONFIG_IA32_EMULATION
>> > -bool __ia32_enabled __ro_after_init = !IS_ENABLED(CONFIG_IA32_EMULATION_DEFAULT_DISABLED);
>> > -
>> > -static int __init ia32_emulation_override_cmdline(char *arg)
>> > -{
>> > -       return kstrtobool(arg, &__ia32_enabled);
>> > -}
>> > -early_param("ia32_emulation", ia32_emulation_override_cmdline);
>> > -#endif
>> > -
>> >  /*
>> >   * Invoke a 32-bit syscall.  Called with IRQs on in CT_STATE_KERNEL.
>> >   */
>> > @@ -87,6 +77,14 @@ static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs, int nr)
>> >  }
>> >
>> >  #ifdef CONFIG_IA32_EMULATION
>> > +bool __ia32_enabled __ro_after_init = !IS_ENABLED(CONFIG_IA32_EMULATION_DEFAULT_DISABLED);
>> > +
>> > +static int __init ia32_emulation_override_cmdline(char *arg)
>> > +{
>> > +       return kstrtobool(arg, &__ia32_enabled);
>> > +}
>> > +early_param("ia32_emulation", ia32_emulation_override_cmdline);
>> > +
>> >  static __always_inline bool int80_is_external(void)
>> >  {
>> >         const unsigned int offs = (0x80 / 32) * 0x10;
>
>The 32-bit syscall code is a bit of a mess right now, and it was hard
>to see its state while it was lumped in with everything else in
>common.c.  I'd like to see the INT80 code merged back into one
>function, but that will probably have to wait until the dust settles
>with FRED.
>
>
>Brian Gerst
>

That function gets really, really ugly. The reason we factored out the FRED code is that the number of conditionals ended up grotesque, *and* that they are part of completely separate flows – they should have separate every points, not dynamic conditionals.