Re: [PATCH 3/3] x86: Disable running 32bit processes if ia32_disabled is passed

[Nikolay Borisov]

On 7.06.23 г. 15:01 ч., Thomas Gleixner wrote:
> On Wed, Jun 07 2023 at 10:29, Nikolay Borisov wrote:
> > In addition to disabling 32bit syscall interface let's also disable the
> > ability to run 32bit processes altogether. This is achieved by setting
> > the GDT_ENTRY_DEFAULT_USER32_CS descriptor to not present which would
> > cause 32 bit processes to trap with a #NP exception. Furthermore,
> > forbid loading compat processes as well.
>
> This is obviously the wrong order of things. Prevent loading of compat
> processes is the first step, no?

You mean to change the sequence in which those things are mentioned in 
the log?

> >   
> > +extern bool ia32_disabled;
> >   #define compat_elf_check_arch(x)					\
>
> So in patch 1 you add the declaration with #ifdef guards and now you add
> another one without. Fortunately this is the last patch otherwise we'd
> might end up with another incarnation in the next header file.

My bad, will fix it.

> > -	(elf_check_arch_ia32(x) ||					\
> > -	 (IS_ENABLED(CONFIG_X86_X32_ABI) && (x)->e_machine == EM_X86_64))
> > +	(!ia32_disabled && (elf_check_arch_ia32(x) ||			\
> > +	 (IS_ENABLED(CONFIG_X86_X32_ABI) && (x)->e_machine == EM_X86_64)))
>
> If I'm reading this correctly then ia32_disabled also prevents binaries
> with X32 ABI to be loaded.
>
> That might be intentional but I'm failing to find any explanation for
> this in the changelog.
>
> X32_ABI != IA32_EMULATION

Right, however given the other changes (i.e disabling sysenter/int 0x80) 
can we really have a working X32 abi when ia32_disabled is true? Now I'm 
thinking can we really have IA32_EMULATION && X32_ABI && ia32_disabled, 
I guess the answer is no?

> >   static inline void elf_common_init(struct thread_struct *t,
> >   				   struct pt_regs *regs, const u16 ds)
> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index 71f8b55f70c9..ddc301c09419 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -2359,6 +2359,11 @@ void microcode_check(struct cpuinfo_x86 *prev_info)
> >   }
> >   #endif
> >   
> > +static void remove_user32cs_from_gdt(void * __unused)
> > +{
> > +	get_current_gdt_rw()[GDT_ENTRY_DEFAULT_USER32_CS].p = 0;
> > +}
> > +
> >   /*
> >    * Invoked from core CPU hotplug code after hotplug operations
> >    */
> > @@ -2368,4 +2373,7 @@ void arch_smt_update(void)
> >   	cpu_bugs_smt_update();
> >   	/* Check whether IPI broadcasting can be enabled */
> >   	apic_smt_update();
> > +	if (ia32_disabled)
> > +		on_each_cpu(remove_user32cs_from_gdt, NULL, 1);
> > +
> >   }
>
> This issues a SMP function call on all online CPUs to set these entries
> to 0 on _every_ CPU hotplug operation.
>
> I'm sure there is a reason why these bits need to be cleared over and
> over. It's just not obvious to me why clearing them _ONCE_ per boot is
> not sufficient. It's neither clear to me why CPU0 must do that ($NCPUS -
> 1) times, but for the last CPU it's enough to do it once.

Actually clearing them once per-cpu is perfectly fine. Looking around 
the code i saw arch_smt_update() to be the only place where a 
on_each_cpu() call is being made hence I put the code there. Another 
aspect I was thinking of is what if a cpu gets onlined at a later stage 
and a 32bit process is scheduled on that cpu, if the gdt entry wasn't 
cleared on that CPU then it would be possible to run 32bit processes on 
it. I guess a better alternative is to use arch_initcall() ?

> That aside, what's the justification for doing this in the first place
> and why is this again inconsistent vs. CONFIG_IA32_EMULATION=n?

I'll put it under an ifdef CONFIG_IA32_EMULATION, unfortunately the 
traps.h header can't be included in elf.h without causing build breakage.

> The changelog is full of void in that aspect.
>
> Thanks,
>
>          tglx
>