Re: [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true

[Nikolay Borisov]

On 9.06.23 г. 18:22 ч., Thomas Gleixner wrote:
> On Fri, Jun 09 2023 at 14:13, Nikolay Borisov wrote:
> > First stage of disabling ia32 compat layer is to disable 32bit syscall
> > entry points. Legacy int 0x80 vector is disabled by zeroing out its gate
> > descriptor in the idt and the sysenter vector is disabled by re-using
> > the existing code in case IA32_EMULATION is disabled.
>
> This describes WHAT the patch does without providing any context.
>
> > +	if ((IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) ||
> > +	    !IS_ENABLED(CONFIG_IA32_EMULATION)) {
>
> I told you before that my brain based compiler complains about your
> patches not building with CONFIG_IA32_EMULATION=n. The above still fails
> to build.

Yes, it does. My bad.

> Aside of that this condition is convoluted and can be simplified to
> exactly a simple and understandable
>
>          if (foo)
>
> which is actually the obvious solution to make it compile in all
> configurations.

I fail to see how this can be done the way you suggest given that 
ia32_disabled is visible iff IA32_EMULATION is selected, this means an 
#ifdef is mandatory so that ia32_disabled is checked when we know it 
will exist as a symbol, the same applies for the entry_SYSCALL_compat 
symbol which has to be used iff IA32_EMULATION is defined. I.e the 
ignore code should also be duplicated in the #ifdef IA32_EMULATION && 
ia32_disabled and in the #else  condition.

> It's not too much asked to flip the config switch which affects the code
> you are changing for a test.

Sorry, missed it the first time.

> > @@ -226,6 +226,13 @@ void __init idt_setup_early_traps(void)
> >   void __init idt_setup_traps(void)
> >   {
> >   	idt_setup_from_table(idt_table, def_idts, ARRAY_SIZE(def_idts), true);
> > +
> > +	if (IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) {
>
> Ditto.

This actually doesn't fail, because if IA32_EMULATION is n that 
conditional expands to 'if (0 && ia32_disabled)' which is eliminated by 
the compiler.

> > +		gate_desc null_desc = {};
>
> Lacks a newline between declaration and code. It's documented to be
> required, no?
>
> > +		write_idt_entry(idt_table, IA32_SYSCALL_VECTOR, &null_desc);
> > +		clear_bit(IA32_SYSCALL_VECTOR, system_vectors);
> > +	}
>
> That aside, I asked you to split IA32_SYSCALL_VECTOR out of def_idts[]
> and handle it separately, no? If you disagree with me then reply to my
> review first instead of ignoring me silently.

I tried doing this but it's no go since def_its is defined statically. 
Since tha IA32_SYSCALL_VECTOR is the last one it can't simply be tacked 
at the end of this array in a separate place. Hence the only viable 
solution ( apart from making def_its a dynamically sized array) was to 
simply overwrite IA32_SYSCALL_VECTOR in idt_table before it's being 
loaded into the ldtr.

<snip>