On 5/14/25 12:18 PM, Thomas Huth wrote:
> On 09/05/2025 00.50, Zhuoying Cai wrote:
>> DIAG 320 subcode 2 provides verification-certificates (VCs) that are in the
>> certificate store. Only X509 certificates in DER format and SHA-256 hash
>> type are recognized.
>>
>> The subcode value is denoted by setting the second-left-most bit
>> of an 8-byte field.
>>
>> The Verification Certificate Block (VCB) contains the output data
>> when the operation completes successfully. It includes a common
>> header followed by zero or more Verification Certificate Entries (VCEs),
>> depending on the VCB input length and the VC range (from the first VC
>> index to the last VC index) in the certificate store.
>>
>> Each VCE contains information about a certificate retrieved from
>> the S390IPLCertificateStore, such as the certificate name, key type,
>> key ID length, hash length, and the raw certificate data.
>> The key ID and hash are extracted from the raw certificate by the crypto API.
>>
>> Signed-off-by: Zhuoying Cai <zy...@linux.ibm.com>
>> ---
>> crypto/x509-utils.c | 204 ++++++++++++++++++++++++++++-
>> include/crypto/x509-utils.h | 10 ++
>> include/hw/s390x/ipl/diag320.h | 47 +++++++
>> qapi/crypto.json | 20 +++
>> target/s390x/diag.c | 227 ++++++++++++++++++++++++++++++++-
>> 5 files changed, 506 insertions(+), 2 deletions(-)
>>
>> diff --git a/crypto/x509-utils.c b/crypto/x509-utils.c
>> index 0b8cfc9022..51bd75d4eb 100644
>> --- a/crypto/x509-utils.c
>> +++ b/crypto/x509-utils.c
>> @@ -129,6 +129,7 @@ int qcrypto_get_x509_cert_fingerprint(uint8_t *cert,
>> size_t size,
>> int hlen;
>> gnutls_x509_crt_t crt;
>> gnutls_datum_t datum = {.data = cert, .size = size};
>> + gnutls_x509_crt_fmt_t fmt;
>>
>> if (alg >= G_N_ELEMENTS(qcrypto_to_gnutls_hash_alg_map)) {
>> error_setg(errp, "Unknown hash algorithm");
>> @@ -140,9 +141,15 @@ int qcrypto_get_x509_cert_fingerprint(uint8_t *cert,
>> size_t size,
>> return -1;
>> }
>>
>> + fmt = qcrypto_get_x509_cert_fmt(cert, size, errp);
>> + if (fmt == -1) {
>> + error_setg(errp, "Certificate is neither in DER or PEM format");
>
> qcrypto_get_x509_cert_fmt() already has an errp parameter, so I'd expect
> that the qcrypto_get_x509_cert_fmt() function already sets up errp in case
> of errors, doesn't it? In that case you should not set errp here again, I
> think.
>
>> + return -1;
>> + }
>> +
>> gnutls_x509_crt_init(&crt);
>>
>> - if (gnutls_x509_crt_import(crt, &datum, GNUTLS_X509_FMT_PEM) != 0) {
>> + if (gnutls_x509_crt_import(crt, &datum, fmt) != 0) {
>> error_setg(errp, "Failed to import certificate");
>> goto cleanup;
>> }
>> @@ -199,6 +206,173 @@ cleanup:
>> return rc;
>> }
>>
>> +int qcrypto_get_x509_cert_version(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");
>
> dito?
>
>> + 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;
>> + }
>> +
>> + rc = gnutls_x509_crt_get_version(crt);
>> +
>> +cleanup:
>> + gnutls_x509_crt_deinit(crt);
>> + return rc;
>> +}
>> +
>> +int qcrypto_check_x509_cert_times(uint8_t *cert, size_t size, Error **errp)
>> +{
>> + int rc = -1;
>> + gnutls_x509_crt_t crt;
>> + gnutls_datum_t datum = {.data = cert, .size = size};
>> + time_t now = time(0);
>> + gnutls_x509_crt_fmt_t fmt;
>> +
>> + if (now == ((time_t)-1)) {
>> + error_setg(errp, "Cannot get current time");
>
> Maybe use error_setg_errno() here to get the information from errno, too?
>
>> + return rc;
>> + }
>> +
>> + fmt = qcrypto_get_x509_cert_fmt(cert, size, errp);
>> + if (fmt == -1) {
>> + error_setg(errp, "Certificate is neither in DER or PEM format");
>
> This is again ignoring the errp from qcrypto_get_x509_cert_fmt().
>
>> + 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;
>> + }
>> +
>> + if (gnutls_x509_crt_get_expiration_time(crt) < now) {
>> + error_setg(errp, "The certificate has expired");
>> + goto cleanup;
>> + }
>> +
>> + if (gnutls_x509_crt_get_activation_time(crt) > now) {
>> + error_setg(errp, "The certificate is not yet active");
>> + goto cleanup;
>> + }
>
> gnutls_x509_crt_get_expiration_time() and
> gnutls_x509_crt_get_activation_time() can both return -1 on errors. I think
> you should take that into account in the checks here, too.
>
>> + rc = 0;
>> +
>> +cleanup:
>> + gnutls_x509_crt_deinit(crt);
>> + return rc;
>> +}
>> +
>> +int qcrypto_get_x509_pk_algorithm(uint8_t *cert, size_t size, Error **errp)
>> +{
>> + int rc = -1;
>> + unsigned int bits;
>> + 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;
>> + }
>> +
>> + rc = gnutls_x509_crt_get_pk_algorithm(crt, &bits);
>> +
>> +cleanup:
>> + gnutls_x509_crt_deinit(crt);
>> + return rc;
>> +}
>> +
>> +int qcrypto_get_x509_cert_key_id(uint8_t *cert, size_t size,
>> + QCryptoKeyidFlags flag,
>> + uint8_t *result,
>> + size_t *resultlen,
>> + Error **errp)
>> +{
>> + int ret = -1;
>> + int keyid_len;
>> + gnutls_x509_crt_t crt;
>> + gnutls_datum_t datum = {.data = cert, .size = size};
>> + gnutls_x509_crt_fmt_t fmt;
>> +
>> + if (flag >= G_N_ELEMENTS(qcrypto_to_gnutls_keyid_flags_map)) {
>> + error_setg(errp, "Unknown key id flag");
>> + return -1;
>> + }
>> +
>> + if (result == NULL) {
>> + error_setg(errp, "No valid buffer given");
>> + return -1;
>> + }
>
> This check sounds like it could also be a simple g_assert() statement instead?
>
g_assert() may not be ideal here, as it will terminate the guest if the
assertion fails, which is not the intended behavior.
[...]
