Re: [PATCH v4 09/28] s390x/diag: Implement DIAG 320 subcode 2

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Jul 11, 2025 at 05:10:45PM -0400, 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 <zycai@linux.ibm.com>
> ---
>  include/hw/s390x/ipl/diag320.h |  47 ++++++
>  target/s390x/diag.c            | 254 ++++++++++++++++++++++++++++++++-
>  2 files changed, 300 insertions(+), 1 deletion(-)
> 

>  int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
> @@ -191,8 +193,252 @@ out:
>      }
>  }
>  
> +static int diag_320_is_cert_valid(S390IPLCertificate qcert)
> +{
> +    int version;
> +    int rc;
> +    Error *err = NULL;
> +
> +    version = qcrypto_x509_get_cert_version(qcert.raw, qcert.size, &err);
> +    if (version < 0) {
> +        error_report_err(err);
> +        return -1;
> +    }

IMHO this isn't doing anything worthwhile given qemu accepts
any version at all here.

> +
> +    rc = qcrypto_x509_check_cert_times(qcert.raw, qcert.size, &err);
> +    if (rc != 0) {
> +        error_report_err(err);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int diag_320_get_cert_info(VCEntry *vce, S390IPLCertificate qcert,
> +                                  uint8_t **cert_der, unsigned char **key_id_data,
> +                                  void **hash_data)
> +{
> +    int algo;
> +    int rc;
> +    Error *err = NULL;
> +
> +    /* key-type */
> +    algo = qcrypto_x509_get_pk_algorithm(qcert.raw, qcert.size, &err);
> +    if (algo < 0) {
> +        error_report_err(err);
> +        return -1;
> +    }
> +    if (algo == QCRYPTO_PK_ALGO_RSA) {
> +        vce->key_type = DIAG_320_VCE_KEYTYPE_SELF_DESCRIBING;
> +    }

Shouldn't this have an "else" clause to either fill in vce->key_type,
or raise an error

> +
> +    /* certificate in DER format */
> +    rc = qcrypto_x509_convert_cert_der(qcert.raw, qcert.size,
> +                                       cert_der, &qcert.der_size, &err);
> +    if (rc < 0) {
> +        error_report_err(err);
> +        goto out;
> +    }
> +
> +    /* VC format */
> +    vce->format = DIAG_320_VCE_FORMAT_X509_DER;
> +
> +    /* key id and key id len */
> +    rc = qcrypto_x509_get_cert_key_id(qcert.raw, qcert.size,
> +                                      QCRYPTO_KEYID_FLAGS_SHA256,
> +                                      key_id_data, &qcert.key_id_size, &err);
> +    if (rc < 0) {
> +        error_report_err(err);
> +        goto out;
> +    }
> +    vce->keyid_len = cpu_to_be16(qcert.key_id_size);
> +
> +    /* hash type */
> +    if (qcert.hash_type == QCRYPTO_SIG_ALGO_RSA_SHA256) {
> +        vce->hash_type = DIAG_320_VCE_HASHTYPE_SHA2_256;
> +    }

Again, an "else" clause to either fill in vce->hash_type,
or raise an error ?

> +
> +    /* hash and hash len */
> +    *hash_data = g_malloc0(qcert.hash_size);
> +    rc = qcrypto_get_x509_cert_fingerprint(qcert.raw, qcert.size,
> +                                           QCRYPTO_HASH_ALGO_SHA256,
> +                                           *hash_data, &qcert.hash_size, &err);
> +    if (rc < 0) {
> +        error_report_err(err);
> +        goto out;
> +    }
> +    vce->hash_len = cpu_to_be16(qcert.hash_size);
> +
> +    return 0;
> +
> +out:
> +    g_clear_pointer(cert_der, g_free);
> +    g_clear_pointer(key_id_data, g_free);
> +    g_clear_pointer(hash_data, g_free);
> +
> +    return -1;
> +}



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|