linux-kernel - Re: [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs

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: <f205da1c-db33-299c-5fc6-922a8ebd1983@suse.com>
Date:   Mon, 22 Aug 2022 07:17:40 +0200
From:   Juergen Gross <jgross@...e.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     xen-devel@...ts.xenproject.org, x86@...nel.org,
        linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>, stable@...r.kernel.org
Subject: Re: [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs

On 21.08.22 23:41, Borislav Petkov wrote:
> On Sun, Aug 21, 2022 at 02:25:59PM +0200, Borislav Petkov wrote:
>>> Fix that by using percpu variables for saving the MSR contents.
>>>
>>> Cc: stable@...r.kernel.org
>>> Signed-off-by: Juergen Gross <jgross@...e.com>
>>> ---
>>> I thought adding a "Fixes:" tag for the kernel's initial git commit
>>> would maybe be entertaining, but without being really helpful.
>>> The percpu variables were preferred over on-stack ones in order to
>>> avoid more code churn in followup patches decoupling PAT from MTRR
>>> support.
>>
>> So if that thing has been broken for so long and no one noticed, we
>> could just as well not backport to stable at all...
> 
> Yeah, you can't do that.
> 
> The whole day today I kept thinking that something's wrong with this
> here. As in, why hasn't it been reported until now.
> 
> You say above:
> 
> "... for all cpus is racy in case the MSR contents differ across cpus."
> 
> But they don't differ:
> 
> "7.7.5 MTRRs in Multi-Processing Environments
> 
> In multi-processing environments, the MTRRs located in all processors
> must characterize memory in the same way. Generally, this means that
> identical values are written to the MTRRs used by the processors. This
> also means that values CR0.CD and the PAT must be consistent across
> processors. Failure to do so may result in coherency violations or loss
> of atomicity. Processor implementations do not check the MTRR settings
> in other processors to ensure consistency. It is the responsibility of
> system software to initialize and maintain MTRR consistency across all
> processors."
> 
> And you can't have different fixed MTRR type on each CPU - that would
> lead to all kinds of nasty bugs.
> 
> And here's from a big fat box:
> 
> $ rdmsr -a 0x2ff | uniq -c
>      256 c00
> 
> All 256 CPUs have the def type set to the same thing.
> 
> Now, if all CPUs go write that same deftype_lo variable in the
> rendezvous handler, the only issue that could happen is if a read
> sees a partial write. BUT, AFAIK, x86 doesn't tear 32-bit writes so I
> *think* all CPUs see the same value being corrected by using mtrr_state
> previously saved on the BSP.
> 
> As I said, we should've seen this exploding left and right otherwise...

And then there is mtrr_state_warn() in arch/x86/kernel/cpu/mtrr/generic.c
which has a comment saying:

/* Some BIOS's are messed up and don't set all MTRRs the same! */

Yes, the chances are slim to hit such a box, but your reasoning suggests
I should remove the related code?


Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3099 bytes)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (496 bytes)