From: Jarkko Sakkinen <[email protected]>
SEV-SNP does not initialize to a legit state, unless the firmware is
loaded twice, when SEP API version < 1.43, and the firmware is updated
to a later version. Because of this user space needs to work around
this with "rmmod && modprobe" combo. Fix this by implementing the
workaround to the driver.
Reported-by: Harald Hoyer <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
drivers/crypto/ccp/sev-dev.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 799b476fc3e8..f2abb7439dde 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -76,6 +76,9 @@ static void *sev_es_tmr;
#define NV_LENGTH (32 * 1024)
static void *sev_init_ex_buffer;
+/*
+ * SEV API version >= maj.min?
+ */
static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
{
struct sev_device *sev = psp_master->sev_data;
@@ -89,6 +92,14 @@ static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
return false;
}
+/*
+ * SEV API version < maj.min?
+ */
+static inline bool sev_version_less(u8 maj, u8 min)
+{
+ return !sev_version_greater_or_equal(maj, min);
+}
+
static void sev_irq_handler(int irq, void *data, unsigned int status)
{
struct sev_device *sev = data;
@@ -1274,6 +1285,7 @@ void sev_pci_init(void)
{
struct sev_device *sev = psp_master->sev_data;
int error, rc;
+ int i;
if (!sev)
return;
@@ -1283,9 +1295,13 @@ void sev_pci_init(void)
if (sev_get_api_version())
goto err;
- if (sev_version_greater_or_equal(0, 15) &&
- sev_update_firmware(sev->dev) == 0)
- sev_get_api_version();
+ /*
+ * SEV-SNP does not work properly before loading the FW twice in the API
+ * versions older than SEV 1.43.
+ */
+ for (i = 0; i < sev_version_greater_or_equal(0, 15) + sev_version_less(1, 43); i++)
+ if (sev_update_firmware(sev->dev) == 0)
+ sev_get_api_version();
/* If an init_ex_path is provided rely on INIT_EX for PSP initialization
* instead of INIT.
--
2.37.1
On 8/3/22 20:02, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <[email protected]>
>
> SEV-SNP does not initialize to a legit state, unless the firmware is
> loaded twice, when SEP API version < 1.43, and the firmware is updated
> to a later version. Because of this user space needs to work around
> this with "rmmod && modprobe" combo. Fix this by implementing the
> workaround to the driver.
The SNP hypervisor patches are placing a minimum supported version
requirement for the SEV firmware that exceeds the specified version
above [1] (for the reason above, as well as some others), so this patch
is not needed, NAK.
[1] https://lore.kernel.org/lkml/87a0481526e66ddd5f6192cbb43a50708aee2883.1655761627.git.ashish.kalra@amd.com/
Thanks,
Tom
>
> Reported-by: Harald Hoyer <[email protected]>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> drivers/crypto/ccp/sev-dev.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 799b476fc3e8..f2abb7439dde 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -76,6 +76,9 @@ static void *sev_es_tmr;
> #define NV_LENGTH (32 * 1024)
> static void *sev_init_ex_buffer;
>
> +/*
> + * SEV API version >= maj.min?
> + */
> static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
> {
> struct sev_device *sev = psp_master->sev_data;
> @@ -89,6 +92,14 @@ static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
> return false;
> }
>
> +/*
> + * SEV API version < maj.min?
> + */
> +static inline bool sev_version_less(u8 maj, u8 min)
> +{
> + return !sev_version_greater_or_equal(maj, min);
> +}
> +
> static void sev_irq_handler(int irq, void *data, unsigned int status)
> {
> struct sev_device *sev = data;
> @@ -1274,6 +1285,7 @@ void sev_pci_init(void)
> {
> struct sev_device *sev = psp_master->sev_data;
> int error, rc;
> + int i;
>
> if (!sev)
> return;
> @@ -1283,9 +1295,13 @@ void sev_pci_init(void)
> if (sev_get_api_version())
> goto err;
>
> - if (sev_version_greater_or_equal(0, 15) &&
> - sev_update_firmware(sev->dev) == 0)
> - sev_get_api_version();
> + /*
> + * SEV-SNP does not work properly before loading the FW twice in the API
> + * versions older than SEV 1.43.
> + */
> + for (i = 0; i < sev_version_greater_or_equal(0, 15) + sev_version_less(1, 43); i++)
> + if (sev_update_firmware(sev->dev) == 0)
> + sev_get_api_version();
>
> /* If an init_ex_path is provided rely on INIT_EX for PSP initialization
> * instead of INIT.
Am 04.08.22 um 15:13 schrieb Tom Lendacky:
> On 8/3/22 20:02, Jarkko Sakkinen wrote:
>> From: Jarkko Sakkinen <[email protected]>
>>
>> SEV-SNP does not initialize to a legit state, unless the firmware is
>> loaded twice, when SEP API version < 1.43, and the firmware is updated
>> to a later version. Because of this user space needs to work around
>> this with "rmmod && modprobe" combo. Fix this by implementing the
>> workaround to the driver.
>
> The SNP hypervisor patches are placing a minimum supported version
> requirement for the SEV firmware that exceeds the specified version
> above [1] (for the reason above, as well as some others), so this patch
> is not needed, NAK.
As described in the "Milan Release Notes.txt" of the AMD firmware update package amd_sev_fam19h_model0xh_1.33.03.zip.
"If upgrading to 1.33.01 or later from something older (picking up CSF-1201), it is required that two Download Firmware commands be run to fix the "Committed Version" across the firmware. CSF-1201
fixed a bug where the committed version in the attestation report was incorrect. Performing a single Download Firmware will upgrade the firmware, but performing a second one will correct the committed
version. This is a one-time upgrade issue.
"
Note that `1.33.01` is not the same version number as "1.51" in [1]. One is the firmware version, the other is the SEV-SNP API version.
I am definitely seeing a wrong TCB version, if the firmware is only updated once to `1.33.01` aka "1.51".
Reloading the `ccp` module, which triggers another firmware load, cures the problem.
The patch might be wrong, as it might not do the right thing, but the problem and the solution exist.
What is your suggestion then to fix the wrong committed TCB version?
>
> [1] https://lore.kernel.org/lkml/87a0481526e66ddd5f6192cbb43a50708aee2883.1655761627.git.ashish.kalra@amd.com/
>
> Thanks,
> Tom
>
>>
>> Reported-by: Harald Hoyer <[email protected]>
>> Signed-off-by: Jarkko Sakkinen <[email protected]>
>> ---
>> drivers/crypto/ccp/sev-dev.c | 22 +++++++++++++++++++---
>> 1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 799b476fc3e8..f2abb7439dde 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -76,6 +76,9 @@ static void *sev_es_tmr;
>> #define NV_LENGTH (32 * 1024)
>> static void *sev_init_ex_buffer;
>> +/*
>> + * SEV API version >= maj.min?
>> + */
>> static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
>> {
>> struct sev_device *sev = psp_master->sev_data;
>> @@ -89,6 +92,14 @@ static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
>> return false;
>> }
>> +/*
>> + * SEV API version < maj.min?
>> + */
>> +static inline bool sev_version_less(u8 maj, u8 min)
>> +{
>> + return !sev_version_greater_or_equal(maj, min);
>> +}
>> +
>> static void sev_irq_handler(int irq, void *data, unsigned int status)
>> {
>> struct sev_device *sev = data;
>> @@ -1274,6 +1285,7 @@ void sev_pci_init(void)
>> {
>> struct sev_device *sev = psp_master->sev_data;
>> int error, rc;
>> + int i;
>> if (!sev)
>> return;
>> @@ -1283,9 +1295,13 @@ void sev_pci_init(void)
>> if (sev_get_api_version())
>> goto err;
>> - if (sev_version_greater_or_equal(0, 15) &&
>> - sev_update_firmware(sev->dev) == 0)
>> - sev_get_api_version();
>> + /*
>> + * SEV-SNP does not work properly before loading the FW twice in the API
>> + * versions older than SEV 1.43.
>> + */
>> + for (i = 0; i < sev_version_greater_or_equal(0, 15) + sev_version_less(1, 43); i++)
>> + if (sev_update_firmware(sev->dev) == 0)
>> + sev_get_api_version();
>> /* If an init_ex_path is provided rely on INIT_EX for PSP initialization
>> * instead of INIT.
On 8/4/22 08:37, Harald Hoyer wrote:
> Am 04.08.22 um 15:13 schrieb Tom Lendacky:
>> On 8/3/22 20:02, Jarkko Sakkinen wrote:
>>> From: Jarkko Sakkinen <[email protected]>
>>>
>>> SEV-SNP does not initialize to a legit state, unless the firmware is
>>> loaded twice, when SEP API version < 1.43, and the firmware is updated
>>> to a later version. Because of this user space needs to work around
>>> this with "rmmod && modprobe" combo. Fix this by implementing the
>>> workaround to the driver.
>>
>> The SNP hypervisor patches are placing a minimum supported version
>> requirement for the SEV firmware that exceeds the specified version
>> above [1] (for the reason above, as well as some others), so this patch
>> is not needed, NAK.
>
> As described in the "Milan Release Notes.txt" of the AMD firmware update
> package amd_sev_fam19h_model0xh_1.33.03.zip.
>
> "If upgrading to 1.33.01 or later from something older (picking up
> CSF-1201), it is required that two Download Firmware commands be run to
> fix the "Committed Version" across the firmware. CSF-1201 fixed a bug
> where the committed version in the attestation report was incorrect.
> Performing a single Download Firmware will upgrade the firmware, but
> performing a second one will correct the committed version. This is a
> one-time upgrade issue.
> "
>
> Note that `1.33.01` is not the same version number as "1.51" in [1]. One
> is the firmware version, the other is the SEV-SNP API version.
It is the same and are meant to correlate, the 33 is hex => 51.
>
> I am definitely seeing a wrong TCB version, if the firmware is only
> updated once to `1.33.01` aka "1.51".
> Reloading the `ccp` module, which triggers another firmware load, cures
> the problem.
>
> The patch might be wrong, as it might not do the right thing, but the
> problem and the solution exist.
>
> What is your suggestion then to fix the wrong committed TCB version?
Hmmm... ok, I see what you're saying. We don't want to have to make
everyone update their BIOS/firmware to get to a starting level above 1.43
to begin with.
Ok, let me review/comment on the patch.
Thanks,
Tom
>
>>
>> [1]
>> https://lore.kernel.org/lkml/87a0481526e66ddd5f6192cbb43a50708aee2883.1655761627.git.ashish.kalra@amd.com/
>>
>>
>> Thanks,
>> Tom
>>
>>>
>>> Reported-by: Harald Hoyer <[email protected]>
>>> Signed-off-by: Jarkko Sakkinen <[email protected]>
>>> ---
>>> drivers/crypto/ccp/sev-dev.c | 22 +++++++++++++++++++---
>>> 1 file changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>> index 799b476fc3e8..f2abb7439dde 100644
>>> --- a/drivers/crypto/ccp/sev-dev.c
>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>> @@ -76,6 +76,9 @@ static void *sev_es_tmr;
>>> #define NV_LENGTH (32 * 1024)
>>> static void *sev_init_ex_buffer;
>>> +/*
>>> + * SEV API version >= maj.min?
>>> + */
>>> static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
>>> {
>>> struct sev_device *sev = psp_master->sev_data;
>>> @@ -89,6 +92,14 @@ static inline bool sev_version_greater_or_equal(u8
>>> maj, u8 min)
>>> return false;
>>> }
>>> +/*
>>> + * SEV API version < maj.min?
>>> + */
>>> +static inline bool sev_version_less(u8 maj, u8 min)
>>> +{
>>> + return !sev_version_greater_or_equal(maj, min);
>>> +}
>>> +
>>> static void sev_irq_handler(int irq, void *data, unsigned int status)
>>> {
>>> struct sev_device *sev = data;
>>> @@ -1274,6 +1285,7 @@ void sev_pci_init(void)
>>> {
>>> struct sev_device *sev = psp_master->sev_data;
>>> int error, rc;
>>> + int i;
>>> if (!sev)
>>> return;
>>> @@ -1283,9 +1295,13 @@ void sev_pci_init(void)
>>> if (sev_get_api_version())
>>> goto err;
>>> - if (sev_version_greater_or_equal(0, 15) &&
>>> - sev_update_firmware(sev->dev) == 0)
>>> - sev_get_api_version();
>>> + /*
>>> + * SEV-SNP does not work properly before loading the FW twice in
>>> the API
>>> + * versions older than SEV 1.43.
>>> + */
>>> + for (i = 0; i < sev_version_greater_or_equal(0, 15) +
>>> sev_version_less(1, 43); i++)
>>> + if (sev_update_firmware(sev->dev) == 0)
>>> + sev_get_api_version();
>>> /* If an init_ex_path is provided rely on INIT_EX for PSP
>>> initialization
>>> * instead of INIT.
>
On 8/3/22 20:02, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <[email protected]>
>
> SEV-SNP does not initialize to a legit state, unless the firmware is
> loaded twice, when SEP API version < 1.43, and the firmware is updated
> to a later version. Because of this user space needs to work around
> this with "rmmod && modprobe" combo. Fix this by implementing the
> workaround to the driver.
>
> Reported-by: Harald Hoyer <[email protected]>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> drivers/crypto/ccp/sev-dev.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 799b476fc3e8..f2abb7439dde 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1274,6 +1285,7 @@ void sev_pci_init(void)
> {
> struct sev_device *sev = psp_master->sev_data;
> int error, rc;
> + int i;
>
> if (!sev)
> return;
> @@ -1283,9 +1295,13 @@ void sev_pci_init(void)
> if (sev_get_api_version())
> goto err;
>
> - if (sev_version_greater_or_equal(0, 15) &&
> - sev_update_firmware(sev->dev) == 0)
> - sev_get_api_version();
> + /*
> + * SEV-SNP does not work properly before loading the FW twice in the API
> + * versions older than SEV 1.43.
> + */
> + for (i = 0; i < sev_version_greater_or_equal(0, 15) + sev_version_less(1, 43); i++)
> + if (sev_update_firmware(sev->dev) == 0)
> + sev_get_api_version();
I prefer to have this logic in the sev_update_firmware() function.
And while the loop is correct, it isn't obvious. Something in
sev_update_firmware() that just does:
ret = sev_do_cmd(SEV_CMD_DOWNLOAD_FIRMWARE, data, &error);
if (!ret && sev_version_greater_or_equal(0, 15) && sev_version_less(1, 43))
ret = sev_do_cmd(SEV_CMD_DOWNLOAD_FIRMWARE, data, &error);
And place a comment before the second command that references the reason
for the second download.
Thanks,
Tom
>
On Thu, Aug 04, 2022 at 03:37:20PM +0200, Harald Hoyer wrote:
> Am 04.08.22 um 15:13 schrieb Tom Lendacky:
> > On 8/3/22 20:02, Jarkko Sakkinen wrote:
> > > From: Jarkko Sakkinen <[email protected]>
> > >
> > > SEV-SNP does not initialize to a legit state, unless the firmware is
> > > loaded twice, when SEP API version < 1.43, and the firmware is updated
> > > to a later version. Because of this user space needs to work around
> > > this with "rmmod && modprobe" combo. Fix this by implementing the
> > > workaround to the driver.
> >
> > The SNP hypervisor patches are placing a minimum supported version
> > requirement for the SEV firmware that exceeds the specified version
> > above [1] (for the reason above, as well as some others), so this patch
> > is not needed, NAK.
>
> As described in the "Milan Release Notes.txt" of the AMD firmware update package amd_sev_fam19h_model0xh_1.33.03.zip.
>
> "If upgrading to 1.33.01 or later from something older (picking up
> CSF-1201), it is required that two Download Firmware commands be run to fix
> the "Committed Version" across the firmware. CSF-1201 fixed a bug where the
> committed version in the attestation report was incorrect. Performing a
> single Download Firmware will upgrade the firmware, but performing a second
> one will correct the committed version. This is a one-time upgrade issue.
> "
Reference should be part of the commit message. I'll
update for the next iteration. Thanks for the remark.
BR, Jarkko
On Thu, Aug 04, 2022 at 08:51:02AM -0500, Tom Lendacky wrote:
> On 8/4/22 08:37, Harald Hoyer wrote:
> > Am 04.08.22 um 15:13 schrieb Tom Lendacky:
> > > On 8/3/22 20:02, Jarkko Sakkinen wrote:
> > > > From: Jarkko Sakkinen <[email protected]>
> > > >
> > > > SEV-SNP does not initialize to a legit state, unless the firmware is
> > > > loaded twice, when SEP API version < 1.43, and the firmware is updated
> > > > to a later version. Because of this user space needs to work around
> > > > this with "rmmod && modprobe" combo. Fix this by implementing the
> > > > workaround to the driver.
> > >
> > > The SNP hypervisor patches are placing a minimum supported version
> > > requirement for the SEV firmware that exceeds the specified version
> > > above [1] (for the reason above, as well as some others), so this patch
> > > is not needed, NAK.
> >
> > As described in the "Milan Release Notes.txt" of the AMD firmware update
> > package amd_sev_fam19h_model0xh_1.33.03.zip.
> >
> > "If upgrading to 1.33.01 or later from something older (picking up
> > CSF-1201), it is required that two Download Firmware commands be run to
> > fix the "Committed Version" across the firmware. CSF-1201 fixed a bug
> > where the committed version in the attestation report was incorrect.
> > Performing a single Download Firmware will upgrade the firmware, but
> > performing a second one will correct the committed version. This is a
> > one-time upgrade issue.
> > "
> >
> > Note that `1.33.01` is not the same version number as "1.51" in [1]. One
> > is the firmware version, the other is the SEV-SNP API version.
>
> It is the same and are meant to correlate, the 33 is hex => 51.
>
> >
> > I am definitely seeing a wrong TCB version, if the firmware is only
> > updated once to `1.33.01` aka "1.51".
> > Reloading the `ccp` module, which triggers another firmware load, cures
> > the problem.
> >
> > The patch might be wrong, as it might not do the right thing, but the
> > problem and the solution exist.
> >
> > What is your suggestion then to fix the wrong committed TCB version?
>
> Hmmm... ok, I see what you're saying. We don't want to have to make everyone
> update their BIOS/firmware to get to a starting level above 1.43 to begin
> with.
>
> Ok, let me review/comment on the patch.
This was has a bug, and the reference to what Harald denoted
is missing. Hold on for v2. I'll put it out soon.
BR, Jarkko
On Thu, Aug 04, 2022 at 09:59:44AM -0500, Tom Lendacky wrote:
> On 8/3/22 20:02, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <[email protected]>
> >
> > SEV-SNP does not initialize to a legit state, unless the firmware is
> > loaded twice, when SEP API version < 1.43, and the firmware is updated
> > to a later version. Because of this user space needs to work around
> > this with "rmmod && modprobe" combo. Fix this by implementing the
> > workaround to the driver.
> >
> > Reported-by: Harald Hoyer <[email protected]>
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > ---
> > drivers/crypto/ccp/sev-dev.c | 22 +++++++++++++++++++---
> > 1 file changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index 799b476fc3e8..f2abb7439dde 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
>
> > @@ -1274,6 +1285,7 @@ void sev_pci_init(void)
> > {
> > struct sev_device *sev = psp_master->sev_data;
> > int error, rc;
> > + int i;
> > if (!sev)
> > return;
> > @@ -1283,9 +1295,13 @@ void sev_pci_init(void)
> > if (sev_get_api_version())
> > goto err;
> > - if (sev_version_greater_or_equal(0, 15) &&
> > - sev_update_firmware(sev->dev) == 0)
> > - sev_get_api_version();
> > + /*
> > + * SEV-SNP does not work properly before loading the FW twice in the API
> > + * versions older than SEV 1.43.
> > + */
> > + for (i = 0; i < sev_version_greater_or_equal(0, 15) + sev_version_less(1, 43); i++)
> > + if (sev_update_firmware(sev->dev) == 0)
> > + sev_get_api_version();
>
> I prefer to have this logic in the sev_update_firmware() function.
>
> And while the loop is correct, it isn't obvious. Something in
> sev_update_firmware() that just does:
>
> ret = sev_do_cmd(SEV_CMD_DOWNLOAD_FIRMWARE, data, &error);
>
> if (!ret && sev_version_greater_or_equal(0, 15) && sev_version_less(1, 43))
> ret = sev_do_cmd(SEV_CMD_DOWNLOAD_FIRMWARE, data, &error);
>
> And place a comment before the second command that references the reason
> for the second download.
OK, I'll do that. Thank you.
BR, Jarkko