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/[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 ... --.
+ 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.
