On 03.11.25 16:18, Philippe Mathieu-Daudé wrote:
> Hi Jan,
>
> On 17/10/25 14:03, Jan Kiszka wrote:
>> From: Jan Kiszka <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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-
> [email protected]/]
>
> 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 ... --.
>
So, leave it like it is for now?
>> + 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.
Same story: Will pick it up and test.
Jan
--
Siemens AG, Foundational Technologies
Linux Expert Center
