linux-kernel - Re: [PATCH] crypto: ccp: Load the firmware twice when SEV API version

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <de02389f-249d-f565-1136-4af3655fab2a@profian.com>
Date:   Thu, 4 Aug 2022 15:37:20 +0200
From:   Harald Hoyer <harald@...fian.com>
To:     Tom Lendacky <thomas.lendacky@....com>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     Jarkko Sakkinen <jarkko@...fian.com>,
        Brijesh Singh <brijesh.singh@....com>,
        John Allen <john.allen@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        "open list:AMD CRYPTOGRAPHIC COPROCESSOR (CCP) DRIVER - SE..." 
        <linux-crypto@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] crypto: ccp: Load the firmware twice when SEV API version
 < 1.43

Am 04.08.22 um 15:13 schrieb Tom Lendacky:
> On 8/3/22 20:02, Jarkko Sakkinen wrote:
>> From: Jarkko Sakkinen <jarkko@...fian.com>
>>
>> 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 <harald@...fian.com>
>> Signed-off-by: Jarkko Sakkinen <jarkko@...fian.com>
>> ---
>>   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.