Re: [PATCH v2 02/25] hw/s390x/ipl: Create certificate store

[Zhuoying Cai <zycai@linux.ibm.com>]

On 5/14/25 5:03 AM, Daniel P. Berrangé wrote:
> On Thu, May 08, 2025 at 06:50:18PM -0400, Zhuoying Cai wrote:
>> Create a certificate store for boot certificates used for secure IPL.
>>
>> Load certificates from the -boot-certificate option into the cert store.
>>
>> Currently, only x509 certificates in DER format and uses SHA-256 hashing
>> algorithm are supported, as these are the types required for secure boot
>> on s390.
>>
>> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
>> ---
>>  crypto/meson.build          |   5 +-
>>  crypto/x509-utils.c         | 163 ++++++++++++++++++++++++
>>  hw/s390x/cert-store.c       | 242 ++++++++++++++++++++++++++++++++++++
>>  hw/s390x/cert-store.h       |  39 ++++++
>>  hw/s390x/ipl.c              |   9 ++
>>  hw/s390x/ipl.h              |   3 +
>>  hw/s390x/meson.build        |   1 +
>>  include/crypto/x509-utils.h |   6 +
>>  include/hw/s390x/ipl/qipl.h |   3 +
>>  qapi/crypto.json            |  80 ++++++++++++
> 
> 
> Please split the crypto subsystem changes from the s390x subsystem
> changes, as separate commits. Also be sure to CC myself (as crypto
> maintainer) on patches that change the crypto subsystem.
> 
> 
>> diff --git a/crypto/x509-utils.c b/crypto/x509-utils.c
>> index 8bad00a51b..0b8cfc9022 100644
>> --- a/crypto/x509-utils.c
>> +++ b/crypto/x509-utils.c
>> @@ -11,6 +11,12 @@
>>  #include "qemu/osdep.h"
>>  #include "qapi/error.h"
>>  #include "crypto/x509-utils.h"
>> +
>> +/*
>> + * Surround with GNUTLS marco to ensure the interfaces are
>> + * still available when GNUTLS is not enabled.
> 
> This comment is redundant - we don't need to explain
> what an #ifdef does.
> 
>> + */
>> +#ifdef CONFIG_GNUTLS
>>  #include <gnutls/gnutls.h>
>>  #include <gnutls/crypto.h>
>>  #include <gnutls/x509.h>
>> @@ -25,6 +31,94 @@ static const int qcrypto_to_gnutls_hash_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
>>      [QCRYPTO_HASH_ALGO_RIPEMD160] = GNUTLS_DIG_RMD160,
>>  };
>>  
>> +static const int qcrypto_to_gnutls_keyid_flags_map[QCRYPTO_KEYID_FLAGS__MAX] = {
>> +    [QCRYPTO_KEYID_FLAGS_SHA1] = GNUTLS_KEYID_USE_SHA1,
>> +    [QCRYPTO_KEYID_FLAGS_SHA256] = GNUTLS_KEYID_USE_SHA256,
>> +    [QCRYPTO_KEYID_FLAGS_SHA512] = GNUTLS_KEYID_USE_SHA512,
>> +    [QCRYPTO_KEYID_FLAGS_BEST_KNOWN] = GNUTLS_KEYI_DUSE_BEST_KNOWN,
>> +};
>> +
>> +static const int qcrypto_to_gnutls_cert_fmt_map[QCRYPTO_CERT_FMT__MAX] = {
>> +    [QCRYPTO_CERT_FMT_DER] = GNUTLS_X509_FMT_DER,
>> +    [QCRYPTO_CERT_FMT_PEM] = GNUTLS_X509_FMT_PEM,
>> +};
>> +
>> +int qcrypto_check_x509_cert_fmt(uint8_t *cert, size_t size,
>> +                                 QCryptoCertFmt fmt, Error **errp)
>> +{
>> +    int rc;
>> +    int ret = 0;
>> +    gnutls_x509_crt_t crt;
>> +    gnutls_datum_t datum = {.data = cert, .size = size};
>> +
>> +    if (fmt >= G_N_ELEMENTS(qcrypto_to_gnutls_cert_fmt_map)) {
>> +        error_setg(errp, "Unknown certificate format");
>> +        return ret;
>> +    }
>> +
>> +    if (gnutls_x509_crt_init(&crt) < 0) {
>> +        error_setg(errp, "Failed to initialize certificate");
>> +        goto cleanup;
>> +    }
>> +
>> +    rc = gnutls_x509_crt_import(crt, &datum, qcrypto_to_gnutls_cert_fmt_map[fmt]);
>> +    if (rc == GNUTLS_E_ASN1_TAG_ERROR) {
>> +        ret = 0;
>> +        goto cleanup;
>> +    }
>> +
>> +    ret = 1;
>> +
>> +cleanup:
>> +    gnutls_x509_crt_deinit(crt);
>> +    return ret;
>> +}
>> +
>> +static int qcrypto_get_x509_cert_fmt(uint8_t *cert, size_t size, Error **errp)
>> +{
>> +    int fmt;
>> +
>> +    if (qcrypto_check_x509_cert_fmt(cert, size, QCRYPTO_CERT_FMT_DER, errp)) {
>> +        fmt = GNUTLS_X509_FMT_DER;
>> +    } else if (qcrypto_check_x509_cert_fmt(cert, size, QCRYPTO_CERT_FMT_PEM, errp)) {
>> +        fmt = GNUTLS_X509_FMT_PEM;
>> +    } else {
>> +        return -1;
>> +    }
>> +
>> +    return fmt;
>> +}
>> +
>> +int qcrypto_get_x509_hash_len(QCryptoHashAlgo alg, Error **errp)
>> +{
>> +    if (alg >= G_N_ELEMENTS(qcrypto_to_gnutls_hash_alg_map)) {
>> +        error_setg(errp, "Unknown hash algorithm");
>> +        return 0;
>> +    }
>> +
>> +    return gnutls_hash_get_len(qcrypto_to_gnutls_hash_alg_map[alg]);
>> +}
>> +
>> +int qcrypto_get_x509_keyid_len(QCryptoKeyidFlags flag, Error **errp)
>> +{
>> +    QCryptoHashAlgo alg;
>> +
>> +    if (flag >= G_N_ELEMENTS(qcrypto_to_gnutls_keyid_flags_map)) {
>> +        error_setg(errp, "Unknown key id flag");
>> +        return 0;
>> +    }
>> +
>> +    alg = QCRYPTO_HASH_ALGO_SHA1;
>> +    if ((flag & qcrypto_to_gnutls_keyid_flags_map[QCRYPTO_KEYID_FLAGS_SHA512]) ||
>> +        (flag & qcrypto_to_gnutls_keyid_flags_map[QCRYPTO_KEYID_FLAGS_BEST_KNOWN])) {
>> +        alg = QCRYPTO_HASH_ALGO_SHA512;
>> +    } else if (flag & qcrypto_to_gnutls_keyid_flags_map[QCRYPTO_KEYID_FLAGS_SHA256]) {
>> +        alg = QCRYPTO_HASH_ALGO_SHA256;
>> +    }
>> +
>> +    return qcrypto_get_x509_hash_len(alg, errp);
>> +}
> 
> This method looks fairly pointless to me. Why doesn't the caller just
> directly call qcrypto_get_x509_hash_len without this indirection of
> QCRYPTO_KEYID_FLAGS, especially given that you've just hardcoded to
> use of QCRYPTO_KEYID_FLAGS_SHA256 in the caller ?
> 

This function will be used in a later patch related to DIAG320 subcode
support, where we need to obtain the certificate key ID. That process
requires calculating the key ID length based on the QCryptoKeyidFlags value.

It's a mistake that I hardcoded QCRYPTO_KEYID_FLAGS_SHA256 in the later
patch instead of using the QCryptoKeyidFlags parameter. I’ll correct
that in the next version.

>> @@ -74,3 +168,72 @@ int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, size_t size,
>>      gnutls_x509_crt_deinit(crt);
>>      return ret;
>>  }
>> +
>> +int qcrypto_get_x509_signature_algorithm(uint8_t *cert, size_t size, Error **errp)
>> +{
>> +    int rc = -1;
>> +    gnutls_x509_crt_t crt;
>> +    gnutls_datum_t datum = {.data = cert, .size = size};
>> +    gnutls_x509_crt_fmt_t fmt;
>> +
>> +    fmt = qcrypto_get_x509_cert_fmt(cert, size, errp);
>> +    if (fmt == -1) {
>> +        error_setg(errp, "Certificate is neither in DER or PEM format");
>> +        return rc;
>> +    }
>> +
>> +    if (gnutls_x509_crt_init(&crt) < 0) {
>> +        error_setg(errp, "Failed to initialize certificate");
>> +        return rc;
>> +    }
>> +
>> +    if (gnutls_x509_crt_import(crt, &datum, fmt) != 0) {
>> +        error_setg(errp, "Failed to import certificate");
>> +        goto cleanup;
>> +    }
> 
> This code pattern looks dubious to me.  The qcrypto_get_x509_cert_fmt
> method will call qcrypto_check_x509_cert_fmt which will call
> gnutls_x509_crt_import(), the result of which is then thrown
> away, and this method calls gnutls_x509_crt_import again. Get
> rid of qcrypto_check_x509_cert_fmt from this. 
> 
>> +
>> +    rc = gnutls_x509_crt_get_signature_algorithm(crt);
> 
> This needs to be remapped to the QCryptoSigAlgo enum values.
> 
>> +
>> +cleanup:
>> +    gnutls_x509_crt_deinit(crt);
>> +    return rc;
>> +}
>> +
>> +#else /* ! CONFIG_GNUTLS */
>> +
>> +int qcrypto_check_x509_cert_fmt(uint8_t *cert, size_t size,
>> +                                 QCryptoCertFmt fmt, Error **errp)
>> +{
>> +    error_setg(errp, "To get certificate format requires GNUTLS");
>> +    return -ENOTSUP;
>> +}
>> +
>> +int qcrypto_get_x509_hash_len(QCryptoHashAlgo alg, Error **errp)
>> +{
>> +    error_setg(errp, "To get hash length requires GNUTLS");
>> +    return -ENOTSUP;
>> +}
>> +
>> +int qcrypto_get_x509_keyid_len(QCryptoKeyidFlags flag, Error **errp)
>> +{
>> +    error_setg(errp, "To get key ID length requires GNUTLS");
>> +    return -ENOTSUP;
>> +}
>> +
>> +int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, size_t size,
>> +                                      QCryptoHashAlgo hash,
>> +                                      uint8_t *result,
>> +                                      size_t *resultlen,
>> +                                      Error **errp)
>> +{
>> +    error_setg(errp, "To get fingerprint requires GNUTLS");
>> +    return -ENOTSUP;
>> +}
>> +
>> +int qcrypto_get_x509_signature_algorithm(uint8_t *cert, size_t size, Error **errp)
>> +{
>> +    error_setg(errp, "To get signature algorithm requires GNUTLS");
>> +    return -ENOTSUP;
>> +}
>> +
>> +#endif /* ! CONFIG_GNUTLS */
>> diff --git a/include/crypto/x509-utils.h b/include/crypto/x509-utils.h
>> index 1e99661a71..8fb263b9e1 100644
>> --- a/include/crypto/x509-utils.h
>> +++ b/include/crypto/x509-utils.h
>> @@ -19,4 +19,10 @@ int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, size_t size,
>>                                        size_t *resultlen,
>>                                        Error **errp);
>>  
>> +int qcrypto_check_x509_cert_fmt(uint8_t *cert, size_t size,
>> +                                 QCryptoCertFmt fmt, Error **errp);
>> +int qcrypto_get_x509_hash_len(QCryptoHashAlgo alg, Error **errp);
>> +int qcrypto_get_x509_keyid_len(QCryptoKeyidFlags flag, Error **errp);
>> +int qcrypto_get_x509_signature_algorithm(uint8_t *cert, size_t size, Error **errp);
> 
> Please add API docs for each method.
> 
> 
>> diff --git a/qapi/crypto.json b/qapi/crypto.json
>> index c9d967d782..2bbf29affe 100644
>> --- a/qapi/crypto.json
>> +++ b/qapi/crypto.json
>> @@ -612,3 +612,83 @@
>>    'base': { 'alg': 'QCryptoAkCipherAlgo' },
>>    'discriminator': 'alg',
>>    'data': { 'rsa': 'QCryptoAkCipherOptionsRSA' }}
>> +
>> +##
>> +# @QCryptoKeyidFlags:
>> +#
>> +# The supported flags for the key ID
>> +#
>> +# @sha1: SHA-1
>> +#
>> +# @sha256: SHA-256
>> +#
>> +# @sha512: SHA-512
>> +#
>> +# @best-known: BEST-KNOWN
>> +#
>> +# Since: 9.2
>> +##
>> +{ 'enum': 'QCryptoKeyidFlags',
>> +  'data': ['sha1', 'sha256', 'sha512', 'best-known']}
>> +
>> +##
>> +# @QCryptoCertFmt:
>> +#
>> +# The supported certificate encoding formats
>> +#
>> +# @der: DER
>> +#
>> +# @pem: PEM
>> +#
>> +# Since: 9.2
> 
> We're in the 10.1 dev cycle
> 
> 
> With regards,
> Daniel