linux-kernel - RE: [PATCH] x86/ia32: Do not modify the DPL bits for a null selector

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <SA1PR11MB6734547F70FAFC9297D89F7BA832A@SA1PR11MB6734.namprd11.prod.outlook.com>
Date:   Sat, 8 Jul 2023 08:44:14 +0000
From:   "Li, Xin3" <xin3.li@...el.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "brgerst@...il.com" <brgerst@...il.com>
Subject: RE: [PATCH] x86/ia32: Do not modify the DPL bits for a null selector

> > 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.

useg seems the right term afaict.

> >> +	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.

Your understanding is correct. And I am NOT opposed to your change,
but want to see if there is any concern from other people.

On the other side, I got to mention that when FRED is enabled, ERETU
doesn't forcibly set non-zero null selector values to 0. Then for the
sigreturn self-test, it can set DS to any of the null selector values,
and expect DS is set to the value after sigreturn.

With your proposal, 1, 2 and 3 are no longer valid selector values in
Linux, and Linux would fociably set non-zero null selector values to 0,
just like what IRET has been doing.

Then the sigreturn self-test should be changed to check any non-zero
null selector value will be set to 0 after sigreturn. And this behavior
is consistent w/ and w/o FRED.

I think we should do it.

> 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.

Agreed! We *should* fix it in the same way.

I guess people are just conservative with 32-bit kernel specific changes.

How many OSVs still release 32-bit kernel? What about the testing?