linux-kernel - Re: [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32

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 for Android: free password hash cracker in your pocket

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

Message-ID: <599dea2f-f158-fd67-2c62-d5372d1d18a0@suse.com>
Date:   Fri, 9 Jun 2023 19:03:59 +0300
From:   Nikolay Borisov <nik.borisov@...e.com>
To:     Thomas Gleixner <tglx@...utronix.de>, x86@...nel.org
Cc:     linux-kernel@...r.kernel.org, mhocko@...e.com, jslaby@...e.cz
Subject: Re: [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled
 is true

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>