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

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

Message-ID: <d164d84b-6773-36e3-1136-672072e9233d@suse.com>
Date:   Thu, 20 Apr 2023 14:30:09 +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 14:15, Borislav Petkov wrote:
> On Sat, Apr 01, 2023 at 08:36:48AM +0200, Juergen Gross wrote:
>> +static void rm_map_entry_at(int idx)
>> +{
>> +	cache_map_n--;
> 
> Let's not call memmove() when cache_map_n == idx.
> 
> Below too.
> 
> Especially since cache_map + idx + 1 is not valid and I wouldn't want it
> getting prefetched from %rsi in the hw when there's no reason for it and
> also the RET even from a function which doesn't do anything, costs.

OTOH the additional compare to 0 has costs, too, and this cost is spent for
ALL calls, while the zero size call is a rather rare case.

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 [1].

> 
>> +	memmove(cache_map + idx, cache_map + idx + 1,
>> +		sizeof(*cache_map) * (cache_map_n - idx));
>> +}
> 
> Ok, first weird issue I encountered while playing with my carved out
> program to exercise this cache_map handling thing. I can share it if you
> want it - it is ugly but it works.
> 
> So while rebuilding the map, I have these current regions in the map, at
> one point in time:
> 
> 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: 0x00000d4b0000c000, type: 0x1
> 4: start: 0x00000d4b0000c000, end: 0x00019fc000001000, type: 0x0
> 5: start: 0x00019fc000001000, end: 0x0001df2d00001000, type: 0x2
> 
> note entry #3.
> 
> Now the next one it inserts is:
> 
> add_map_entry_at: start: 0x3bf0000c000, end: 0xd4b0000c000, type: 0x0, idx: 3
>   merge_prev 0: prev->fixed: 0, prev->end: 0x3bf0000c000, prev->type: 0x2
>   merge_next: 1, next->fixed: 0, next->start: 0xd4b0000c000, next->type: 0x0
> add_map_entry_at: ret: 1
> 
> Note how it is the same as entry number #3 - just a different type.
> 
> What it ends up doing is, it simply overwrites the previous one and
> merges it with the next:
> 
> 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.

> Now I think right about now we should've screamed loudly.

Now this is another requirement, right? Today's MTRR code wouldn't scream
either.

> I know I know, this should never happen, right? And firmware programming
> those MTRRs doesn't make mistakes...

At least we don't correct such mistakes today. Do you think we should change
that?

> However, we should be loud here and scream when a MTRR range disappears
> like that.
> 
> Right?

TBH, I don't know.


Juergen

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

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