[PATCH 7/8] hw/sd/sdcard: Handle RPMB MAC field

Jan Kiszka posted 8 patches 2 months, 3 weeks ago
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Bin Meng <bmeng.cn@gmail.com>
There is a newer version of this series
[PATCH 7/8] hw/sd/sdcard: Handle RPMB MAC field
Posted by Jan Kiszka 2 months, 3 weeks ago
From: Jan Kiszka <jan.kiszka@siemens.com>

Implement correct setting of the MAC field when passing RPMB frames back
to the guest. Also check the MAC on authenticated write requests.

As this depends on HMAC support for QCRYPTO_HASH_ALGO_SHA256, only
register the eMMC class if that is available.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/sd/sd.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 89 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index f9578c6e55..1acf9f5306 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -51,6 +51,7 @@
 #include "qemu/module.h"
 #include "sdmmc-internal.h"
 #include "trace.h"
+#include "crypto/hmac.h"
 
 //#define DEBUG_SD 1
 
@@ -118,6 +119,7 @@ typedef struct SDProto {
 } SDProto;
 
 #define RPMB_KEY_MAC_LEN    32
+#define RPMB_HASH_LEN       284
 
 typedef struct {
     uint8_t stuff_bytes[196];
@@ -1125,6 +1127,66 @@ static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
     }
 }
 
+static bool rpmb_calc_hmac(SDState *sd, RPMBDataFrame *frame,
+                           unsigned int num_blocks, uint8_t *mac)
+{
+    size_t mac_len = RPMB_KEY_MAC_LEN;
+    bool success = true;
+    Error *err = NULL;
+    QCryptoHmac *hmac;
+    uint64_t addr;
+
+    hmac = qcrypto_hmac_new(QCRYPTO_HASH_ALGO_SHA256, sd->rpmb_key,
+                            RPMB_KEY_MAC_LEN, &err);
+    if (!hmac) {
+        error_report_err(err);
+        return false;
+    }
+
+    /*
+     * This implies a read request because we only support single-block write
+     * requests so far.
+     */
+    if (num_blocks > 1) {
+        /*
+         * Unfortunately, the underlying crypto libraries do not allow us to
+         * migrate an active QCryptoHmac state. Therefore, we have to calculate
+         * the HMAC in one run. To avoid buffering a complete read sequence in
+         * SDState, reconstruct all frames except for the last one.
+         */
+        char *buf = (char *)sd->data;
+
+        memcpy(buf, frame->data, RPMB_HASH_LEN);
+        addr = be16_to_cpu(frame->address) * 256 + sd_part_offset(sd);
+        do {
+            if (blk_pread(sd->blk, addr, 256, buf, 0) < 0) {
+                fprintf(stderr, "sd_blk_read: read error on host side\n");
+                success = false;
+                break;
+            }
+            if (qcrypto_hmac_bytes(hmac, buf, RPMB_HASH_LEN, NULL, NULL,
+                                   &err) < 0) {
+                error_report_err(err);
+                success = false;
+                break;
+            }
+            addr += 256;
+        } while (--num_blocks > 1);
+    }
+
+    if (success &&
+        qcrypto_hmac_bytes(hmac, (const char*)frame->data, RPMB_HASH_LEN, &mac,
+                           &mac_len, &err) < 0) {
+        error_report_err(err);
+        success = false;
+    }
+    assert(!success || mac_len == RPMB_KEY_MAC_LEN);
+
+    qcrypto_hmac_free(hmac);
+
+    return success;
+}
+
 static void emmc_rpmb_blk_read(SDState *sd, uint64_t addr, uint32_t len)
 {
     uint16_t resp = be16_to_cpu(sd->rpmb_result.req_resp);
@@ -1145,6 +1207,17 @@ static void emmc_rpmb_blk_read(SDState *sd, uint64_t addr, uint32_t len)
             memset(sd->rpmb_result.data, 0, sizeof(sd->rpmb_result.data));
             sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_READ_FAILURE);
         }
+        if (sd->multi_blk_cnt == 1 &&
+            !rpmb_calc_hmac(sd, &sd->rpmb_result,
+                            be16_to_cpu(sd->rpmb_result.block_count),
+                            sd->rpmb_result.key_mac)) {
+            memset(sd->rpmb_result.data, 0, sizeof(sd->rpmb_result.data));
+            sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_AUTH_FAILURE);
+        }
+    } else if (!rpmb_calc_hmac(sd, &sd->rpmb_result, 1,
+                               sd->rpmb_result.key_mac)) {
+        memset(sd->rpmb_result.data, 0, sizeof(sd->rpmb_result.data));
+        sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_AUTH_FAILURE);
     }
     memcpy(sd->data, &sd->rpmb_result, sizeof(sd->rpmb_result));
 
@@ -1156,6 +1229,7 @@ static void emmc_rpmb_blk_write(SDState *sd, uint64_t addr, uint32_t len)
 {
     RPMBDataFrame *frame = (RPMBDataFrame *)sd->data;
     uint16_t req = be16_to_cpu(frame->req_resp);
+    uint8_t mac[RPMB_KEY_MAC_LEN];
 
     if (req == RPMB_REQ_READ_RESULT) {
         /* just return the current result register */
@@ -1189,6 +1263,11 @@ static void emmc_rpmb_blk_write(SDState *sd, uint64_t addr, uint32_t len)
             sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_GENERAL_FAILURE);
             break;
         }
+        if (!rpmb_calc_hmac(sd, frame, 1, mac) ||
+            memcmp(frame->key_mac, mac, RPMB_KEY_MAC_LEN) != 0) {
+            sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_AUTH_FAILURE);
+            break;
+        }
         if (be32_to_cpu(frame->write_counter) != sd->rpmb_write_counter) {
             sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_COUNTER_FAILURE);
             break;
@@ -3122,6 +3201,7 @@ static const TypeInfo sd_types[] = {
         .parent         = TYPE_SD_CARD,
         .class_init     = sd_spi_class_init,
     },
+    /* must be last element */
     {
         .name           = TYPE_EMMC,
         .parent         = TYPE_SDMMC_COMMON,
@@ -3129,4 +3209,12 @@ static const TypeInfo sd_types[] = {
     },
 };
 
-DEFINE_TYPES(sd_types)
+static void sd_register_types(void)
+{
+    int num = ARRAY_SIZE(sd_types);
+    if (!qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256)) {
+        num--;
+    }
+    type_register_static_array(sd_types, num);
+}
+type_init(sd_register_types);
-- 
2.43.0
Re: [PATCH 7/8] hw/sd/sdcard: Handle RPMB MAC field
Posted by Jerome Forissier 2 months ago
Hi Jan,

On 8/24/25 09:18, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Implement correct setting of the MAC field when passing RPMB frames back
> to the guest. Also check the MAC on authenticated write requests.
> 
> As this depends on HMAC support for QCRYPTO_HASH_ALGO_SHA256, only
> register the eMMC class if that is available.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/sd/sd.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 89 insertions(+), 1 deletion(-)

I tested this series successfully on top of QEMU v10.1.0 with OP-TEE ('master'
branch, arm64 build), the u-boot 'next' branch, Linux v6.14 and mmc-utils 1.0.
So feel free to add:

Tested-by: Jerome Forissier <jerome.forissier@linaro.org>

Many thanks for your work. It will be valuable in the OP-TEE CI.

Regards,
-- 
Jerome
Re: [PATCH 7/8] hw/sd/sdcard: Handle RPMB MAC field
Posted by Philippe Mathieu-Daudé 2 months, 3 weeks ago
Hi Jan,

On 24/8/25 09:18, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Implement correct setting of the MAC field when passing RPMB frames back
> to the guest. Also check the MAC on authenticated write requests.
> 
> As this depends on HMAC support for QCRYPTO_HASH_ALGO_SHA256, only
> register the eMMC class if that is available.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>   hw/sd/sd.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 89 insertions(+), 1 deletion(-)


> @@ -3122,6 +3201,7 @@ static const TypeInfo sd_types[] = {
>           .parent         = TYPE_SD_CARD,
>           .class_init     = sd_spi_class_init,
>       },
> +    /* must be last element */
>       {
>           .name           = TYPE_EMMC,
>           .parent         = TYPE_SDMMC_COMMON,
> @@ -3129,4 +3209,12 @@ static const TypeInfo sd_types[] = {
>       },
>   };
>   
> -DEFINE_TYPES(sd_types)
> +static void sd_register_types(void)
> +{
> +    int num = ARRAY_SIZE(sd_types);
> +    if (!qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256)) {
> +        num--;

Instead, expose RPMB feature in CSD when HMAC supported?

Something in emmc_set_ext_csd() in the lines of:

   if (qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256)) {
       sd->ext_csd[EXT_CSD_REV] = 5;
       sd->ext_csd[EXT_CSD_RPMB_MULT] = sd->rpmb_part_size / (128 * KiB);
       sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0b111;
   } else {
       sd->ext_csd[EXT_CSD_REV] = 3;
   }

> +    }
> +    type_register_static_array(sd_types, num);
> +}
> +type_init(sd_register_types);
Re: [PATCH 7/8] hw/sd/sdcard: Handle RPMB MAC field
Posted by Jan Kiszka 2 months, 3 weeks ago
On 25.08.25 11:47, Philippe Mathieu-Daudé wrote:
> Hi Jan,
> 
> On 24/8/25 09:18, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Implement correct setting of the MAC field when passing RPMB frames back
>> to the guest. Also check the MAC on authenticated write requests.
>>
>> As this depends on HMAC support for QCRYPTO_HASH_ALGO_SHA256, only
>> register the eMMC class if that is available.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>   hw/sd/sd.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 89 insertions(+), 1 deletion(-)
> 
> 
>> @@ -3122,6 +3201,7 @@ static const TypeInfo sd_types[] = {
>>           .parent         = TYPE_SD_CARD,
>>           .class_init     = sd_spi_class_init,
>>       },
>> +    /* must be last element */
>>       {
>>           .name           = TYPE_EMMC,
>>           .parent         = TYPE_SDMMC_COMMON,
>> @@ -3129,4 +3209,12 @@ static const TypeInfo sd_types[] = {
>>       },
>>   };
>>   -DEFINE_TYPES(sd_types)
>> +static void sd_register_types(void)
>> +{
>> +    int num = ARRAY_SIZE(sd_types);
>> +    if (!qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256)) {
>> +        num--;
> 
> Instead, expose RPMB feature in CSD when HMAC supported?
> 
> Something in emmc_set_ext_csd() in the lines of:
> 
>   if (qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256)) {
>       sd->ext_csd[EXT_CSD_REV] = 5;
>       sd->ext_csd[EXT_CSD_RPMB_MULT] = sd->rpmb_part_size / (128 * KiB);
>       sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0b111;
>   } else {
>       sd->ext_csd[EXT_CSD_REV] = 3;
>   }

I need to check if revision 5 still had RPMB as optional (current ones
definitely require it), but I don't think rolling back to revision 3
would be good idea. If start to add more features from newer revisions,
that may cause even more weird results from the user perspective. I'm
not saying we are fully compliant in one or the other version, rather
that we need to work towards becoming so. Have to support multiple
versions along that will not make it easier.

Jan

> 
>> +    }
>> +    type_register_static_array(sd_types, num);
>> +}
>> +type_init(sd_register_types);
> 

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH 7/8] hw/sd/sdcard: Handle RPMB MAC field
Posted by Philippe Mathieu-Daudé 2 months, 3 weeks ago
+Dan

On 25/8/25 18:12, Jan Kiszka wrote:
> On 25.08.25 11:47, Philippe Mathieu-Daudé wrote:
>> Hi Jan,
>>
>> On 24/8/25 09:18, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Implement correct setting of the MAC field when passing RPMB frames back
>>> to the guest. Also check the MAC on authenticated write requests.
>>>
>>> As this depends on HMAC support for QCRYPTO_HASH_ALGO_SHA256, only
>>> register the eMMC class if that is available.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>    hw/sd/sd.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 89 insertions(+), 1 deletion(-)
>>
>>
>>> @@ -3122,6 +3201,7 @@ static const TypeInfo sd_types[] = {
>>>            .parent         = TYPE_SD_CARD,
>>>            .class_init     = sd_spi_class_init,
>>>        },
>>> +    /* must be last element */
>>>        {
>>>            .name           = TYPE_EMMC,
>>>            .parent         = TYPE_SDMMC_COMMON,
>>> @@ -3129,4 +3209,12 @@ static const TypeInfo sd_types[] = {
>>>        },
>>>    };
>>>    -DEFINE_TYPES(sd_types)
>>> +static void sd_register_types(void)
>>> +{
>>> +    int num = ARRAY_SIZE(sd_types);
>>> +    if (!qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256)) {
>>> +        num--;
>>
>> Instead, expose RPMB feature in CSD when HMAC supported?
>>
>> Something in emmc_set_ext_csd() in the lines of:
>>
>>    if (qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256)) {
>>        sd->ext_csd[EXT_CSD_REV] = 5;
>>        sd->ext_csd[EXT_CSD_RPMB_MULT] = sd->rpmb_part_size / (128 * KiB);
>>        sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0b111;
>>    } else {
>>        sd->ext_csd[EXT_CSD_REV] = 3;
>>    }
> 
> I need to check if revision 5 still had RPMB as optional (current ones
> definitely require it), but I don't think rolling back to revision 3
> would be good idea. If start to add more features from newer revisions,
> that may cause even more weird results from the user perspective. I'm
> not saying we are fully compliant in one or the other version, rather
> that we need to work towards becoming so. Have to support multiple
> versions along that will not make it easier.

Daniel, do you have a rough idea how many of our build config do
not support QCRYPTO_HASH_ALGO_SHA256?
(looking about making the SD device unconditional to it).

>>> +    }
>>> +    type_register_static_array(sd_types, num);
>>> +}
>>> +type_init(sd_register_types);
>>
> 


Re: [PATCH 7/8] hw/sd/sdcard: Handle RPMB MAC field
Posted by Daniel P. Berrangé 2 months, 2 weeks ago
On Mon, Aug 25, 2025 at 06:30:52PM +0200, Philippe Mathieu-Daudé wrote:
> +Dan
> 
> On 25/8/25 18:12, Jan Kiszka wrote:
> > On 25.08.25 11:47, Philippe Mathieu-Daudé wrote:
> > > Hi Jan,
> > > 
> > > On 24/8/25 09:18, Jan Kiszka wrote:
> > > > From: Jan Kiszka <jan.kiszka@siemens.com>
> > > > 
> > > > Implement correct setting of the MAC field when passing RPMB frames back
> > > > to the guest. Also check the MAC on authenticated write requests.
> > > > 
> > > > As this depends on HMAC support for QCRYPTO_HASH_ALGO_SHA256, only
> > > > register the eMMC class if that is available.
> > > > 
> > > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > > ---
> > > >    hw/sd/sd.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > >    1 file changed, 89 insertions(+), 1 deletion(-)
> > > 
> > > 
> > > > @@ -3122,6 +3201,7 @@ static const TypeInfo sd_types[] = {
> > > >            .parent         = TYPE_SD_CARD,
> > > >            .class_init     = sd_spi_class_init,
> > > >        },
> > > > +    /* must be last element */
> > > >        {
> > > >            .name           = TYPE_EMMC,
> > > >            .parent         = TYPE_SDMMC_COMMON,
> > > > @@ -3129,4 +3209,12 @@ static const TypeInfo sd_types[] = {
> > > >        },
> > > >    };
> > > >    -DEFINE_TYPES(sd_types)
> > > > +static void sd_register_types(void)
> > > > +{
> > > > +    int num = ARRAY_SIZE(sd_types);
> > > > +    if (!qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256)) {
> > > > +        num--;
> > > 
> > > Instead, expose RPMB feature in CSD when HMAC supported?
> > > 
> > > Something in emmc_set_ext_csd() in the lines of:
> > > 
> > >    if (qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256)) {
> > >        sd->ext_csd[EXT_CSD_REV] = 5;
> > >        sd->ext_csd[EXT_CSD_RPMB_MULT] = sd->rpmb_part_size / (128 * KiB);
> > >        sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0b111;
> > >    } else {
> > >        sd->ext_csd[EXT_CSD_REV] = 3;
> > >    }
> > 
> > I need to check if revision 5 still had RPMB as optional (current ones
> > definitely require it), but I don't think rolling back to revision 3
> > would be good idea. If start to add more features from newer revisions,
> > that may cause even more weird results from the user perspective. I'm
> > not saying we are fully compliant in one or the other version, rather
> > that we need to work towards becoming so. Have to support multiple
> > versions along that will not make it easier.
> 
> Daniel, do you have a rough idea how many of our build config do
> not support QCRYPTO_HASH_ALGO_SHA256?
> (looking about making the SD device unconditional to it).

That's always available, since we can get it from 'glib' even when no
crypto libs are linked.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


Re: [PATCH 7/8] hw/sd/sdcard: Handle RPMB MAC field
Posted by Jan Kiszka 2 months, 2 weeks ago
On 26.08.25 12:18, Daniel P. Berrangé wrote:
> On Mon, Aug 25, 2025 at 06:30:52PM +0200, Philippe Mathieu-Daudé wrote:
>> +Dan
>>
>> On 25/8/25 18:12, Jan Kiszka wrote:
>>> On 25.08.25 11:47, Philippe Mathieu-Daudé wrote:
>>>> Hi Jan,
>>>>
>>>> On 24/8/25 09:18, Jan Kiszka wrote:
>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> Implement correct setting of the MAC field when passing RPMB frames back
>>>>> to the guest. Also check the MAC on authenticated write requests.
>>>>>
>>>>> As this depends on HMAC support for QCRYPTO_HASH_ALGO_SHA256, only
>>>>> register the eMMC class if that is available.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>>    hw/sd/sd.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>    1 file changed, 89 insertions(+), 1 deletion(-)
>>>>
>>>>
>>>>> @@ -3122,6 +3201,7 @@ static const TypeInfo sd_types[] = {
>>>>>            .parent         = TYPE_SD_CARD,
>>>>>            .class_init     = sd_spi_class_init,
>>>>>        },
>>>>> +    /* must be last element */
>>>>>        {
>>>>>            .name           = TYPE_EMMC,
>>>>>            .parent         = TYPE_SDMMC_COMMON,
>>>>> @@ -3129,4 +3209,12 @@ static const TypeInfo sd_types[] = {
>>>>>        },
>>>>>    };
>>>>>    -DEFINE_TYPES(sd_types)
>>>>> +static void sd_register_types(void)
>>>>> +{
>>>>> +    int num = ARRAY_SIZE(sd_types);
>>>>> +    if (!qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256)) {
>>>>> +        num--;
>>>>
>>>> Instead, expose RPMB feature in CSD when HMAC supported?
>>>>
>>>> Something in emmc_set_ext_csd() in the lines of:
>>>>
>>>>    if (qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256)) {
>>>>        sd->ext_csd[EXT_CSD_REV] = 5;
>>>>        sd->ext_csd[EXT_CSD_RPMB_MULT] = sd->rpmb_part_size / (128 * KiB);
>>>>        sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0b111;
>>>>    } else {
>>>>        sd->ext_csd[EXT_CSD_REV] = 3;
>>>>    }
>>>
>>> I need to check if revision 5 still had RPMB as optional (current ones
>>> definitely require it), but I don't think rolling back to revision 3
>>> would be good idea. If start to add more features from newer revisions,
>>> that may cause even more weird results from the user perspective. I'm
>>> not saying we are fully compliant in one or the other version, rather
>>> that we need to work towards becoming so. Have to support multiple
>>> versions along that will not make it easier.
>>
>> Daniel, do you have a rough idea how many of our build config do
>> not support QCRYPTO_HASH_ALGO_SHA256?
>> (looking about making the SD device unconditional to it).
> 
> That's always available, since we can get it from 'glib' even when no
> crypto libs are linked.
> 

Perfect, makes things simpler.

So what is best practice, assert() availability or silently assume that
it is there?

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH 7/8] hw/sd/sdcard: Handle RPMB MAC field
Posted by Daniel P. Berrangé 2 months, 2 weeks ago
On Wed, Aug 27, 2025 at 07:53:12AM +0200, Jan Kiszka wrote:
> On 26.08.25 12:18, Daniel P. Berrangé wrote:
> > On Mon, Aug 25, 2025 at 06:30:52PM +0200, Philippe Mathieu-Daudé wrote:
> >> +Dan
> >>
> >> On 25/8/25 18:12, Jan Kiszka wrote:
> >>> On 25.08.25 11:47, Philippe Mathieu-Daudé wrote:
> >>>> Hi Jan,
> >>>>
> >>>> On 24/8/25 09:18, Jan Kiszka wrote:
> >>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>
> >>>>> Implement correct setting of the MAC field when passing RPMB frames back
> >>>>> to the guest. Also check the MAC on authenticated write requests.
> >>>>>
> >>>>> As this depends on HMAC support for QCRYPTO_HASH_ALGO_SHA256, only
> >>>>> register the eMMC class if that is available.
> >>>>>
> >>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>> ---
> >>>>>    hw/sd/sd.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>>>    1 file changed, 89 insertions(+), 1 deletion(-)
> >>>>
> >>>>
> >>>>> @@ -3122,6 +3201,7 @@ static const TypeInfo sd_types[] = {
> >>>>>            .parent         = TYPE_SD_CARD,
> >>>>>            .class_init     = sd_spi_class_init,
> >>>>>        },
> >>>>> +    /* must be last element */
> >>>>>        {
> >>>>>            .name           = TYPE_EMMC,
> >>>>>            .parent         = TYPE_SDMMC_COMMON,
> >>>>> @@ -3129,4 +3209,12 @@ static const TypeInfo sd_types[] = {
> >>>>>        },
> >>>>>    };
> >>>>>    -DEFINE_TYPES(sd_types)
> >>>>> +static void sd_register_types(void)
> >>>>> +{
> >>>>> +    int num = ARRAY_SIZE(sd_types);
> >>>>> +    if (!qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256)) {
> >>>>> +        num--;
> >>>>
> >>>> Instead, expose RPMB feature in CSD when HMAC supported?
> >>>>
> >>>> Something in emmc_set_ext_csd() in the lines of:
> >>>>
> >>>>    if (qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256)) {
> >>>>        sd->ext_csd[EXT_CSD_REV] = 5;
> >>>>        sd->ext_csd[EXT_CSD_RPMB_MULT] = sd->rpmb_part_size / (128 * KiB);
> >>>>        sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0b111;
> >>>>    } else {
> >>>>        sd->ext_csd[EXT_CSD_REV] = 3;
> >>>>    }
> >>>
> >>> I need to check if revision 5 still had RPMB as optional (current ones
> >>> definitely require it), but I don't think rolling back to revision 3
> >>> would be good idea. If start to add more features from newer revisions,
> >>> that may cause even more weird results from the user perspective. I'm
> >>> not saying we are fully compliant in one or the other version, rather
> >>> that we need to work towards becoming so. Have to support multiple
> >>> versions along that will not make it easier.
> >>
> >> Daniel, do you have a rough idea how many of our build config do
> >> not support QCRYPTO_HASH_ALGO_SHA256?
> >> (looking about making the SD device unconditional to it).
> > 
> > That's always available, since we can get it from 'glib' even when no
> > crypto libs are linked.
> > 
> 
> Perfect, makes things simpler.
> 
> So what is best practice, assert() availability or silently assume that
> it is there?

Best practice is always to propagate errors, but if your call chain can't
do that, then in this case you can assert or use &error_abort since this
should never trigger.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|