On 5/24/26 6:32 PM, Jared Rossi wrote:
>
>
> On 5/5/26 4:18 PM, Zhuoying Cai wrote:
>> [...]
>
>> +}
>> +
>> +static int get_key_type(const S390IPLCertificate *cert)
>> +{
>> + int rc;
>> + Error *err = NULL;
>> +
>> + 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;
>> +}
>> +
>> +static int build_vce_header(VCEntry *vce, const S390IPLCertificate *cert,
>> int idx)
>> +{
>> + int key_type;
>> +
>> + vce->vce_hdr.len = cpu_to_be32(sizeof(VCEntryHeader));
>> + vce->vce_hdr.cert_idx = cpu_to_be16(idx + 1);
>> + memcpy(vce->vce_hdr.name, cert->name, CERT_NAME_MAX_LEN);
>> +
>> + key_type = get_key_type(cert);
>> + if (key_type == -1) {
>> + return -1;
>> + }
>> + vce->vce_hdr.key_type = key_type;
>> +
>> + return 0;
>> +}
>> +
>> +static int build_vce_data(VCEntry *vce, const S390IPLCertificate *cert)
>> +{
>> + uint16_t keyid_field_len;
>> + uint16_t hash_field_len;
>> + uint32_t cert_field_len;
>> + uint32_t vce_len;
>> + int rc;
>> +
>> + rc = handle_key_id(vce, cert);
>> + if (rc) {
>> + return -1;
>> + }
>> + keyid_field_len = ROUND_UP(be16_to_cpu(vce->vce_hdr.keyid_len), 4);
> Maybe return the converted length from the handle_* functions, with 0
> being the
> error condition?
>
> e.g. (untested)
> keyid_len = handle_key_id(vce, cert);
> if (!keyid_len) {
> return -1;
> }
>
> Just a nit, but I think it would improve readability.
>
>> +
>> + rc = handle_hash(vce, cert, keyid_field_len);
>> + if (rc) {
>> + return -1;
>> + }
>> + hash_field_len = ROUND_UP(be16_to_cpu(vce->vce_hdr.hash_len), 4);
>> +
>> + rc = handle_cert(vce, cert, hash_field_len);
>> + if (rc || !is_cert_valid(cert)) {
>> + return -1;
>> + }
>> + cert_field_len = ROUND_UP(be32_to_cpu(vce->vce_hdr.cert_len), 4);
>> +
>> + vce_len = sizeof(VCEntryHeader) + keyid_field_len + hash_field_len +
>> cert_field_len;
>> + if (vce_len > be32_to_cpu(vce->vce_hdr.len)) {
>> + return -1;
>> + }
> We already used the maximum size when we allocated space for the VCE, so
> this
> check is redundant I think?
>
>> +
>> + vce->vce_hdr.flags |= DIAG_320_VCE_FLAGS_VALID;
>> +
>> + /* Update vce length to reflect the actual size used by vce */
>> + vce->vce_hdr.len = cpu_to_be32(vce_len);
>> +
>> + return 0;
>> +}
>> +
>> +static VCEntry *diag_320_build_vce(const S390IPLCertificate *cert, int idx)
>> +{
>> + g_autofree VCEntry *vce = NULL;
>> + uint32_t vce_max_size;
>> + int rc;
>> +
>> + /*
>> + * Each field of the VCE is word-aligned.
>> + * Allocate enough space for the largest possible size for this VCE.
>> + * As the certificate fields (key-id, hash, data) are parsed, the
>> + * VCE's length field will be updated accordingly.
>> + */
>> + vce_max_size = sizeof(VCEntryHeader) +
>> + ROUND_UP(CERT_KEY_ID_LEN, 4) +
>> + ROUND_UP(CERT_HASH_LEN, 4) +
>> + ROUND_UP(cert->der_size, 4);
> In Patch 10, we define MAX_ENTRY_SIZE. Is that something different than
> this?
> Let's use the constant here unless there is a specific reason not to.
>
>> +
>> + vce = g_malloc0(vce_max_size);
>> + rc = build_vce_header(vce, cert, idx);
>> + if (rc) {
>> + /*
>> + * Error occurs - VCE does not contain a valid certificate.
>> + * Bit 0 of the VCE flags is 0 and the VCE length is set.
>> + */
>> + vce->vce_hdr.len = cpu_to_be32(VCE_INVALID_LEN);
>> + goto out;
>> + }
>> +
>> + vce->vce_hdr.len = cpu_to_be32(vce_max_size);
> It is not obvious later in the code if the VCE length is a true value or a
> place holder for the max size, so MAX_ENTRY_SIZE would help clarify also.
>
>> + rc = build_vce_data(vce, cert);
>> + if (rc) {
>> + vce->vce_hdr.len = cpu_to_be32(VCE_INVALID_LEN);
>> + }
>> +
>> +out:
>> + return g_steal_pointer(&vce);
>> +}
> Actually, I wonder if we might be able to remove the diag_320_build_vce()
> function since it is basically just a wrapper for building the header
> and data.
>
> If we introduce the MAX_ENTRY_SIZE constant and allocate memory before
> calling
> the build functions, I think we can simplify the whole thing.
>
> See notes below in the loop to build VCEs...
>
>> +
>> +static int handle_diag320_store_vc(S390CPU *cpu, uint64_t addr, uint64_t
>> r1, uintptr_t ra,
>> + S390IPLCertificateStore *cs)
>> +{
>> + g_autofree VCBlock *vcb = NULL;
>> + size_t remaining_space;
>> + uint16_t first_vc_index;
>> + uint16_t last_vc_index;
>> + int cs_start_index;
>> + int cs_end_index;
>> + uint32_t in_len;
>> +
>> + vcb = g_new0(VCBlock, 1);
>> + if (s390_cpu_virt_mem_read(cpu, addr, r1, vcb, sizeof(*vcb))) {
>> + s390_cpu_virt_mem_handle_exc(cpu, ra);
>> + return -1;
>> + }
> This is only the VCB header, right? That's all the space is allocated for
> anyway. I find it confusing, here is my understanding, please correct
> if wrong:
>
> We are allocating a VCB and updating the header values, but we never
> populate
> the VCB's buffer area because each VCE being built has it's own malloc call?
>
> Meaning when we read/write the VCB it is actually just the header, and
> the VCEs
> are written independently?
>
> If that is correct, then we never really build a full VCB right? Could
> we just
> use the VCBHeader struct here instead to make that clear?
>
>> +
>> + in_len = be32_to_cpu(vcb->vcb_hdr.in_len);
>> + first_vc_index = be16_to_cpu(vcb->vcb_hdr.first_vc_index);
>> + last_vc_index = be16_to_cpu(vcb->vcb_hdr.last_vc_index);
>> +
>> + if (in_len % TARGET_PAGE_SIZE != 0) {
>> + return DIAG_320_RC_INVAL_VCB_LEN;
>> + }
>> +
>> + if (first_vc_index > last_vc_index) {
>> + return DIAG_320_RC_BAD_RANGE;
>> + }
>> +
>> + vcb->vcb_hdr.out_len = sizeof(VCBlockHeader);
>> +
>> + /*
>> + * DIAG 320 subcode 2 expects to query a certificate store that
>> + * maintains an index origin of 1. However, the S390IPLCertificateStore
>> + * maintains an index origin of 0. Thus, the indices must be adjusted
>> + * for correct access into the cert store. A couple of special cases
>> + * must also be accounted for.
>> + */
>> +
>> + /* Both indices are 0; return header with no certs */
>> + if (first_vc_index == 0 && last_vc_index == 0) {
>> + goto out;
>> + }
>> +
>> + /* Normalize indices */
>> + cs_start_index = (first_vc_index == 0) ? 0 : first_vc_index - 1;
>> + cs_end_index = last_vc_index - 1;
>> +
>> + /* Requested range is outside the cert store; return header with no
>> certs */
>> + if (cs_start_index >= cs->count || cs_end_index >= cs->count) {
>> + goto out;
>> + }
>> +
>> + remaining_space = in_len - sizeof(VCBlockHeader);
>> +
>> + for (int i = cs_start_index; i <= cs_end_index; i++) {
>> + VCEntry *vce;
> What about g_autofree VCEntry *vce = g_malloc0(MAX_ENTRY_SIZE);
>
vce_max_size = sizeof(VCEntryHeader) +
ROUND_UP(CERT_KEY_ID_LEN, 4) +
ROUND_UP(CERT_HASH_LEN, 4) +
ROUND_UP(cert->der_size, 4);
After thinking about this, I’m not entirely sure, but I think using the
calculated vce_max_size might be a better choice than MAX_ENTRY_SIZE,
since vce_max_size more closely reflects the size of the current VCE.
MAX_ENTRY_SIZE is currently defined as 8192, which is significantly
larger than the size of a single VCB that will be requested in a later
patch. Although we check whether the remaining space is sufficient to
store the VCE, allocating MAX_ENTRY_SIZE for each VCE might be unnecessary?
Just wondering if this makes sense, or if you think we should keep using
MAX_ENTRY_SIZE here?
> Collin already mentioned using g_autofree, but in addition to that if we can
> allocate the memory up front that would allow us to call the build
> functions directly from here e.g. (untested)
>
> if (build_vce_header(vce, cert, i) || build_vce_data(vce, cert) {
> vce->vce_hdr.len = cpu_to_be32(VCE_INVALID_LEN);
> }
>
> That would avoid the frees and steal pointer too.
>
>> + const S390IPLCertificate *cert = &cs->certs[i];
>> +
>> + /*
>> + * Bit 0 of the VCE flags indicates whether the certificate is
>> valid.
>> + * The caller of DIAG320 subcode 2 is responsible for verifying that
>> + * the VCE contains a valid certificate.
>> + */
>> + vce = diag_320_build_vce(cert, i);
>> +
>> + /*
>> + * If there is no more space to store the cert,
>> + * set the remaining verification cert count and
>> + * break early.
>> + */
>> + if (remaining_space < vce->vce_hdr.len) {
>> + vcb->vcb_hdr.remain_ct = cpu_to_be16(last_vc_index - i);
>> + g_free(vce);
>> + break;
>> + }
>> +
>> + /* Write VCE */
>> + if (s390_cpu_virt_mem_write(cpu, addr + vcb->vcb_hdr.out_len, r1,
>> + vce, vce->vce_hdr.len)) {
>> + s390_cpu_virt_mem_handle_exc(cpu, ra);
>> + g_free(vce);
>> + return -1;
>> + }
>> +
>> + vcb->vcb_hdr.out_len += vce->vce_hdr.len;
>> + remaining_space -= vce->vce_hdr.len;
>> + vcb->vcb_hdr.stored_ct++;
>> +
>> + g_free(vce);
>> + }
>> + vcb->vcb_hdr.stored_ct = cpu_to_be16(vcb->vcb_hdr.stored_ct);
>> +
>> +out:
>> + vcb->vcb_hdr.out_len = cpu_to_be32(vcb->vcb_hdr.out_len);
>> +
>> + if (s390_cpu_virt_mem_write(cpu, addr, r1, vcb, sizeof(VCBlockHeader)))
>> {
>> + s390_cpu_virt_mem_handle_exc(cpu, ra);
>> + return -1;
>> + }
>> +
>> + return DIAG_320_RC_OK;
>> +}
>> +
>> QEMU_BUILD_BUG_MSG(sizeof(VCStorageSizeBlock) != VCSSB_MIN_LEN,
>> "size of VCStorageSizeBlock is wrong");
>> +QEMU_BUILD_BUG_MSG(sizeof(VCBlock) != 64, "size of VCBlock is wrong");
>> +QEMU_BUILD_BUG_MSG(sizeof(VCEntry) != 128, "size of VCEntry is wrong");
>>
>> void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3,
>> uintptr_t ra)
>> {
>> @@ -270,7 +594,8 @@ void handle_diag_320(CPUS390XState *env, uint64_t r1,
>> uint64_t r3, uintptr_t ra)
>> * for now.
>> */
>> uint32_t ism_word0 = cpu_to_be32(DIAG_320_ISM_QUERY_SUBCODES |
>> - DIAG_320_ISM_QUERY_VCSI);
>> + DIAG_320_ISM_QUERY_VCSI |
>> + DIAG_320_ISM_STORE_VC);
>>
>> if (s390_cpu_virt_mem_write(cpu, addr, r1, &ism_word0,
>> sizeof(ism_word0))) {
>> s390_cpu_virt_mem_handle_exc(cpu, ra);
>> @@ -296,6 +621,18 @@ void handle_diag_320(CPUS390XState *env, uint64_t r1,
>> uint64_t r3, uintptr_t ra)
>> }
>> env->regs[r1 + 1] = rc;
>> break;
>> + case DIAG_320_SUBC_STORE_VC:
>> + if (addr & ~TARGET_PAGE_MASK) {
>> + s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> + return;
>> + }
>> +
>> + rc = handle_diag320_store_vc(cpu, addr, r1, ra, cs);
>> + if (rc == -1) {
>> + return;
>> + }
>> + env->regs[r1 + 1] = rc;
>> + break;
>> default:
>> env->regs[r1 + 1] = DIAG_320_RC_NOT_SUPPORTED;
>> break;
> Regards,
> Jared Rossi
