linux-kernel - Re: [PATCH] x86/PAT: have pat_enabled() properly reflect state when running on e.g. Xen

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, 6 Jul 2022 08:17:41 +0200
From:   Jan Beulich <jbeulich@...e.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Andrew Lutomirski <luto@...nel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        lkml <linux-kernel@...r.kernel.org>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>
Subject: Re: [PATCH] x86/PAT: have pat_enabled() properly reflect state when
 running on e.g. Xen

On 05.07.2022 18:14, Borislav Petkov wrote:
> On Tue, Jul 05, 2022 at 05:56:36PM +0200, Jan Beulich wrote:
>> Re-using pat_disabled like you do in your suggestion below won't
>> work, because mtrr_bp_init() calls pat_disable() when MTRRs
>> appear to be disabled (from the kernel's view). The goal is to
>> honor "nopat" without honoring any other calls to pat_disable().
> 
> Actually, the current goal is to adjust Xen dom0 because:
> 
> 1. it uses the PAT code
> 
> 2. but then it does something special and hides the MTRRs
> 
> which is not something real hardware does.
> 
> So this one-off thing should be prominent, visible and not get in the
> way.
> 
> As to mtrr_bp_init(), can you use X86_FEATURE_XENPV there to detect this
> special case when the kernel is running as dom0 and set stuff there
> accordingly so that it doesn't disable PAT?

Sure, but that alone won't help. There's a beneficial side effect
of running through pat_disable(): That way pat_init() will bail
right away. Without that I'd need to further special case things
there (as under Xen/PV PAT must not be written, only read) and I'd
also need to set pat_bp_enabled and pat_bp_initialized somewhere.
I could of course check X86_FEATURE_XENPV in all the necessary
places, but I was quite certain _that_ wouldn't be liked (nor
would I be convinced this is the right thing to do - see bottom).

> Then you don't have to touch pat_disabled() either but intergrate the
> Xen variant properly...
> 
>> I can probably fiddle with pat_enabled() instead of with
>> init_cache_modes(), but when making the change I had the feeling
>> this might be less liked (as looking more hacky, at least to me).
> 
> Why would that be more hacky?

My view on it, as said. I did actually make several attempts, until
reaching what I then submitted. All earlier ones were quite a bit
more intrusive (see above for an outline).

> I'd much rather check upfront what the kernel is running on and act
> accordingly instead of hooking into random functions and then years
> later wonder why was it done in the first place.

Thank you for putting it that kindly. It was a pretty conscious
decision where to make the changes, after - as said - quite a bit
of trying other variants. History with Xen-specific changes has
taught me to try to keep them as uninvasive and generic as possible.
The more things smelled like Xen-only, the less they were liked.

>> But besides the "where" the other question is: Do you really want
>> me to limit this to Xen/PV, rather than - as I have it now -
>> extending it to any hypervisor, which may behave in similar ways?
> 
> Well, do you know of some other HV which hides MTRRs from the guest?
> 
> I haven't heard of any...

Any decent hypervisor will allow overriding CPUID, so in principle
I'd expect any to permit disabling MTRR to leave a guest to use
the (more modern and less cumbersome) PAT alone.

Jan