| Message ID | 20211102142331.3753798-3-pgonda@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Add SEV_INIT_EX support | expand |
On Tue, Nov 02, 2021, Peter Gonda wrote: > This change moves the data corrupted retry of SEV_INIT into the Use imperative mood. > __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 <pgonda@google.com> > Reviewed-by: Marc Orr <marcorr@google.com> > 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 <brijesh.singh@amd.com> > Cc: Marc Orr <marcorr@google.com> > Cc: Joerg Roedel <jroedel@suse.de> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: David Rientjes <rientjes@google.com> > Cc: John Allen <john.allen@amd.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: linux-crypto@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > 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. > + /* > + * INIT command returned an integrity check failure > + * status code, meaning that firmware load and > + * validation of SEV related persistent data has > + * failed and persistent state has been erased. > + * Retrying INIT command here should succeed. > + */ > + dev_dbg(sev->dev, "SEV: retrying INIT command"); > + rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error); > + } > + > if (rc) > return rc; > > @@ -1091,18 +1103,6 @@ void sev_pci_init(void) > > /* Initialize the platform */ > rc = sev_platform_init(&error); > - if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) { > - /* > - * INIT command returned an integrity check failure > - * status code, meaning that firmware load and > - * validation of SEV related persistent data has > - * failed and persistent state has been erased. > - * Retrying INIT command here should succeed. > - */ > - dev_dbg(sev->dev, "SEV: retrying INIT command"); > - rc = sev_platform_init(&error); > - } > - > if (rc) { > dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n", > error, rc); > -- > 2.33.1.1089.g2158813163f-goog >
On Tue, Nov 9, 2021 at 9:31 AM Sean Christopherson <seanjc@google.com> wrote: > > On Tue, Nov 02, 2021, Peter Gonda wrote: > > This change moves the data corrupted retry of SEV_INIT into the > > Use imperative mood. Will do for next revision > > > __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 <pgonda@google.com> > > Reviewed-by: Marc Orr <marcorr@google.com> > > 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 <brijesh.singh@amd.com> > > Cc: Marc Orr <marcorr@google.com> > > Cc: Joerg Roedel <jroedel@suse.de> > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > Cc: David Rientjes <rientjes@google.com> > > Cc: John Allen <john.allen@amd.com> > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: linux-crypto@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > --- > > 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? 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. 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); ``` About the API being a mess that seems a little out of scope for this change. I am not changing the API surface at all here. Again happy to discuss improvements with you and Tom for follow up series. > > > + /* > > + * INIT command returned an integrity check failure > > + * status code, meaning that firmware load and > > + * validation of SEV related persistent data has > > + * failed and persistent state has been erased. > > + * Retrying INIT command here should succeed. > > + */ > > + dev_dbg(sev->dev, "SEV: retrying INIT command"); > > + rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error); > > + } > > + > > if (rc) > > return rc; > > > > @@ -1091,18 +1103,6 @@ void sev_pci_init(void) > > > > /* Initialize the platform */ > > rc = sev_platform_init(&error); > > - if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) { > > - /* > > - * INIT command returned an integrity check failure > > - * status code, meaning that firmware load and > > - * validation of SEV related persistent data has > > - * failed and persistent state has been erased. > > - * Retrying INIT command here should succeed. > > - */ > > - dev_dbg(sev->dev, "SEV: retrying INIT command"); > > - rc = sev_platform_init(&error); > > - } > > - > > if (rc) { > > dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n", > > error, rc); > > -- > > 2.33.1.1089.g2158813163f-goog > >
On Tue, Nov 09, 2021, Peter Gonda wrote: > On Tue, Nov 9, 2021 at 9:31 AM Sean Christopherson <seanjc@google.com> wrote: > > > > On Tue, Nov 02, 2021, Peter Gonda wrote: > > > This change moves the data corrupted retry of SEV_INIT into the > > > > Use imperative mood. > > Will do for next revision > > > > > > __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 <pgonda@google.com> > > > Reviewed-by: Marc Orr <marcorr@google.com> > > > 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 <brijesh.singh@amd.com> > > > Cc: Marc Orr <marcorr@google.com> > > > Cc: Joerg Roedel <jroedel@suse.de> > > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > > Cc: David Rientjes <rientjes@google.com> > > > Cc: John Allen <john.allen@amd.com> > > > Cc: "David S. Miller" <davem@davemloft.net> > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > Cc: linux-crypto@vger.kernel.org > > > Cc: linux-kernel@vger.kernel.org > > > --- > > > 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. > 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. > 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);
On Tue, Nov 9, 2021 at 10:31 AM Sean Christopherson <seanjc@google.com> wrote: > > On Tue, Nov 09, 2021, Peter Gonda wrote: > > On Tue, Nov 9, 2021 at 9:31 AM Sean Christopherson <seanjc@google.com> wrote: > > > > > > On Tue, Nov 02, 2021, Peter Gonda wrote: > > > > This change moves the data corrupted retry of SEV_INIT into the > > > > > > Use imperative mood. > > > > Will do for next revision > > > > > > > > > __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 <pgonda@google.com> > > > > Reviewed-by: Marc Orr <marcorr@google.com> > > > > 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 <brijesh.singh@amd.com> > > > > Cc: Marc Orr <marcorr@google.com> > > > > Cc: Joerg Roedel <jroedel@suse.de> > > > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > > > Cc: David Rientjes <rientjes@google.com> > > > > Cc: John Allen <john.allen@amd.com> > > > > Cc: "David S. Miller" <davem@davemloft.net> > > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > > Cc: linux-crypto@vger.kernel.org > > > > Cc: linux-kernel@vger.kernel.org > > > > --- > > > > 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. > > > 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. > > > 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.
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) { + /* + * INIT command returned an integrity check failure + * status code, meaning that firmware load and + * validation of SEV related persistent data has + * failed and persistent state has been erased. + * Retrying INIT command here should succeed. + */ + dev_dbg(sev->dev, "SEV: retrying INIT command"); + rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error); + } + if (rc) return rc; @@ -1091,18 +1103,6 @@ void sev_pci_init(void) /* Initialize the platform */ rc = sev_platform_init(&error); - if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) { - /* - * INIT command returned an integrity check failure - * status code, meaning that firmware load and - * validation of SEV related persistent data has - * failed and persistent state has been erased. - * Retrying INIT command here should succeed. - */ - dev_dbg(sev->dev, "SEV: retrying INIT command"); - rc = sev_platform_init(&error); - } - if (rc) { dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n", error, rc);