Re: [PATCH v5 4/6] hw/sd/sdcard: Handle RPMB MAC field

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Jan,

On 17/10/25 14:03, 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.
> 
> This depends on HMAC support for QCRYPTO_HASH_ALGO_SHA256 which is
> always available via glib - assert this, just to be safe.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>   hw/sd/sd.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 81 insertions(+)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 918fe9f79f..759e284ac0 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];
> @@ -1120,6 +1122,66 @@ static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
>       }
>   }
>   
> +static bool rpmb_calc_hmac(SDState *sd, RPMBDataFrame *frame,

const frame.

> +                           unsigned int num_blocks, uint8_t *mac)
> +{
> +    size_t mac_len = RPMB_KEY_MAC_LEN;
> +    bool success = true;
> +    Error *err = NULL;
> +    QCryptoHmac *hmac;

Preferably:

        g_autoptr(QCryptoHmac) hmac = NULL;

> +    uint64_t addr;

Maybe better named 'offset'.

> +
> +    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;
Why not plain 'void *'? Ah, qcrypto_hmac_bytes() takes a 'char *', 
odd... [Update, now about to be fixed:
https://lore.kernel.org/qemu-devel/20251103133727.423041-4-berrange@redhat.com/]

Better safe than sorry:

            assert(RPMB_HASH_LEN <= sizeof(sd->data));

> +        memcpy(buf, frame->data, RPMB_HASH_LEN);

Nitpicking, no need to fill the block we'll overwrite:

            memcpy(&buf[256], &frame->data[256], RPMB_HASH_LEN - 256);

> +        addr = be16_to_cpu(frame->address) * 256 + sd_part_offset(sd);
Personally I prefer the ld/st API, and I'd rather see it used
consistently within hw/sd/. So (matter of taste):

            block_index = ldw_be_p(&frame->address);
            block_offset = block_index * 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");

Although a pre-existing pattern in this file, no new fprintf(stderr)
please. Better use the Error* API, otherwise error_report().

> +                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);

Ideally Error* should to be propagated to upper layers.

Here SDCardClass::read_byte() and SDCardClass::write_byte()
don't expect failure, so OK -- maybe they should ... --.

> +        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);
> @@ -1142,6 +1204,17 @@ static void emmc_rpmb_blk_read(SDState *sd, uint64_t addr, uint32_t len)
>               sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_READ_FAILURE |
>                   (result & RPMB_RESULT_COUTER_EXPIRED));
>           }
> +        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));
>   
> @@ -1153,6 +1226,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 */
> @@ -1190,6 +1264,11 @@ static void emmc_rpmb_blk_write(SDState *sd, uint64_t addr, uint32_t len)
>               sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_WRITE_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;
> @@ -3115,6 +3194,8 @@ static void emmc_class_init(ObjectClass *klass, const void *data)
>       DeviceClass *dc = DEVICE_CLASS(klass);
>       SDCardClass *sc = SDMMC_COMMON_CLASS(klass);
>   
> +    assert(qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256));
> +
>       dc->desc = "eMMC";
>       dc->realize = emmc_realize;
>       device_class_set_props(dc, emmc_properties);
I'd like to squash on this patch:
-- >8 --
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index f2260796db8..6e4eeeda157 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1129,3 +1129,3 @@ static void sd_blk_write(SDState *sd, uint64_t 
addr, uint32_t len)

-static bool rpmb_calc_hmac(SDState *sd, RPMBDataFrame *frame,
+static bool rpmb_calc_hmac(SDState *sd, const RPMBDataFrame *frame,
                             unsigned int num_blocks, uint8_t *mac)
@@ -1133,5 +1133,5 @@ static bool rpmb_calc_hmac(SDState *sd, 
RPMBDataFrame *frame,
      size_t mac_len = RPMB_KEY_MAC_LEN;
+    g_autoptr(QCryptoHmac) hmac = NULL;
      bool success = true;
      Error *err = NULL;
-    QCryptoHmac *hmac;
      uint64_t addr;
@@ -1156,9 +1156,12 @@ static bool rpmb_calc_hmac(SDState *sd, 
RPMBDataFrame *frame,
           */
-        char *buf = (char *)sd->data;
+        void *buf = sd->data;

-        memcpy(buf, frame->data, RPMB_HASH_LEN);
-        addr = be16_to_cpu(frame->address) * 256 + sd_part_offset(sd);
+        assert(RPMB_HASH_LEN <= sizeof(sd->data));
+        memcpy(&buf[RPMB_DATA_LEN], &frame->data[RPMB_DATA_LEN],
+               RPMB_HASH_LEN - RPMB_DATA_LEN);
+
+        addr = be16_to_cpu(frame->address) * RPMB_DATA_LEN + 
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");
+            if (blk_pread(sd->blk, addr, RPMB_DATA_LEN, buf, 0) < 0) {
+                error_report("sd_blk_read: read error on host side");
                  success = false;
@@ -1172,3 +1175,3 @@ static bool rpmb_calc_hmac(SDState *sd, 
RPMBDataFrame *frame,
              }
-            addr += 256;
+            addr += RPMB_DATA_LEN;
          } while (--num_blocks > 1);
@@ -1177,3 +1180,3 @@ static bool rpmb_calc_hmac(SDState *sd, 
RPMBDataFrame *frame,
      if (success &&
-        qcrypto_hmac_bytes(hmac, (const char*)frame->data, 
RPMB_HASH_LEN, &mac,
+        qcrypto_hmac_bytes(hmac, frame->data, RPMB_HASH_LEN, &mac,
                             &mac_len, &err) < 0) {
@@ -1184,4 +1187,2 @@ static bool rpmb_calc_hmac(SDState *sd, 
RPMBDataFrame *frame,

-    qcrypto_hmac_free(hmac);
-
      return success;
---

Regards,

Phil.