Hi Cédric
> Subject: Re: [PATCH v3 6/9] hw/misc/aspeed_sbc: Support the ECDSA verify
> command
>
> On 8/31/26 05:42, Jamin Lin wrote:
> > The AST10x0 secure boot controller register block also hosts an ECDSA
> > engine. Emulate its secp384r1 "verify" command: on a trigger write to
> > the command register, read the public key, signature and SHA-384
> > digest that the firmware staged in the SEC SRAM and defer the
> > verification to the crypto akcipher backend, reporting the result
> > through the status register.
> >
> > The model reads its operands from the SEC SRAM through a dedicated
> > address space, using a 'sram' link. The SEC SRAM is mapped at offset 0
> > of that address space, so the engine addresses each operand directly
> > with its SRAM-relative offset.
> >
> > Signed-off-by: Jamin Lin <[email protected]>
> > ---
> > include/hw/misc/aspeed_sbc.h | 4 +
> > hw/misc/aspeed_sbc.c | 152
> +++++++++++++++++++++++++++++++++++
> > hw/misc/trace-events | 2 +
> > 3 files changed, 158 insertions(+)
> >
> > diff --git a/include/hw/misc/aspeed_sbc.h
> > b/include/hw/misc/aspeed_sbc.h index 07c7c22a86..8d3f9207fc 100644
> > --- a/include/hw/misc/aspeed_sbc.h
> > +++ b/include/hw/misc/aspeed_sbc.h
> > @@ -40,12 +40,16 @@ struct AspeedSBCState {
> > uint32_t regs[ASPEED_SBC_NR_REGS];
> >
> > AspeedOTPState otp;
> > +
> > + MemoryRegion *sram;
> > + AddressSpace sram_as;
> > };
> >
> > struct AspeedSBCClass {
> > SysBusDeviceClass parent_class;
> >
> > bool has_otp;
> > + bool has_ecdsa;
> > };
> >
> > #endif /* ASPEED_SBC_H */
> > diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c index
> > 1dfcf14e5b..aa167be4c0 100644
> > --- a/hw/misc/aspeed_sbc.c
> > +++ b/hw/misc/aspeed_sbc.c
> > @@ -10,11 +10,13 @@
> >
> > #include "qemu/osdep.h"
> > #include "qemu/log.h"
> > +#include "qemu/cutils.h"
> > #include "qemu/error-report.h"
> > #include "hw/core/qdev-properties.h"
> > #include "hw/misc/aspeed_sbc.h"
> > #include "qapi/error.h"
> > #include "migration/vmstate.h"
> > +#include "crypto/akcipher.h"
> > #include "trace.h"
> >
> > #define R_PROT (0x000 / 4)
> > @@ -24,8 +26,22 @@
> > #define R_CAMP1 (0x020 / 4)
> > #define R_CAMP2 (0x024 / 4)
> > #define R_QSR (0x040 / 4)
> > +#define R_SEC_TRIGGER (0x0bc / 4)
> > +
> > +/*
> > + * SEC SRAM layout for a secp384r1 ECDSA verify operation. All
> > +operands are
> > + * 48-byte big-endian values.
> > + */
> > +#define ECDSA_SRAM_QX 0x2080
> > +#define ECDSA_SRAM_QY 0x20c0
> > +#define ECDSA_SRAM_R 0x21c0
> > +#define ECDSA_SRAM_S 0x2200
> > +#define ECDSA_SRAM_M 0x2240
> > +#define ECDSA_P384_COORD_LEN 48
> >
> > /* R_STATUS */
> > +#define ECDSA_VERIFY_PASS BIT(21)
> > +#define ECDSA_VERIFY_DONE BIT(20)
> > #define ABR_EN BIT(14) /* Mirrors SCU510[11] */
> > #define ABR_IMAGE_SOURCE BIT(13)
> > #define SPI_ABR_IMAGE_SOURCE BIT(12)
> > @@ -42,6 +58,10 @@
> > #define OTP_MEM_IDLE BIT(1)
> > #define OTP_COMPARE_STATUS BIT(0)
> >
> > +/* R_SEC_TRIGGER */
> > +#define ECDSA_CMD_TRIGGER BIT(1)
> > +#define RSA_CMD_TRIGGER BIT(0)
> > +
> > /* QSR */
> > #define QSR_RSA_MASK (0x3 << 12)
> > #define QSR_HASH_MASK (0x3 << 10)
> > @@ -220,10 +240,111 @@ static void aspeed_sbc_handle_command(void
> *opaque, uint32_t cmd)
> > s->regs[R_STATUS] |= (OTP_MEM_IDLE | OTP_IDLE);
> > }
> >
> > +static void sbc_ecdsa_hexdump(const char *desc, const char *buf,
> > +size_t size) {
> > + g_autoptr(GString) str = g_string_sized_new(64);
> > + size_t len;
> > + size_t i;
> > +
> > + for (i = 0; i < size; i += len) {
> > + len = MIN(16, size - i);
> > + g_string_truncate(str, 0);
> > + qemu_hexdump_line(str, buf + i, len, 1, 4);
> > + trace_aspeed_sbc_ecdsa_hexdump(desc, i, str->str);
> > + }
> > +}
> > +
> > +/*
> > + * The hardware only supports ECDSA secp384r1 (NIST P-384). The
> > +firmware has
> > + * already staged the public key, signature and digest in the SEC
> > +SRAM; read
> > + * them out and defer the actual verification to the crypto backend.
> > + */
> > +static bool aspeed_sbc_ecdsa_verify(AspeedSBCState *s) {
> > + QCryptoAkCipherOptions opts = {
> > + .alg = QCRYPTO_AK_CIPHER_ALGO_ECDSA,
> > + .u.ecdsa.curve_id = QCRYPTO_CURVE_ID_SECP384R1,
> > + };
> > + g_autoptr(QCryptoAkCipher) akcipher = NULL;
> > + uint8_t pubkey[ECDSA_P384_COORD_LEN * 2];
> > + uint8_t sig[ECDSA_P384_COORD_LEN * 2];
> > + uint8_t dgst[ECDSA_P384_COORD_LEN];
> > + Error *err = NULL;
> > +
> > + if (!qcrypto_akcipher_supports(&opts)) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: ECDSA secp384r1 is not supported by the
> crypto "
> > + "backend\n", __func__);
> > + return false;
> > + }
> > +
> > + if (address_space_read(&s->sram_as, ECDSA_SRAM_QX,
> > + MEMTXATTRS_UNSPECIFIED, pubkey,
> > + ECDSA_P384_COORD_LEN) !=
> MEMTX_OK) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: failed to read ECDSA QX from SEC SRAM\n",
> __func__);
> > + return false;
> > + }
> > + if (address_space_read(&s->sram_as, ECDSA_SRAM_QY,
> > + MEMTXATTRS_UNSPECIFIED,
> > + pubkey + ECDSA_P384_COORD_LEN,
> > + ECDSA_P384_COORD_LEN) !=
> MEMTX_OK) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: failed to read ECDSA QY from SEC SRAM\n",
> __func__);
> > + return false;
> > + }
> > + if (address_space_read(&s->sram_as, ECDSA_SRAM_R,
> > + MEMTXATTRS_UNSPECIFIED, sig,
> > + ECDSA_P384_COORD_LEN) !=
> MEMTX_OK) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: failed to read ECDSA R from SEC SRAM\n",
> __func__);
> > + return false;
> > + }
> > + if (address_space_read(&s->sram_as, ECDSA_SRAM_S,
> > + MEMTXATTRS_UNSPECIFIED, sig +
> ECDSA_P384_COORD_LEN,
> > + ECDSA_P384_COORD_LEN) !=
> MEMTX_OK) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: failed to read ECDSA S from SEC SRAM\n",
> __func__);
> > + return false;
> > + }
> > + if (address_space_read(&s->sram_as, ECDSA_SRAM_M,
> > + MEMTXATTRS_UNSPECIFIED, dgst,
> > + ECDSA_P384_COORD_LEN) !=
> MEMTX_OK) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: failed to read ECDSA M from SEC SRAM\n",
> > + __func__);
> > + return false;
> > + }
> > +
> > + if
> (trace_event_get_state_backends(TRACE_ASPEED_SBC_ECDSA_HEXDUMP)) {
> > + sbc_ecdsa_hexdump("pubkey", (char *)pubkey, sizeof(pubkey));
> > + sbc_ecdsa_hexdump("signature", (char *)sig, sizeof(sig));
> > + sbc_ecdsa_hexdump("digest", (char *)dgst, sizeof(dgst));
> > + }
> > +
> > + akcipher = qcrypto_akcipher_new(&opts,
> QCRYPTO_AK_CIPHER_KEY_TYPE_PUBLIC,
> > + pubkey, sizeof(pubkey), &err);
> > + if (!akcipher) {
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: %s\n", __func__,
> > + error_get_pretty(err));
> > + error_free(err);
> > + return false;
> > + }
> > +
> > + if (qcrypto_akcipher_verify(akcipher, sig, sizeof(sig),
> > + dgst, sizeof(dgst), &err) != 0) {
> > + error_free(err);
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > static void aspeed_sbc_write(void *opaque, hwaddr addr, uint64_t data,
> > unsigned int size)
> > {
> > AspeedSBCState *s = ASPEED_SBC(opaque);
> > + AspeedSBCClass *sc = ASPEED_SBC_GET_CLASS(s);
> >
> > addr >>= 2;
> >
> > @@ -244,6 +365,26 @@ static void aspeed_sbc_write(void *opaque, hwaddr
> addr, uint64_t data,
> > case R_CMD:
> > aspeed_sbc_handle_command(opaque, data);
> > return;
> > + case R_SEC_TRIGGER:
> > + if (data & RSA_CMD_TRIGGER) {
> > + qemu_log_mask(LOG_UNIMP,
> > + "%s: RSA is not supported\n", __func__);
> > + }
> > + if (data & ECDSA_CMD_TRIGGER) {
> > + if (!sc->has_ecdsa) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: ECDSA is not supported\n",
> __func__);
> > + return;
> > + }
> > + s->regs[R_STATUS] &= ~(ECDSA_VERIFY_DONE |
> ECDSA_VERIFY_PASS);
> > + if (aspeed_sbc_ecdsa_verify(s)) {
> > + s->regs[R_STATUS] |= ECDSA_VERIFY_PASS;
> > + }
> > + s->regs[R_STATUS] |= ECDSA_VERIFY_DONE;
> > + trace_aspeed_sbc_ecdsa_verify(
> > + (s->regs[R_STATUS] & ECDSA_VERIFY_PASS) ? "pass" :
> "fail");
> > + }
> > + return;
> > default:
> > break;
> > }
> > @@ -306,6 +447,14 @@ static void aspeed_sbc_realize(DeviceState *dev,
> Error **errp)
> > }
> > }
> >
> > + if (sc->has_ecdsa) {
> > + if (!s->sram) {
>
> Bisectability is broken because the link is only set in patch 8.
>
>
Thanks for the review and suggestion.
Will move patch 8 before this patch to fix this issue.
Thanks,
Jamin
> > + error_setg(errp, TYPE_ASPEED_SBC ": 'sram' link not set");
> > + return;
> > + }
> > + address_space_init(&s->sram_as, s->sram, TYPE_ASPEED_SBC
> ".sram");
> > + }
> > +
> > memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sbc_ops,
> s,
> > TYPE_ASPEED_SBC, 0x1000);
> >
> > @@ -325,6 +474,8 @@ static const VMStateDescription
> vmstate_aspeed_sbc = {
> > static const Property aspeed_sbc_properties[] = {
> > DEFINE_PROP_BOOL("emmc-abr", AspeedSBCState, emmc_abr, 0),
> > DEFINE_PROP_UINT32("signing-settings", AspeedSBCState,
> > signing_settings, 0),
> > + DEFINE_PROP_LINK("sram", AspeedSBCState, sram,
> > + TYPE_MEMORY_REGION, MemoryRegion *),
> > };
> >
> > static void aspeed_sbc_class_init(ObjectClass *klass, const void
> > *data) @@ -355,6 +506,7 @@ static void
> > aspeed_ast10x0_sbc_class_init(ObjectClass *klass, const void *data)
> >
> > dc->desc = "AST10X0 Secure Boot Controller";
> > sc->has_otp = true;
> > + sc->has_ecdsa = true;
> > }
> >
> > static const TypeInfo aspeed_sbc_types[] = { diff --git
> > a/hw/misc/trace-events b/hw/misc/trace-events index
> > bbec0d2178..735d11d447 100644
> > --- a/hw/misc/trace-events
> > +++ b/hw/misc/trace-events
> > @@ -95,6 +95,8 @@ aspeed_sbc_ignore_cmd(uint32_t cmd) "Ignoring
> command 0x%" PRIx32
> > aspeed_sbc_handle_cmd(uint32_t cmd, uint32_t addr, bool ret) "Handling
> command 0x%" PRIx32 " for OTP addr 0x%" PRIx32 " Result: %d"
> > aspeed_sbc_otp_read(uint32_t addr, uint32_t value) "OTP Memory read:
> addr 0x%" PRIx32 " value 0x%" PRIx32
> > aspeed_sbc_otp_prog(uint32_t addr, uint32_t value) "OTP Memory
> > write: addr 0x%" PRIx32 " value 0x%" PRIx32
> > +aspeed_sbc_ecdsa_verify(const char *result) "ECDSA verify done: %s"
> > +aspeed_sbc_ecdsa_hexdump(const char *desc, uint32_t offset, const char
> *s) "%s: 0x%08x: %s"
> >
> > # aspeed_scu.c
> > aspeed_scu_write(uint64_t offset, unsigned size, uint32_t data) "To
> > 0x%" PRIx64 " of size %u: 0x%" PRIx32
