Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 21 Nov 2022 17:14:54 +0100
From:   Borislav Petkov <bp@alien8.de>
To:     "Nikunj A. Dadhania" <nikunj@amd.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org, kvm@vger.kernel.org,
        mingo@redhat.com, tglx@linutronix.de, dave.hansen@linux.intel.com,
        seanjc@google.com, pbonzini@redhat.com, thomas.lendacky@amd.com,
        michael.roth@amd.com, stable@kernel.org,
        "Rao, Bharata Bhasker" <bharata@amd.com>
Subject: Re: [PATCH] x86/sev: Add SEV-SNP guest feature negotiation support
Message-ID: <Y3uj/gb8i4koG6Jv@zn.tnic>
References: <20221117044433.244656-1-nikunj@amd.com>
 <Y3YP5FQ6OHzVFKbp@zn.tnic>
 <7e224da6-6b02-2972-1334-bc8e7894cdab@amd.com>
 <Y3YuxGO8Kycymxg3@zn.tnic>
 <4ea6e1c3-fae5-a64a-9bd3-f9ea0821fc67@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <4ea6e1c3-fae5-a64a-9bd3-f9ea0821fc67@amd.com>
Precedence: bulk

On Fri, Nov 18, 2022 at 06:58:07PM +0530, Nikunj A. Dadhania wrote:
> No: For feature flags that need an enlightened guest, older guests
> should detect and fail booting on any hypervisor that sets this
> feature flag.

What would happen with such old guests nowadays? Wouldn't they explode
anyway?

And how is this supposed to work in practice?

I'm guessing this is supposed to address a case where guest owner goes
to a cloud provider, boots an old guest and it magically survives
booting and then it runs with some false sense of security.

But isn't that part of the whole attestation dance where the guest owner
checks for a minimum set of features it expects to be present?

Because if you do this and the cloud provider updates the hypervisor,
all the guest owner images might stop working all of a sudden because of
this check.

So what is the actual, real-life example where this helps? At all?

> The hypervisor can enable various new features flags(in
> SEV_FEATURES[1:63]) and start the guest. The hypervisor does not know
> beforehand that the guest kernel supports the feature that is being
> enabled.

This is not the right criterion: it should be more like: can a guest
still continue booting with a new feature it doesn't know about and
still provide the same security.

But see above - you need to think very practically here before even
considering such a thing.

> While booting, SNP guests can discover the hypervisor-enabled features
> by looking at SEV_STATUS MSR. At this point, the SNP guest needs to
> decide either to continue boot or terminate depending on the feature
> support in the guest kernel. Otherwise, if we let the guest continue
> booting with an unsupported feature, the guest can fail in non-obvious
> manner.

How can an old guest decide when it doesn't even have the intelligence
to do so?

What you're doing is, have old guests immediately stop booting when they
encounter a new feature - even if that new feature doesn't impair their
security.

> +-------------------+----------+-------------+----------+
> | HypervisorEnabled | Required | Available   |   Boot   |
> +-------------------+----------+-------------+----------+
> | Y                 |  Y       |  N          | N (Fail) |

This means that you need to know those features which would fail an old
guest *upfront*. Like right now. And I hardly doubt that.

> PREVENT_HOST_IBS will be defined but shouldn't be part of the 
> "Required" mask.

So it doesn't need to be mentioned at all.

> SECURE_TSC should be part of "Required" mask and once secure tsc 
> support is up-streamed it should be added to "Available" mask.

So when a guest owner gets a new guest which supports SECURE_TSC and
tries to boot it on an older HV - which is very much pretty everywhere
the case - cloud providers won't update their HV kernel whenever - that
new guest won't boot at all.

Which is a bad bad idea. I don't think you want that.

What you want, rather, is to say in the SECURE_TSC enablement code

	pr_warn("HV doesn't support secure TSC - your guest won't be 100% secure\n");

or so.

> Guests need to check for features enabled by the hypervisor that is not 
> supported as well, so that we can terminate the guest right there.

That needs to be done in the feature enablement code of each feature but
not wholesale like this.

Anyway, I think you get my point.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette