kernel-hardening - Re: [kvm-unit-tests PATCH] x86: Add control register pinning tests

Follow @Openwall on Twitter for new release announcements and other news

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

Date: Thu, 18 Jun 2020 05:08:34 +0000
From: "Andersen, John" <john.s.andersen@...el.com>
To: Nadav Amit <nadav.amit@...il.com>
Cc: corbet@....net, Paolo Bonzini <pbonzini@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>, mingo <mingo@...hat.com>,
	bp <bp@...en8.de>, hpa@...or.com, shuah@...nel.org,
	sean.j.christopherson@...el.com, rick.p.edgecombe@...el.com,
	kvm@...r.kernel.org, kernel-hardening@...ts.openwall.com
Subject: Re: [kvm-unit-tests PATCH] x86: Add control register pinning tests

On Wed, Jun 17, 2020 at 08:18:39PM -0700, Nadav Amit wrote:
> > On Jun 17, 2020, at 3:52 PM, Nadav Amit <nadav.amit@...il.com> wrote:
> > 
> >> On Jun 17, 2020, at 3:46 PM, John Andersen <john.s.andersen@...el.com> wrote:
> >> 
> >> Paravirutalized control register pinning adds MSRs guests can use to
> >> discover which bits in CR0/4 they may pin, and MSRs for activating
> >> pinning for any of those bits.
> > 
> > [ sni[
> > 
> >> +static void vmx_cr_pin_test_guest(void)
> >> +{
> >> +	unsigned long i, cr0, cr4;
> >> +
> >> +	/* Step 1. Skip feature detection to skip handling VMX_CPUID */
> >> +	/* nop */
> > 
> > I do not quite get this comment. Why do you skip checking whether the
> > feature is enabled? What happens if KVM/bare-metal/other-hypervisor that
> > runs this test does not support this feature?
> 
> My bad, I was confused between the nested checks and the non-nested ones.
> 
> Nevertheless, can we avoid situations in which
> rdmsr(MSR_KVM_CR0_PIN_ALLOWED) causes #GP when the feature is not
> implemented? Is there some protocol for detection that this feature is
> supported by the hypervisor, or do we need something like rdmsr_safe()?
> 

Ah, yes we can. By checking the CPUID for the feature bit. Thanks for pointing
this out, I was confused about this. I was operating under the assumption that
the unit tests assume the features in the latest kvm/next are present and
available when the unit tests are being run.

I'm happy to add the check, but I haven't see anywhere else where a
KVM_FEATURE_ was checked for. Which is why it doesn't check in this patch. As
soon as I get an answer from you or anyone else as to if the unit tests assume
that the features in the latest kvm/next are present and available or not when
the unit tests are being run I'll modify if necessary.

Thanks,
John

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.