RE: [PATCH v3 14/16] hw/misc/aspeed_hace: Support the AES-GCM mode for the crypto command

Hi Cédric

> Subject: Re: [PATCH v3 14/16] hw/misc/aspeed_hace: Support the AES-GCM
> mode for the crypto command
> 
> On 8/11/26 05:10, Jamin Lin wrote:
> > Implement the AES-GCM mode (HACE10[6:4] = 0b101) used by the AST2700
> > crypto engine: decode the GCM selection, read the 96-bit IV from the
> > context buffer, operate on the exact data length (GCM handles a
> > partial final block itself), and write the 128-bit authentication tag
> > to the tag buffer (HACE18/HACE8C). The hardware GCM path is only used
> > without associated data (the driver falls back to software otherwise),
> > so AAD is not modelled and a non-zero HACE14 is reported as
> unimplemented.
> >
> > Signed-off-by: Jamin Lin <[email protected]>
> > Reviewed-by: Kane Chen <[email protected]>
> > ---
> >   hw/misc/aspeed_hace.c | 70
> ++++++++++++++++++++++++++++++++++++++-----
> >   1 file changed, 62 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index
> > 0a6e2fa829..e455216dd4 100644
> > --- a/hw/misc/aspeed_hace.c
> > +++ b/hw/misc/aspeed_hace.c
> > @@ -31,6 +31,9 @@
> >   /* HACE0C[27:0] holds the crypto data length */
> >   #define  CRYPT_DATA_LEN_MASK    0x0FFFFFFF
> >   #define R_CRYPT_CMD         (0x10 / 4)
> > +/* AES-GCM associated data length (HACE14) and tag write buffer
> > +(HACE18) */ #define R_CRYPT_GCM_ADD_LEN (0x14 / 4)
> > +#define R_CRYPT_GCM_TAG     (0x18 / 4)
> >   /* Crypto engine command register (HACE10) bits */
> >   #define  CRYPT_CMD_ENCRYPT          BIT(7)
> >   #define  CRYPT_CMD_ISR_EN           BIT(12)
> > @@ -42,6 +45,7 @@
> >   #define  CRYPT_CMD_ECB              (0x0 << 4)
> >   #define  CRYPT_CMD_CBC              (0x1 << 4)
> >   #define  CRYPT_CMD_CTR              (0x4 << 4)
> > +#define  CRYPT_CMD_GCM              (0x5 << 4)
> >   /* AES key length HACE10[3:2] */
> >   #define  CRYPT_CMD_AES_KEY_LEN_MASK (0x3 << 2)
> >   #define  CRYPT_CMD_AES256           (0x2 << 2)
> > @@ -57,10 +61,15 @@
> >   #define CRYPT_CTX_KEY_OFFSET        0x10
> >   #define CRYPT_CTX_SIZE              0x30
> >
> > +/* AES-GCM uses a 96-bit IV and a 128-bit authentication tag */
> > +#define CRYPT_GCM_IV_LEN            12
> > +#define CRYPT_GCM_TAG_LEN          16
> > +
> >   /* AST2700 64-bit DMA high address registers for the crypto command */
> >   #define R_CRYPT_SRC_HI      (0x80 / 4)
> >   #define R_CRYPT_DEST_HI     (0x84 / 4)
> >   #define R_CRYPT_CONTEXT_HI  (0x88 / 4)
> > +#define R_CRYPT_GCM_TAG_HI  (0x8c / 4)
> >
> >   #define R_STATUS        (0x1c / 4)
> >   #define HASH_IRQ        BIT(9)
> > @@ -596,6 +605,9 @@ static bool crypt_decode_cmd(uint32_t cmd,
> QCryptoCipherAlgo *alg,
> >       case CRYPT_CMD_CTR:
> >           *mode = QCRYPTO_CIPHER_MODE_CTR;
> >           break;
> > +    case CRYPT_CMD_GCM:
> > +        *mode = QCRYPTO_CIPHER_MODE_GCM;
> > +        break;
> >       default:
> >           return false;
> >       }
> > @@ -689,11 +701,12 @@ static uint64_t crypt_get_addr(AspeedHACEState
> *s, int reg, int reg_hi)
> >   }
> >
> >   /*
> > - * Perform an AES/DES/3DES ECB/CBC operation. The source and
> > destination are
> > - * either single contiguous buffers (direct access mode) or
> > scatter-gather
> > - * lists (HACE10[18]/[19]), addressed by HACE00/HACE04; the IV/key
> > come from
> > - * the context buffer (HACE08). For CBC the resulting chaining IV is
> > written
> > - * back to the context buffer so the driver can continue the chain.
> > + * Perform an AES/DES/3DES ECB/CBC/CTR or AES-GCM operation. The
> > + source and
> > + * destination are either single contiguous buffers (direct access
> > + mode) or
> > + * scatter-gather lists (HACE10[18]/[19]), addressed by
> > + HACE00/HACE04; the
> > + * IV/key come from the context buffer (HACE08). For CBC and CTR the
> > + resulting
> > + * chaining state is written back to the context buffer so the driver
> > + can
> > + * continue; for GCM the authentication tag is written to the tag buffer.
> >    */
> >   static void do_crypt_operation(AspeedHACEState *s, uint32_t cmd)
> >   {
> > @@ -703,6 +716,7 @@ static void do_crypt_operation(AspeedHACEState *s,
> uint32_t cmd)
> >       g_autoptr(QCryptoCipher) cipher = NULL;
> >       g_autofree uint8_t *src_buf = NULL;
> >       g_autofree uint8_t *dst_buf = NULL;
> > +    uint8_t tag[CRYPT_GCM_TAG_LEN];
> >       uint8_t ctx[CRYPT_CTX_SIZE];
> >       Error *local_err = NULL;
> >       QCryptoCipherMode mode;
> > @@ -711,10 +725,13 @@ static void do_crypt_operation(AspeedHACEState
> *s, uint32_t cmd)
> >       uint64_t ctx_addr;
> >       uint64_t src_addr;
> >       uint64_t dst_addr;
> > +    uint64_t tag_addr;
> > +    uint32_t aad_len;
> >       size_t iv_offset;
> >       size_t blocklen;
> >       size_t buf_len;
> >       size_t keylen;
> > +    size_t ivlen;
> >       bool status;
> >
> >       if (len == 0) {
> > @@ -734,6 +751,20 @@ static void do_crypt_operation(AspeedHACEState *s,
> uint32_t cmd)
> >           return;
> >       }
> >
> > +    /* GCM uses a 96-bit IV; the block modes use a full-block IV. */
> > +    ivlen = (mode == QCRYPTO_CIPHER_MODE_GCM) ?
> CRYPT_GCM_IV_LEN :
> > + blocklen;
> > +
> > +    /*
> > +     * The hardware GCM path is only exercised without associated data
> (the
> > +     * driver falls back to software when there is any), so AAD is not
> modelled.
> > +     */
> > +    aad_len = s->regs[R_CRYPT_GCM_ADD_LEN] &
> CRYPT_DATA_LEN_MASK;
> 
> should R_CRYPT_GCM_ADD_LEN be masked at write time like
> R_CRYPT_DATA_LEN is ?
> 
Thanks for the review and suggestion.
Will do.

> > +    if (mode == QCRYPTO_CIPHER_MODE_GCM && aad_len != 0) {
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s: GCM associated data is not
> implemented\n", __func__);
> > +        return;
> > +    }
> > +
> >       /* Fetch the IV and key from the context buffer in DRAM. */
> >       ctx_addr = crypt_get_addr(s, R_CRYPT_CONTEXT,
> R_CRYPT_CONTEXT_HI);
> >       if (address_space_read(&s->dram_as, ctx_addr,
> > MEMTXATTRS_UNSPECIFIED, @@ -758,7 +789,7 @@ static void
> do_crypt_operation(AspeedHACEState *s, uint32_t cmd)
> >       }
> >
> >       if (mode != QCRYPTO_CIPHER_MODE_ECB &&
> > -        qcrypto_cipher_setiv(cipher, ctx + iv_offset, blocklen,
> > +        qcrypto_cipher_setiv(cipher, ctx + iv_offset, ivlen,
> >                                &local_err) < 0) {
> >           qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto cipher setiv
> failed: %s\n",
> >                         __func__, error_get_pretty(local_err)); @@
> > -769,9 +800,11 @@ static void do_crypt_operation(AspeedHACEState *s,
> uint32_t cmd)
> >       /*
> >        * Round the working buffers up to a whole block. Block modes are
> already
> >        * block-aligned; the stream-like CTR mode may leave a partial final
> block
> > -     * that the engine still processes a full block at a time.
> > +     * that the engine still processes a full block at a time. GCM handles a
> > +     * partial final block itself, so it operates on the exact length.
> >        */
> > -    buf_len = QEMU_ALIGN_UP(len, blocklen);
> > +    buf_len = (mode == QCRYPTO_CIPHER_MODE_GCM) ?
> > +              len : QEMU_ALIGN_UP(len, blocklen);
> >       src_buf = g_malloc0(buf_len);
> >       dst_buf = g_malloc0(buf_len);
> >
> > @@ -855,6 +888,24 @@ static void do_crypt_operation(AspeedHACEState *s,
> uint32_t cmd)
> >                             "%s: Failed to write IV, addr=0x%"
> HWADDR_PRIx "\n",
> >                             __func__, ctx_addr + iv_offset);
> >           }
> > +    } else if (mode == QCRYPTO_CIPHER_MODE_GCM) {
> > +        /*
> > +         * GCM authenticates the message and writes the resulting tag to
> the
> > +         * dedicated tag buffer (HACE18/HACE8C).
> > +         */
> > +        if (qcrypto_cipher_gettag(cipher, tag, sizeof(tag), &local_err) < 0)
> {
> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto cipher
> gettag failed: "
> > +                          "%s\n", __func__,
> error_get_pretty(local_err));
> > +            error_free(local_err);
> > +            return;
> > +        }
> > +        tag_addr = crypt_get_addr(s, R_CRYPT_GCM_TAG,
> R_CRYPT_GCM_TAG_HI);
> > +        if (address_space_write(&s->dram_as, tag_addr,
> MEMTXATTRS_UNSPECIFIED,
> > +                                tag, sizeof(tag))) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "%s: Failed to write tag, addr=0x%"
> HWADDR_PRIx "\n",
> > +                          __func__, tag_addr);
> > +        }
> >       }
> >   }
> >
> > @@ -985,6 +1036,9 @@ static void aspeed_hace_write(void *opaque,
> hwaddr addr, uint64_t data,
> >       case R_CRYPT_CONTEXT_HI:
> >           data &= ahc->key_hi_mask;
> >           break;
> 
> R_CRYPT_GCM_TAG writes are not masked like the other registers :
> 
>      case R_CRYPT_SRC:
>      case R_CRYPT_DEST:
>      case R_CRYPT_CONTEXT:
>          data &= ahc->src_mask;
>          break;
> 
Will do

Thanks,
Jamin

> Thanks,
> 
> C.
> 
> 
> > +    case R_CRYPT_GCM_TAG_HI:
> > +        data &= ahc->dest_hi_mask;
> > +        break;
> >       default:
> >           break;
> >       }