linux-kernel - Re: [PATCH 4/8] x86/traps: Demand-populate PASID MSR via #GP

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:   Mon, 27 Sep 2021 16:51:25 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     "Luck, Tony" <tony.luck@...el.com>,
        Andy Lutomirski <luto@...nel.org>
Cc:     Fenghua Yu <fenghua.yu@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        Joerg Roedel <joro@...tes.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Dave Jiang <dave.jiang@...el.com>,
        Jacob Jun Pan <jacob.jun.pan@...el.com>,
        Raj Ashok <ashok.raj@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        iommu@...ts.linux-foundation.org,
        the arch/x86 maintainers <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/8] x86/traps: Demand-populate PASID MSR via #GP

On 9/27/21 2:02 PM, Luck, Tony wrote:
> Or are you thinking of a helper that does both the check
> and the update ... so the code here could be:
> 
> 	update_one_xsave_feature(XFEATURE_PASID, &msr_val, sizeof(msr_val));
> 
> With the helper being something like:
> 
> void update_one_xsave_feature(enum xfeature xfeature, void *data, size_t size)
> {
> 	if (xsave_state_in_memory(args ...)) {
> 		addr = get_xsave_addr(xsave, xfeature);
> 		memcpy(addr, data, size);
> 		xsave->header.xfeatures |= (1 << xfeature);
> 		return;
> 	}
> 
> 	switch (xfeature) {
> 	case XFEATURE_PASID:
> 		wrmsrl(MSR_IA32_PASID, *(u64 *)data);
> 		break;
> 
> 	case each_of_the_other_XFEATURE_enums:
> 		code to update registers for that XFEATURE
> 	}
> }
> 
> either way needs the definitive correct coding for xsave_state_in_memory()

That's close to what we want.

The size should probably be implicit.  If it isn't implicit, it needs to
at least be double-checked against the state sizes.

Not to get too fancy, but I think we also want to have a "replace"
operation which is separate from the "update".  Think of a case where
you are trying to set a bit:

	struct pkru_state *pk = start_update_xstate(tsk, XSTATE_PKRU);
	pk->pkru |= 0x100;
	finish_update_xstate(tsk, XSTATE_PKRU, pk);

versus setting a whole new value:

	struct pkru_state *pk = start_replace_xstate(tsk, XSTATE_PKRU);
	memset(pkru, sizeof(*pk), 0);
	pk->pkru = 0x1234;
	finish_replace_xstate(tsk, XSTATE_PKRU, pk);

They look similar, but they actually might have very different costs for
big states like AMX.  We can also do some very different debugging for
them.  In normal operation, these _can_ just return pointers directly to
the fpu...->xstate in some case.  But, if we're debugging, we could
kmalloc() a buffer and do sanity checking before it goes into the task
buffer.

We don't have to do any of that fancy stuff now.  We don't even need to
do an "update" if all we need for now for XFEATURE_PASID is a "replace".

1. Hide whether we need to write to real registers
2. Hide whether we need to update the in-memory image
3. Hide other FPU infrastructure like the TIF flag.
4. Make the users deal with a *whole* state in the replace API