linux-kernel - Re: [PATCH v5 03/15] x86/mtrr: replace some constants with defines

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]

Date:   Wed, 5 Apr 2023 09:55:59 +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>
Subject: Re: [PATCH v5 03/15] x86/mtrr: replace some constants with defines

On 03.04.23 18:03, Borislav Petkov wrote:
> On Sat, Apr 01, 2023 at 08:36:40AM +0200, Juergen Gross wrote:
>> @@ -643,10 +646,12 @@ static bool set_mtrr_var_ranges(unsigned int index, struct mtrr_var_range *vr)
>>   	unsigned int lo, hi;
>>   	bool changed = false;
>>   
>> +#define BASE_MASK	(MTRR_BASE_TYPE_MASK | (size_and_mask << PAGE_SHIFT))
>> +#define MASK_MASK	(MTRR_MASK_VALID | (size_and_mask << PAGE_SHIFT))
> 
> No, "MASK_MASK" is too much. :-)

Any better suggestion for the name? :-)

> Anyway, so I started looking at this and here's what I'm seeing on my
> machine even with the old code:
> 
>          rdmsr(MTRRphysBase_MSR(index), lo, hi);
>          if ((vr->base_lo & 0xfffff0ffUL) != (lo & 0xfffff0ffUL)
>              || (vr->base_hi & (size_and_mask >> (32 - PAGE_SHIFT))) !=
>                  (hi & (size_and_mask >> (32 - PAGE_SHIFT)))) {
> 
> 
> size_and_mask:		0x0000000ffff00000
> the shifted version:	0x000000000000ffff
> 
> which is bits [15:0]
> 
> Now, looking at MTRRphysBase_MSR's definition, the high 32 bits are:
> 
> [63 ... reserved ... ][ max_addr ... 32 ]
> 
> and that second slice: from max_addr to the 32nd bit of the whole MSR is
> the second part of PhysBase.
> 
> max_addr aka phys_addr comes from:
> 
> 	phys_addr = cpuid_eax(0x80000008) & 0xff;
> 
> on that machine, that value is 48.
> 
> Which means, the physaddr slice should be [48 .. 32], i.e.,
> 
> 	0x0001ffff00000000

No. The "48" is the _number_ of physical address bits, so the 64 bit address
mask will be 0000ffff.ffffffff (48 bits set).

> 
> and when you shift that by 32 so that it can be ANDed with the high
> portion of the MSR, it becomes
> 
> 0x000000000001ffff
> 
> i.e., bit 16 is set too.
> 
> And that max address can be up to 51:
> 
> "Range Physical Base-Address (PhysBase)—Bits 51:12. The memory-range base-address in
> physical-address space. PhysBase is aligned on a 4-Kbyte (or greater) address in the 52-bit
> physical-address space supported by the AMD64 architecture. PhysBase represents the most-
> significant 40-address bits of the physical address. Physical-address
> bits 11:0 are assumed to be 0."
> 
> Long story short, size_and_mask needs to be done from
> 
> 	size_and_mask = ~size_or_mask & 0xfffff00000ULL;
> 
> to
> 
> 	size_and_mask = ~size_or_mask & GENMASK_ULL(phys_addr, 20);

This must be phys_addr - 1, as we start to count with bit 0.

> 
> size_or_mask bits already took into consideration the phys_addr:
> 
> 	size_or_mask = SIZE_OR_MASK_BITS(phys_addr);
> 
> and size_and_mask needs to do it too.
> 
> Right?
> 
> I'll test this on the boxes here everywhere. I guess this gets hit only
> on boxes where the phys_addr of the variable MTRRs goes over the 16
> bits.
> 
> As to this patch: please make all the bit and mask definitions you're
> adding as close to the MTRR bit and mask definition names in the
> APM+SDM. I've started this already (ontop of yours):

Okay.


Juergen

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

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