From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7126AEB64D9 for ; Fri, 7 Jul 2023 22:52:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230166AbjGGWwm (ORCPT ); Fri, 7 Jul 2023 18:52:42 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59800 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229629AbjGGWwj (ORCPT ); Fri, 7 Jul 2023 18:52:39 -0400 Received: from out01.mta.xmission.com (out01.mta.xmission.com [166.70.13.231]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 44036B9 for ; Fri, 7 Jul 2023 15:52:38 -0700 (PDT) Received: from in01.mta.xmission.com ([166.70.13.51]:47910) by out01.mta.xmission.com with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1qHuJW-00GDhM-90; Fri, 07 Jul 2023 16:52:34 -0600 Received: from ip68-110-29-46.om.om.cox.net ([68.110.29.46]:34532 helo=email.froward.int.ebiederm.org.xmission.com) by in01.mta.xmission.com with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1qHuJV-00FYwO-6s; Fri, 07 Jul 2023 16:52:33 -0600 From: "Eric W. Biederman" To: "Li, Xin3" Cc: "linux-kernel@vger.kernel.org" , "x86@kernel.org" , "tglx@linutronix.de" , "mingo@redhat.com" , "bp@alien8.de" , "dave.hansen@linux.intel.com" , "hpa@zytor.com" , "brgerst@gmail.com" References: <20230706052231.2183-1-xin3.li@intel.com> <87v8exgmot.fsf@email.froward.int.ebiederm.org> <87sf9zfx76.fsf@email.froward.int.ebiederm.org> Date: Fri, 07 Jul 2023 17:51:49 -0500 In-Reply-To: (Xin3 Li's message of "Fri, 7 Jul 2023 22:17:36 +0000") Message-ID: <87pm53e3ne.fsf@email.froward.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1qHuJV-00FYwO-6s;;;mid=<87pm53e3ne.fsf@email.froward.int.ebiederm.org>;;;hst=in01.mta.xmission.com;;;ip=68.110.29.46;;;frm=ebiederm@xmission.com;;;spf=pass X-XM-AID: U2FsdGVkX19QTPt1VjS7X1hGX42zlzX0I1GsKs+iLg4= X-SA-Exim-Connect-IP: 68.110.29.46 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH] x86/ia32: Do not modify the DPL bits for a null selector X-SA-Exim-Version: 4.2.1 (built Sat, 08 Feb 2020 21:53:50 +0000) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org "Li, Xin3" writes: >> Perhaps something like patch below to make it clear that we are >> normalizing the segment values and forcing that normalization. > > "Normalizing" sounds a good term. > >> I am a bit confused why this code is not the same for ia32 and >> ia32_emulation. I would think the normalization at least should apply >> to the 32bit case as well. >> >> Eric >> >> diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c >> index 9027fc088f97..e5f3978388fd 100644 >> --- a/arch/x86/kernel/signal_32.c >> +++ b/arch/x86/kernel/signal_32.c >> @@ -36,22 +36,47 @@ >> #ifdef CONFIG_IA32_EMULATION >> #include >> >> +static inline unsigned int normalize_seg_index(unsigned int index) > > normalize_usrseg_index? Perhaps useg? I think that is the abbreviation I have seen used elsewhere. I was trying to get things as close as I could to the x86 documentation so people could figure out what is going on by reading the documentation. >> +{ >> + /* >> + * Convert the segment index into normalized form. >> + * >> + * For the indexes 0,1,2,3 always use the value of 0, as IRET >> + * forces this form for the nul segment. >> + * >> + * Otherwise set both DPL bits to force it to be a userspace >> + * ring 3 segment index. >> + */ >> + return (index < 3) ? 0 : index | 3; > > s/ > no? Yes. My typo. The patch was very much a suggestion on how we can perhaps write the code to make it clearer what is happening. Normalizing the segment index values seems like what we have been intending to do all along. In fact it might make sense for clarity to simply use the existing "index | 3" logic in what I called normal_seg_index, and then just update the normalization to add the null pointer case. I was just spending a little time trying to make it so that the code is readable. > With this patch it looks that 1,2,3 are no longer valid selector values > in Linux, which seems the right thing to do but I don't know if there is > any side effect. You have said that IRET always changes 1,2,3 to 0. So I don't expect the selector values of 1,2,3 will last very long. If that is not the case I misunderstood, and we should consider doing something else. It bothers me that the existing code, and your code as well only handles the normalization on x86_64 for ia32 mode. Shouldn't the same normalization logic apply in a 32bit kernel as well? Scope creep I know but the fact the code does not match seems concerning. Eric >> +} >> + >> static inline void reload_segments(struct sigcontext_32 *sc) >> { >> - unsigned int cur; >> + unsigned int new, cur; >> >> + new = normalize_seg_index(sc->gs); >> savesegment(gs, cur); >> - if ((sc->gs | 0x03) != cur) >> - load_gs_index(sc->gs | 0x03); >> + cur = normalize_seg_index(cur); >> + if (new != cur) >> + load_gs_index(new); >> + >> + new = normalize_seg_index(sc->fs); >> savesegment(fs, cur); >> - if ((sc->fs | 0x03) != cur) >> - loadsegment(fs, sc->fs | 0x03); >> + cur = normalize_seg_index(cur); >> + if (new != cur) >> + loadsegment(fs, new); >> + >> + new = normalize_seg_index(sc->ds); >> savesegment(ds, cur); >> - if ((sc->ds | 0x03) != cur) >> - loadsegment(ds, sc->ds | 0x03); >> + cur = normalize_seg_index(cur); >> + if (new != cur) >> + loadsegment(ds, new); >> + >> + new = normalize_seg_index(sc->es); >> savesegment(es, cur); >> - if ((sc->es | 0x03) != cur) >> - loadsegment(es, sc->es | 0x03); >> + cur = normalize_seg_index(cur); >> + if (new != cur) >> + loadsegment(es, new); >> } >> >> #define sigset32_t compat_sigset_t