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 <[email protected]>
>> ---
>> 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
