Thanks for the feedback!
On 1/9/26 7:41 AM, Thomas Huth wrote:
> On 08/12/2025 22.32, 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.
>>
>> Note: SHA2-256 VC hash type is required for retrieving the hash
>> (fingerprint) of the certificate.
>>
>> Signed-off-by: Zhuoying Cai <[email protected]>
>> ---
> ...
> > +struct VCEntry {
> > + uint32_t len;
> > + uint8_t flags;
> > + uint8_t key_type;
> > + uint16_t cert_idx;
> > + uint32_t name[16];
>
> Why is this defined as an array of uint32_t when it is rather a string
> instead?
>
This field contains the 64-byte name of the certificate in EBCDIC
format. I think it would be more appropriate to define it as uint8_t
name[64].
> > + uint8_t format;
> > + uint8_t reserved0;
> > + uint16_t keyid_len;
> > + uint8_t reserved1;
> > + uint8_t hash_type;
> > + uint16_t hash_len;
> > + uint32_t reserved2;
> > + uint32_t cert_len;
> > + uint32_t reserved3[2];
> > + uint16_t hash_offset;
> > + uint16_t cert_offset;
> > + uint32_t reserved4[7];
> > + uint8_t cert_buf[];
> > +};
> > +typedef struct VCEntry VCEntry;
> ...
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index 0e1897e03d..1498b29a0d 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -17,6 +17,7 @@
>> #include "s390x-internal.h"
>> #include "hw/watchdog/wdt_diag288.h"
>> #include "system/cpus.h"
>> +#include "hw/s390x/cert-store.h"
>> #include "hw/s390x/ipl.h"
>> #include "hw/s390x/ipl/diag320.h"
>> #include "hw/s390x/s390-virtio-ccw.h"
>> @@ -24,6 +25,7 @@
>> #include "kvm/kvm_s390x.h"
>> #include "target/s390x/kvm/pv.h"
>> #include "qemu/error-report.h"
>> +#include "crypto/x509-utils.h"
>>
>>
>> static inline bool diag_parm_addr_valid(uint64_t addr, size_t size, bool
>> write)
>> @@ -231,8 +233,330 @@ static int handle_diag320_query_vcsi(S390CPU *cpu,
>> uint64_t addr, uint64_t r1,
>> return DIAG_320_RC_OK;
>> }
>>
>> +static bool is_cert_valid(S390IPLCertificate cert)
>
> You're using call-by-value for all S390IPLCertificate parameters in this
> patch ... that's quite cumbersome since the struct has a size of ~80 bytes.
>
> Unless there is a very good reason for doing so, I'd suggest that you switch
> the code to use (const) pointers instead of passing around that struct on
> the stack.
>
>> +{
>> + int rc;
>> + Error *err = NULL;
>> +
>> + rc = qcrypto_x509_check_cert_times(cert.raw, cert.size, &err);
>> + if (rc != 0) {
>> + error_report_err(err);
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static void handle_key_id(VCEntry *vce, S390IPLCertificate cert)
>> +{
>> + int rc;
>> + g_autofree unsigned char *key_id_data = NULL;
>> + size_t key_id_len;
>> + Error *err = NULL;
>> +
>> + key_id_len = CERT_KEY_ID_LEN;
>> + /* key id and key id len */
>> + rc = qcrypto_x509_get_cert_key_id(cert.raw, cert.size,
>> + QCRYPTO_HASH_ALGO_SHA256,
>> + &key_id_data, &key_id_len, &err);
>> + if (rc < 0) {
>> + error_report_err(err);
>> + return;
>> + }
>> +
>> + if (VCE_HEADER_LEN + key_id_len > be32_to_cpu(vce->len)) {
>> + error_report("Unable to write key ID: exceeds buffer bounds");
>> + return;
>> + }
>> +
>> + vce->keyid_len = cpu_to_be16(key_id_len);
>> +
>> + memcpy(vce->cert_buf, key_id_data, key_id_len);
>> +}
>> +
>> +static int handle_hash(VCEntry *vce, S390IPLCertificate cert, uint16_t
>> keyid_field_len)
>> +{
>> + int rc;
>> + uint16_t hash_offset;
>> + g_autofree void *hash_data = NULL;
>> + size_t hash_len;
>> + Error *err = NULL;
>> +
>> + hash_len = CERT_HASH_LEN;
>> + /* hash and hash len */
>> + hash_data = g_malloc0(hash_len);
>> + rc = qcrypto_get_x509_cert_fingerprint(cert.raw, cert.size,
>> + QCRYPTO_HASH_ALGO_SHA256,
>> + hash_data, &hash_len, &err);
>> + if (rc < 0) {
>> + error_report_err(err);
>> + return -1;
>> + }
>> +
>> + hash_offset = VCE_HEADER_LEN + keyid_field_len;
>> + if (hash_offset + hash_len > be32_to_cpu(vce->len)) {
>> + error_report("Unable to write hash: exceeds buffer bounds");
>> + return -1;
>> + }
>> +
>> + vce->hash_len = cpu_to_be16(hash_len);
>> + vce->hash_type = DIAG_320_VCE_HASHTYPE_SHA2_256;
>> + vce->hash_offset = cpu_to_be16(hash_offset);
>> +
>> + memcpy((uint8_t *)vce + hash_offset, hash_data, hash_len);
>> +
>> + return 0;
>> +}
>> +
>> +static int handle_cert(VCEntry *vce, S390IPLCertificate cert, uint16_t
>> hash_field_len)
>> +{
>> + int rc;
>> + uint16_t cert_offset;
>> + g_autofree uint8_t *cert_der = NULL;
>> + Error *err = NULL;
>> +
>> + /* certificate in DER format */
>> + rc = qcrypto_x509_convert_cert_der(cert.raw, cert.size,
>> + &cert_der, &cert.der_size, &err);
>> + if (rc < 0) {
>> + error_report_err(err);
>> + return -1;
>> + }
>> +
>> + cert_offset = be16_to_cpu(vce->hash_offset) + hash_field_len;
>> + if (cert_offset + cert.der_size > be32_to_cpu(vce->len)) {
>> + error_report("Unable to write certificate: exceeds buffer bounds");
>> + return -1;
>> + }
>> +
>> + vce->format = DIAG_320_VCE_FORMAT_X509_DER;
>> + vce->cert_len = cpu_to_be32(cert.der_size);
>> + vce->cert_offset = cpu_to_be16(cert_offset);
>> +
>> + memcpy((uint8_t *)vce + cert_offset, cert_der, cert.der_size);
>> +
>> + return 0;
>> +}
>> +
>> +static int get_key_type(S390IPLCertificate cert)
>> +{
>> + int algo;
>> + int rc;
>> + Error *err = NULL;
>> +
>> + /* public key algorithm */
>> + algo = qcrypto_x509_get_pk_algorithm(cert.raw, cert.size, &err);
>> + if (algo < 0) {
>> + error_report_err(err);
>> + return -1;
>> + }
>> +
>> + if (algo == QCRYPTO_PK_ALGO_ECDSA) {
>> + rc = qcrypto_x509_check_ecc_curve_p521(cert.raw, cert.size, &err);
>> + if (rc == -1) {
>> + error_report_err(err);
>> + return -1;
>> + }
>> +
>> + return (rc == 1) ? DIAG_320_VCE_KEYTYPE_ECDSA_P521 :
>> + DIAG_320_VCE_KEYTYPE_SELF_DESCRIBING;
>> + }
>> +
>> + return DIAG_320_VCE_KEYTYPE_SELF_DESCRIBING;
>> +}
>> +
>> +static int build_vce_header(VCEntry *vce, S390IPLCertificate cert, int idx)
>> +{
>> + int key_type;
>> +
>> + vce->len = cpu_to_be32(VCE_HEADER_LEN);
>> + vce->cert_idx = cpu_to_be16(idx + 1);
>> + strncpy((char *)vce->name, (char *)cert.vc_name, VC_NAME_LEN_BYTES);
>
> strncpy is often tripping up static analyzers when you use it like this.
> Please consider using memcpy(), pstrcpy() or strpadcpy() instead.
> (I guess memcpy is the right thing to use here since vc_name has already
> been initialized with strpadcpy ?)
>
>> + key_type = get_key_type(cert);
>> + if (key_type == -1) {
>> + return -1;
>> + }
>> + vce->key_type = key_type;
>> +
>> + return 0;
>> +}
>
> Thomas
>
