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;
Message-ID: <80f2045b-f276-e127-8e46-87fb6994fb41@suse.com>
Date:   Wed, 7 Jun 2023 15:19:26 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 3/3] x86: Disable running 32bit processes if ia32_disabled
 is passed
Content-Language: en-US
To:     Thomas Gleixner <tglx@linutronix.de>, x86@kernel.org
Cc:     linux-kernel@vger.kernel.org, mhocko@suse.com, jslaby@suse.cz
References: <20230607072936.3766231-1-nik.borisov@suse.com>
 <20230607072936.3766231-4-nik.borisov@suse.com> <87legvjxat.ffs@tglx>
From:   Nikolay Borisov <nik.borisov@suse.com>
In-Reply-To: <87legvjxat.ffs@tglx>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bWNyR0liMHRpRGxNU1E0QkFNdlRMTlRTQlRhYVBraG9JZDRFbmE2a1ZEemho?=
 =?utf-8?B?dmF4UXFWMUdWRW1IcCsxR2ZmeGxCR0c3Umx6U0RWRlExWHZrOVBmRmo4emp1?=
 =?utf-8?B?aXNQZWFzMjBXMDZQOUhGQXRYT0pEYkFmTWNrMWc3b1JsREU0RXlKcnJHZzZn?=
 =?utf-8?B?THgyak5yazZTaDZMNThVVitxKy9Xc2E2MGh2TUh4cVF2a012eUlrZXNBaVFn?=
 =?utf-8?B?V3pVaUU3MUh6bi84NkFuUUxKalhKdm42ZHZZNjVWNlRITUlLU3l5bWNKRksv?=
 =?utf-8?B?TnEwMmxncGFyUDNZQlVsMXBSL2FWL3hVeE9sWUZkWXBGNUduV2pVNGlZbFNh?=
 =?utf-8?B?VTVrVDMyUXRBUnRXK2RvSjluellKZXBlYmloOURsaGJ4bW1lTVFHRnQrUkcv?=
 =?utf-8?B?OEhQc3phYjVaMENwdklFa2JKZ2dRVTJ1dVpQOFZTQWxKN2c1SitUbXVORDVI?=
 =?utf-8?B?elBEaWVmb1BBYUlZU01TZGo4Z2ZTU2lJTXB5ZEtNL053b2FuZGVlVFB6cUtx?=
 =?utf-8?B?UzdLZW9tNm5DUW05bmR1QlVrRkY5OHVJczdBemxsYVNwb1c3RU15TG02ZE9u?=
 =?utf-8?B?b0lMbEpNU0JHRzZEOXNFWWFHeDVmVEVvSkg3M2FYUzEyTjZzancyT3llOUMv?=
 =?utf-8?B?YjZmSE5NNWVCdlBRM1p4OTEwNVBhT0VsOGdjRWZjeDBIZnllUVJBeFpWNVBQ?=
 =?utf-8?B?dWdYWS80a2NaV1RoZExxVDVLbTJVOVJBWDRibUh2V2gvaXllVmNRQ2FZbFY2?=
 =?utf-8?B?SzVKd1E5dzhSNzF1SzVHazRNTHBRRlEyRUhHUG9JemV2SHZmRW9HSDB3QVJq?=
 =?utf-8?B?SmVKUC8rbXNuQ0M0ajNqOVRubU1oM1FxQ29OSGVreTNnSmhEOFpQRlY2emxO?=
 =?utf-8?B?bE1PL0Q3d2duekpGRXpaQW5SVlRHU0RncUtNc1cwL3RzM2JvM3EvZW1saVBB?=
 =?utf-8?B?QzArcS9NUlMrOWx0dmVyM1g3NmV3M3JnLzFuVGhBTkg0c095SUU5ZlM4RVht?=
 =?utf-8?B?ZWJUMkNXOWljc2I4aFRyV2plS1FoTm9GNElkWnZvY3RJKzlJQUVuaVIxWXBx?=
 =?utf-8?B?MFlKcTRlOFhURWVRU2s3YkZMRWhvTjBxWUI3UHFBcStzTkJMZ0V2QWJZaGN2?=
 =?utf-8?B?dDVzc0dTdUdyWXVvem4vQ2QzbC9YZmo2Vzg3cVZWWEl6cHZEMEM1bDdwbnRU?=
 =?utf-8?B?b2wyWmlibVhvclhRVzZ3RHBzbko2cDNoWkQyNFJpTXpERENsaEJLTEtBMzNu?=
 =?utf-8?B?WmNrcFowWEFMTnRsOVZBR2o0bytJQ3dzQVJSZHd6Zjd0N2ZuRGd0QVNGZVRm?=
 =?utf-8?B?RTlybUhwUExkRmRIMFQrR3g4ZEREWklRNTNMUjVFTnFMcWQxVHBmWitXRzJQ?=
 =?utf-8?B?aXdTV2FEYTNWRXRodktsMDVnZkdEbVhodEJlcjVHV2Z6U1p4TUVFYUx4bURT?=
 =?utf-8?B?QzN5VTJobzNVcHNhd2RjMGFaUjl0MlJCSUJnYkdTZWpvRHp3L0oxcW9iMzU1?=
 =?utf-8?B?dWowKzcvZzZWZXAwOWpHSXVOVHNEeFgrNWNxbkZhZlUxeWdqMmhXVUtpUXdV?=
 =?utf-8?B?TUliek83bmUreFNoQWtKVnRWUnRSYlFNM2V6T1l2NTBqZmMvZXR3Qlkxenp4?=
 =?utf-8?B?aTBhbGZ2eDhyVmlLY296eTREaWszaTltcVFKZVVvbEh2UXZQamx0NUdlbXJT?=
 =?utf-8?B?UFlrYXZGQTNEUWpvSjBReCt1VVdNaGV3VW05aEpkeDlkdytNSHNiUFEvdnhh?=
 =?utf-8?B?ekVDRmkzV3NDYnhSTmh2OER2NUd5c3pWVFNjSXU2VTFDdDFwTEVCQWJxcmN1?=
 =?utf-8?B?OWs5MDNSSGhYZUh0US9sQ01pcjhiRi9CbzFlU3ZmS1lHS2JYbWo1MlA1VlFj?=
 =?utf-8?B?Ylh5QTJpWERQSVF5eURsZzFhSDJPL1JNakEwQk5qSVdTd2NkMmVYcmZ6THY2?=
 =?utf-8?B?b2YyRS9wZnM0WnQ5blRrelBZVkhjL09USllRQlVaQzlUVEFCSzhZN0xkZGg1?=
 =?utf-8?B?STRRVExZLy80aDJYRnhYS1BtVmM2bWpHZkxRb3Y4QjNFL2JEcHFjQTJZUDho?=
 =?utf-8?B?RnJSYU9BK256d1VHdE5QMFh3Q2hwU0tIcHVQZWt3endwU2tVa1dwRDJBcDRN?=
 =?utf-8?B?c1Zmd0FDTHJCZmRmV05GbnNzYVlDbEh5a1ZxNUNMSUZHeFpFemtZbmhrWXZ0?=
 =?utf-8?Q?8BgQbtjqjZ5+q8trmA/gr8coobMDmLsescglA64hdmTs?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4cb36886-ba1a-4aaa-d881-08db675170ef
X-MS-Exchange-CrossTenant-AuthSource: PA4PR04MB7790.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Jun 2023 12:19:28.9533
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: IlaPikTkdH8qwnY4C/vvyF8E+vwHGWDDNa5+s12ZOAHbuu/eJIhUKhs1ONnx9acxb5dO7om9TszcOa1BrDqwsw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB8056
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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
>