On Tue, Nov 9, 2021 at 10:31 AM Sean Christopherson [off-list ref] wrote:
On Tue, Nov 09, 2021, Peter Gonda wrote:
quoted
On Tue, Nov 9, 2021 at 9:31 AM Sean Christopherson [off-list ref] wrote:
quoted
On Tue, Nov 02, 2021, Peter Gonda wrote:
quoted
This change moves the data corrupted retry of SEV_INIT into the
Use imperative mood.
Will do for next revision
quoted
quoted
__sev_platform_init_locked() function. This is for upcoming INIT_EX
support as well as helping direct callers of
__sev_platform_init_locked() which currently do not support the
retry.
Signed-off-by: Peter Gonda <redacted>
Reviewed-by: Marc Orr <redacted>
Acked-by: David Rientjes <rientjes@google.com>
Acked-by: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <redacted>
Cc: Marc Orr <redacted>
Cc: Joerg Roedel <redacted>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David Rientjes <rientjes@google.com>
Cc: John Allen <redacted>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: <redacted>
Cc: <redacted>
---
drivers/crypto/ccp/sev-dev.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index ec89a82ba267..e4bc833949a0 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -267,6 +267,18 @@ static int __sev_platform_init_locked(int *error)
}
rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
+ if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
There are no guarantees that @error is non-NULL as this is reachable via an
exported function, sev_platform_init(). Which ties in with my complaints in the
previous patch that the API is a bit of a mess.
That seems like a bug from the caller right? Is it typical that we
sanity-check the caller in these instances?
sev-dev.c needs to make up its mind. __sev_do_cmd_locked() very clearly allows
a NULL @error, ergo all of the wrappers for sev_do_cmd() support a NULL @error.
quoted
For example the same comment could be made here:
https://elixir.bootlin.com/linux/latest/source/drivers/crypto/ccp/sev-dev.c#L336
static int sev_get_platform_state(int *state, int *error)
{
struct sev_user_data_status data;
int rc;
rc = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &data, error);
if (rc)
return rc;
*state = data.state; <--- State could be null.
No, because this is an internal helper and all call sites can be easily audited.
quoted
return rc;
}
Example outside of this driver:
https://elixir.bootlin.com/linux/v5.15.1/source/arch/x86/kvm/x86.c#L468
int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
{
enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
enum lapic_mode new_mode = kvm_apic_mode(msr_info->data); <---
msr_info could be null here
u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff |
(guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID)
return 1;
if (!msr_info->host_initiated) {
if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC)
return 1;
if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC)
return 1;
}
kvm_lapic_set_base(vcpu, msr_info->data);
kvm_recalculate_apic_map(vcpu->kvm);
return 0;
}
EXPORT_SYMBOL_GPL(kvm_set_apic_base);
The difference is that KVM has consistent expecations for a set of functions,
whereas sev-dev.c does not. Yes, KVM will explode if @msr_info is NULL, and
there are undoubtedly a bajillion flows in the kernel that would do the same,
but unlike the functions declared in include/linux/psp-sev.h() the requirements
on the caller are fairly obvious. E.g. why should this be illegal from a caller's
perspective?
sev_platform_init(NULL);
sev_platform_status(&status, NULL);
Ack. I'll store a intermediate error in __sev_platform_init_locked and
export to @error if its not null.