linux-kernel - Re: [PATCH v5 11/15] x86/mtrr: construct a memory map with cache modes

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: <681c5d8e-0e42-07e1-f91c-7696a2360f1c@suse.com>
Date:   Thu, 20 Apr 2023 15:57:43 +0200
From:   Juergen Gross <jgross@...e.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Michael Kelley <mikelley@...rosoft.com>
Subject: Re: [PATCH v5 11/15] x86/mtrr: construct a memory map with cache
 modes

On 20.04.23 15:01, Borislav Petkov wrote:
> On Thu, Apr 20, 2023 at 02:30:09PM +0200, Juergen Gross wrote:
>> OTOH the additional compare to 0 has costs, too, and this cost is spent for
>> ALL calls
> 
> I'll take the cost of a single
> 
> 	cmpl    %edi, %edx
> 
> for a handful of entries any day of the week.
> 
>> while the zero size call is a rather rare case.
> 
> $ grep "memmove size" /tmp/mtrr.txt
> memmove size 0
> memmove size 0
> memmove size 0
> memmove size 0
> memmove size 0
> memmove size 0
> memmove size 0
> memmove size 0
> memmove size 0
> memmove size 0
> memmove size 0
> 
> for - I admit a largely contrived map - but 5 entries only:
> 
> Current map:
> 0: start: 0x0000000000000000, end: 0x0000000000100000, type: 0x0
> 1: start: 0x0000000100000000, end: 0x0000000820000000, type: 0x6
> 2: start: 0x000050d70000c000, end: 0x000062a60000c000, type: 0x4
> 3: start: 0x000062a60000c000, end: 0x0001d6d200001000, type: 0x0
> 4: start: 0x0001d6d200001000, end: 0x0001dd8100001000, type: 0x6
> 
>> Regarding "cache_map + idx + 1 is not valid": the standard clearly points
>> out that a call with size 0 is valid and won't copy anything
> 
> That's not what I meant. Please read again what I said:
> 
> "I wouldn't want it getting prefetched from %rsi in the hw when there's
> no reason for it".

So you are suggesting that prefetching can happen across one wrong speculated
branch, but not across two of them? And you are not worrying about prefetches
past the end of a copy with size > 0?

> IOW, I don't want to put invalid values in hw registers if I can. Think
> hw mitigations and *very* hungry prefetchers.
> 
>>> Current map:
>>> 0: start: 0x0000000000000000, end: 0x0000000000100000, type: 0x0
>>> 1: start: 0x0000000100000000, end: 0x0000000820000000, type: 0x6
>>> 2: start: 0x000002f10000c000, end: 0x000003bf0000c000, type: 0x2
>>> 3: start: 0x000003bf0000c000, end: 0x00019fc000001000, type: 0x0
>>> 4: start: 0x00019fc000001000, end: 0x0001df2d00001000, type: 0x2
>>
>> The map would reflect hardware behavior. Type 0 wins in case of overlapping
>> MTRRs.
> 
> Type 0 is MTRR_TYPE_UNCACHABLE, 1 is MTRR_TYPE_WRCOMB.
> 
> "Uncacheable (UC)—Reads from, and writes to, UC memory are not cacheable. Reads from UC
> memory cannot be speculative. Write-combining to UC memory is not
> allowed."
> 
> That last sentence.

"If two or more variable memory ranges match and one of the memory types is UC,
the UC memory type used."

So technically no problem, apart from lower performance.

> So if you have conflicting regions and one is WC but then something is
> expecting it to be UC and that something doesn't want for it to
> *especially* to be WC because it should not combine writes to it, then
> that clearly is a misconfiguration, I'd say.

Would you be fine with adding that as an additional patch?

I believe if we really want that, then we should be able to disable such a
cleanup. So it should be an add-on anyway.

>> Now this is another requirement, right? Today's MTRR code wouldn't scream
>> either.
> 
> So are we fixing this right or only a little?

I'm not against adding such additional checks. I wouldn't like to force them
into this series right now, because we need this series for Hyper-V isolated
guest support.

>> At least we don't correct such mistakes today. Do you think we should change
>> that?
> 
> I'm thinking considering how often we've seen all those error messages
> from mtrr_state_warn(), we should at least warn when we encounter
> inconsistencies.

Just to say it explicitly: you are concerned for the case that a complete
MTRR is hidden beneath another one (e.g. a large UC MTRR hiding a smaller
MTRR with another type, or a variable MTRR being hidden by fixed MTRRs)?

> This won't help with released BIOSes but it will help when they do new
> BIOS verification and see those messages. People do use Linux for that
> a lot and then they'll definitely look and address them.

Okay.

> And this is a pretty good goal to have, IMO.

Probably, yes.


Juergen

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

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