Re: [PATCH v1 07/24] s390x/diag: Implement DIAG 320 subcode 2

[Collin Walling <walling@linux.ibm.com>]

On 4/8/25 11:55 AM, Zhuoying Cai wrote:
> DIAG 320 subcode 2 provides certificates that are in the
> certificate store.
> 
> 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. VCB includes a common
> header followed by zero or more verification-certificate entries (VCEs).
> 

Please add a comment stating that only x509 certificates in DER format
and SHA-256 hash type are recognized.

Further, I would further explain what this 320 SC 2 implementation is
doing either in this commit message or where appropriate as comments:
 - DIAG 320 uses a one-origin index for cert entries
 - Describe the data structures in some more detail (mainly the more
obscure fields)
 - Describe why the VCB header is written to userspace last
 - Describe which fields are taken from the certificate and stored in
the data structures
 - State that these certificates are retrieved from your cert store impl

> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> ---
>  include/hw/s390x/ipl/diag320.h |  59 +++++++++
>  target/s390x/diag.c            | 227 ++++++++++++++++++++++++++++++++-
>  2 files changed, 285 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/s390x/ipl/diag320.h b/include/hw/s390x/ipl/diag320.h
> index ded336df25..32b6914b3b 100644
> --- a/include/hw/s390x/ipl/diag320.h
> +++ b/include/hw/s390x/ipl/diag320.h
> @@ -14,15 +14,24 @@
>  
>  #define DIAG_320_SUBC_QUERY_ISM     0
>  #define DIAG_320_SUBC_QUERY_VCSI    1
> +#define DIAG_320_SUBC_STORE_VC      2
>  
>  #define DIAG_320_RC_OK              0x0001
>  #define DIAG_320_RC_NOMEM           0x0202
> +#define DIAG_320_RC_INVAL_VCB_LEN   0x0204
> +#define DIAG_320_RC_BAD_RANGE       0x0302
>  
>  #define VCSSB_MAX_LEN   128
>  #define VCE_HEADER_LEN  128
>  #define VCB_HEADER_LEN  64
>  
>  #define DIAG_320_ISM_QUERY_VCSI     0x4000000000000000
> +#define DIAG_320_ISM_STORE_VC       0x2000000000000000
> +
> +#define DIAG_320_VCE_FLAGS_VALID                0x80
> +#define DIAG_320_VCE_KEYTYPE_SELF_DESCRIBING    0
> +#define DIAG_320_VCE_FORMAT_X509_DER            1
> +#define DIAG_320_VCE_HASHTYPE_SHA2_256          1
>  
>  struct VerificationCertificateStorageSizeBlock {
>      uint32_t length;
> @@ -41,4 +50,54 @@ struct VerificationCertificateStorageSizeBlock {
>  typedef struct VerificationCertificateStorageSizeBlock \
>  VerificationCertificateStorageSizeBlock;
>  
> +struct vcb_header {
> +    uint32_t vcbinlen;
> +    uint32_t reserved0;
> +    uint16_t fvci;> +    uint16_t lvci;
> +    uint32_t reserved1;
> +    uint32_t cstoken;
> +    uint32_t reserved2[3];
> +    uint32_t vcboutlen;
> +    uint8_t reserved3[3];
> +    uint8_t version;
> +    uint16_t svcc;
> +    uint16_t rvcc;
> +    uint32_t reserved4[5];
> +} QEMU_PACKED;
> +typedef struct vcb_header vcb_header;

s/vcbinlen/in_len
s/vcboutlen/out_len

s/fvci/first_vc_index
s/lvci/last_vc_index

s/cstoken/cs_token, which I guess we don't make use of at all... does it
make sense to just remove it and combine the reserved fields?

s/svcc/stored_ct
s/rvcc/remain_ct

This will make things much easier to understand.

> +
> +struct VerficationCertificateBlock {
> +    vcb_header vcb_hdr;
> +    uint8_t vcb_buf[];

s/vcb_buf/vce_buf

What is the justification of having a separate header struct?  Can't we
just put everything in the VerificationCertificateBlock?  It would make
some of the accessing in the functions below a LOT cleaner.

> +} QEMU_PACKED;
> +typedef struct VerficationCertificateBlock VerficationCertificateBlock;
> +
> +struct vce_header {
> +    uint32_t len;
> +    uint8_t flags;
> +    uint8_t keytype;
> +    uint16_t certidx;
> +    uint32_t name[16];
> +    uint8_t format;
> +    uint8_t reserved0;
> +    uint16_t keyidlen;
> +    uint8_t reserved1;
> +    uint8_t hashtype;
> +    uint16_t hashlen;
> +    uint32_t reserved2;
> +    uint32_t certlen;
> +    uint32_t reserved3[2];
> +    uint16_t hashoffset;
> +    uint16_t certoffset;
> +    uint32_t reserved4[7];

Use underscores to separate words in these fields to make them easier on
the eyes.

> +} QEMU_PACKED;
> +typedef struct vce_header vce_header;
> +
> +struct VerificationCertificateEntry {
> +    vce_header vce_hdr;
> +    uint8_t cert_data_buf[];

s/cert_data_buf/cert_buf

and same comments as above w.r.t. to putting all header fields in here

> +} QEMU_PACKED;
> +typedef struct VerificationCertificateEntry VerificationCertificateEntry;
> +
>  #endif
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index cc639819ec..82e4dc9e1e 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/s390-virtio-ccw.h"
>  #include "system/kvm.h"
> @@ -191,6 +192,94 @@ out:
>      }
>  }
>  
> +#ifdef CONFIG_GNUTLS
> +static bool diag_320_is_cert_valid(gnutls_x509_crt_t cert)
> +{
> +    time_t now;
> +
> +    if (gnutls_x509_crt_get_version(cert) < 0) {
> +        return false;
> +    }
> +
> +    now = time(0);
> +    if (!((gnutls_x509_crt_get_activation_time(cert) < now) &&
> +         (gnutls_x509_crt_get_expiration_time(cert) > now))) {
> +        return false;
> +    }
> +
> +    return true;
> +}

qcrypto_tls_creds_check_cert_times may suffice for now.

> +#endif /* CONFIG_GNUTLS */
> +
> +static int diag_320_get_cert_info(VerificationCertificateEntry *vce,
> +                                 S390IPLCertificate qcert, bool *is_valid,
> +                                 unsigned char **key_id_data, void **hash_data)
> +{
> +#ifdef CONFIG_GNUTLS
> +    unsigned int algo;
> +    unsigned int bits;
> +    int hash_type;
> +    int rc;
> +
> +    gnutls_x509_crt_t g_cert = NULL;
> +    if (g_init_cert((uint8_t *)qcert.raw, qcert.size, &g_cert)) {
> +        return -1;
> +    }
> +
> +    /* VCE flag (validity) */
> +    *is_valid = diag_320_is_cert_valid(g_cert);
> +
> +    /* key-type */
> +    algo = gnutls_x509_crt_get_pk_algorithm(g_cert, &bits);
> +    if (algo == GNUTLS_PK_RSA) {
> +        vce->vce_hdr.keytype = DIAG_320_VCE_KEYTYPE_SELF_DESCRIBING;
> +    }
> +
> +    /* VC format */
> +    if (qcert.format == GNUTLS_X509_FMT_DER) {
> +        vce->vce_hdr.format = DIAG_320_VCE_FORMAT_X509_DER;
> +    }
> +
> +    /* key id and key id len */
> +    *key_id_data = g_malloc0(qcert.key_id_size);
> +    rc = gnutls_x509_crt_get_key_id(g_cert, GNUTLS_KEYID_USE_SHA256,
> +                                    *key_id_data, &qcert.key_id_size);
> +    if (rc < 0) {
> +        error_report("Fail to retrieve certificate key ID");
> +        goto out;
> +    }
> +    vce->vce_hdr.keyidlen = (uint16_t)qcert.key_id_size;
> +
> +    /* hash type */
> +    hash_type = gnutls_x509_crt_get_signature_algorithm(g_cert);
> +    if (hash_type == GNUTLS_SIGN_RSA_SHA256) {
> +        vce->vce_hdr.hashtype = DIAG_320_VCE_HASHTYPE_SHA2_256;
> +    }
> +
> +    /* hash and hash len */
> +    *hash_data = g_malloc0(qcert.hash_size);
> +    rc = gnutls_x509_crt_get_fingerprint(g_cert, GNUTLS_DIG_SHA256,
> +                                            *hash_data, &qcert.hash_size);
> +    if (rc < 0) {
> +        error_report("Fail to retrieve certificate hash");
> +        goto out;
> +    }
> +    vce->vce_hdr.hashlen = (uint16_t)qcert.hash_size;
> +
> +    gnutls_x509_crt_deinit(g_cert);
> +
> +    return 0;
> +out:
> +    gnutls_x509_crt_deinit(g_cert);
> +    g_free(*key_id_data);
> +    g_free(*hash_data);
> +
> +    return -1;
> +#else
> +    return -1;
> +#endif /* CONFIG_GNUTLS */

As per Daniel's suggestion, let's move all gnutls functions to somewhere
in the crypto/* directory.  There seems to be a file aptly titled
"x509-utils.c" that may be a good starting point to contribute to.

A lot of this function should get offloaded to new/existing qcrypto
functions, so I'll reserve my comments for now.

> +}
> +
>  void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>  {
>      S390CPU *cpu = env_archcpu(env);
> @@ -211,7 +300,7 @@ void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>  
>      switch (subcode) {
>      case DIAG_320_SUBC_QUERY_ISM:
> -        uint64_t ism = DIAG_320_ISM_QUERY_VCSI;
> +        uint64_t ism = DIAG_320_ISM_QUERY_VCSI | DIAG_320_ISM_STORE_VC;
>  
>          if (s390_cpu_virt_mem_write(cpu, addr, (uint8_t)r1, &ism,
>                                      be64_to_cpu(sizeof(ism)))) {
> @@ -257,6 +346,142 @@ void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>          }
>          rc = DIAG_320_RC_OK;
>          break;
> +    case DIAG_320_SUBC_STORE_VC:
> +        VerficationCertificateBlock *vcb;
> +        size_t vce_offset = VCB_HEADER_LEN;
> +        size_t remaining_space;
> +        size_t vce_hdr_offset;
> +        int i;
> +
> +        unsigned char *key_id_data = NULL;
> +        void *hash_data = NULL;
> +        bool is_valid = false;
> +
> +        vcb = g_new0(VerficationCertificateBlock, 1);
> +        if (s390_cpu_virt_mem_read(cpu, addr, (uint8_t)r1, vcb, sizeof(*vcb))) {
> +            s390_cpu_virt_mem_handle_exc(cpu, ra);
> +            return;
> +        }
> +
> +        if (vcb->vcb_hdr.vcbinlen % 4096 != 0) {

I think you want to use TARGET_PAGE_MASK here instead of the explicit 4096.

> +            rc = DIAG_320_RC_INVAL_VCB_LEN;
> +            g_free(vcb);
> +            break;
> +        }
> +
> +        if (1 > vcb->vcb_hdr.fvci || vcb->vcb_hdr.fvci > vcb->vcb_hdr.lvci) {

This is no longer entirely correct for vc index 0.  If first and last
indexes are both 0, no certificates are returned and rc should be OK.
We may not use this feature in this patch series, but we need to be
compliant.

Additionally, it's valid to have first == 0 and last > first.  In this
case, index 0 is skipped since the DIAG 320 cert store uses a one-origin
index.

> +            rc = DIAG_320_RC_BAD_RANGE;
> +            g_free(vcb);
> +            break;
> +        }
> +
> +        vcb->vcb_hdr.vcboutlen = VCB_HEADER_LEN;
> +        vcb->vcb_hdr.version = 0;
> +        vcb->vcb_hdr.svcc = 0;
> +        vcb->vcb_hdr.rvcc = 0;
> +

g_new0 initalizes all fields to 0, so no need to do that here.

> +        remaining_space = vcb->vcb_hdr.vcbinlen - VCB_HEADER_LEN;
> +
> +        for (i = vcb->vcb_hdr.fvci - 1; i < vcb->vcb_hdr.lvci; i++) {
> +            VerificationCertificateEntry vce;> +
S390IPLCertificate qcert;
> +
> +            /*
> +             * If cert index goes beyond the highest cert
> +             * store index (count - 1), then exit early
> +             */
> +            if (i >= qcs->count) {
> +                break;
> +            }

May make sense to just toss this in with the loop condition and then
initialize qcert = qcs->certs[i]; above.

> +
> +            qcert = qcs->certs[i];
> +
> +            /*
> +             * If there is no more space to store the cert,
> +             * set the remaining verification cert count and
> +             * break early.
> +             */
> +            if (remaining_space < qcert.size) {
> +                vcb->vcb_hdr.rvcc = vcb->vcb_hdr.lvci - i;
> +                break;
> +            }
> +
> +            /* Construct VCE */
> +            vce.vce_hdr.len = VCE_HEADER_LEN;
> +            vce.vce_hdr.certidx = i + 1;
> +            vce.vce_hdr.certlen = qcert.size;
> +
> +            strncpy((char *)vce.vce_hdr.name, (char *)qcert.vc_name, VC_NAME_LEN_BYTES);
> +
> +            rc = diag_320_get_cert_info(&vce, qcert, &is_valid, &key_id_data, &hash_data);
> +            if (rc) {
> +                continue;
> +            }
> +
> +            vce.vce_hdr.len += vce.vce_hdr.keyidlen;
> +            vce.vce_hdr.len += vce.vce_hdr.hashlen;
> +            vce.vce_hdr.len += vce.vce_hdr.certlen;
> +
> +            vce.vce_hdr.hashoffset = VCE_HEADER_LEN + vce.vce_hdr.keyidlen;
> +            vce.vce_hdr.certoffset = VCE_HEADER_LEN + vce.vce_hdr.keyidlen +
> +                                     vce.vce_hdr.hashlen;
> +
> +            vce_hdr_offset = vce_offset;
> +            vce_offset += VCE_HEADER_LEN;
> +
> +            /* Write Key ID */
> +            if (s390_cpu_virt_mem_write(cpu, addr + vce_offset, (uint8_t)r1, key_id_data,
> +                                        be16_to_cpu(vce.vce_hdr.keyidlen))) {
> +                s390_cpu_virt_mem_handle_exc(cpu, ra);
> +                return;
> +            }
> +            vce_offset += vce.vce_hdr.keyidlen;
> +
> +            /* Write Hash key */
> +            if (s390_cpu_virt_mem_write(cpu, addr + vce_offset, (uint8_t)r1, hash_data,
> +                                        be16_to_cpu(vce.vce_hdr.hashlen))) {
> +                s390_cpu_virt_mem_handle_exc(cpu, ra);
> +                return;
> +            }
> +             vce_offset += vce.vce_hdr.hashlen;

Leading space.

> +
> +            /* Write VCE cert data */
> +            if (s390_cpu_virt_mem_write(cpu, addr + vce_offset, (uint8_t)r1, qcert.raw,
> +                                        be32_to_cpu(vce.vce_hdr.certlen))) {
> +                s390_cpu_virt_mem_handle_exc(cpu, ra);
> +                return;
> +            }
> +            vce_offset += qcert.size;
> +
> +            /* The certificate is valid and VCE contains the certificate */
> +            if (is_valid) {
> +                vce.vce_hdr.flags |= DIAG_320_VCE_FLAGS_VALID;
> +            }
> +
> +            /* Write VCE Header */
> +            if (s390_cpu_virt_mem_write(cpu, addr + vce_hdr_offset, (uint8_t)r1, &vce,
> +                                        be32_to_cpu(VCE_HEADER_LEN))) {
> +                s390_cpu_virt_mem_handle_exc(cpu, ra);
> +                return;
> +            }
> +
> +            vcb->vcb_hdr.vcboutlen += vce.vce_hdr.len;
> +            remaining_space -= vce.vce_hdr.len;
> +            vcb->vcb_hdr.svcc++;
> +
> +            g_free(key_id_data);
> +            g_free(hash_data);
> +        }
> +
> +        /* Finally, write the header */

/* Write VCB Header */

> +        if (s390_cpu_virt_mem_write(cpu, addr, (uint8_t)r1, vcb,
> +                                    be32_to_cpu(VCB_HEADER_LEN))) {
> +            s390_cpu_virt_mem_handle_exc(cpu, ra);
> +            return;
> +        }> +        rc = DIAG_320_RC_OK;
> +        g_free(vcb);
> +        break;
>      default:
>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>          return;


-- 
Regards,
  Collin