linux-kernel - Re: [PATCH 09/10] KVM: SEV: introduce KVM_SEV

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20240215144422.st2md65quv34d4tk@amd.com>
Date: Thu, 15 Feb 2024 08:44:22 -0600
From: Michael Roth <michael.roth@....com>
To: Paolo Bonzini <pbonzini@...hat.com>
CC: <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>,
	<seanjc@...gle.com>, <aik@....com>, <isaku.yamahata@...el.com>,
	<thomas.lendacky@....com>
Subject: Re: [PATCH 09/10] KVM: SEV: introduce KVM_SEV_INIT2 operation

On Thu, Feb 15, 2024 at 02:44:47PM +0100, Paolo Bonzini wrote:
> On 2/15/24 02:34, Michael Roth wrote:
> > > +        struct struct kvm_sev_init {
> > Missing the vm_type param here.
> 
> It can go away in the struct actually.  Also, "struct struct".
> 
> > > +If the ``KVM_X86_SEV_VMSA_FEATURES`` attribute does not exist, the hypervisor only
> > > +supports KVM_SEV_INIT and KVM_SEV_ES_INIT.  In that case the set of VMSA features is
> > > +undefined.
> > 
> > It's hard to imagine userspace implementation support for querying
> > KVM_X86_SEV_VMSA_FEATURES but still insisting on KVM_SEV_INIT.
> 
> ... except for backwards compatibility with old kernels.  For example, the
> VMM could first invoke HAS_DEVICE_ATTR, and then fall back to KVM_SEV_INIT
> after checking that the user did not explicitly request or forbid one or
> more VMSA features.

What I mean is that if userspace is modified for these checks, it's
reasonable to also inform them that only VMSA features present in
those older kernels (i.e. debug-swap) will be available via KVM_SEV_INIT,
and for anything else they will need to use KVM_SEV_INIT.

That way we can provide clear documentation on what to expect regarding
VMSA features for KVM_SEV_INIT and not have to have the "undefined"
wording: it'll never use anything other than debug-swap depending on the
module param setting.

> 
> > Maybe it
> > would be better to just lock in that VMSA_FEATURES at what is currently
> > supported: DEBUG_SWAP=on/off depending on the kvm_amd module param, and
> > then for all other features require opt-in via KVM_SEV_INIT2, and then
> > bake that into the documentation. That way way they could still reference
> > this documentation to properly calculate measurements for older/existing
> > VMM implementations.
> 
> Thinking more about it, I think all features including debug_swap should be
> disabled with the old SEV_INIT/SEV_ES_INIT.  Because the features need to
> match between the VMM and the measurement calculation, they need to be added
> explicitly on e.g. the QEMU command line.

That seems reasonable, but the main thing I was hoping to avoid was
another round of VMSA features changing out from underneath the covers
again. The module param setting is something we've needed to convey
internally/externally a good bit due to the fallout and making this
change would lead to another repeat. Not the end of the world but would
be nice to avoid if possible.

-Mike

> 
> Paolo
>