Thanks for the feedback!
On 5/24/26 6:32 PM, Jared Rossi wrote:
>
>
> On 5/5/26 4:18 PM, Zhuoying Cai wrote:
>> [...]
>> +
>> + 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?
>
Although exceeding the maximum size is unlikely, I think it’s good
practice to keep this check, along with those in the handle_* functions.
These checks help ensure that the actual lengths of the certificate
metadata retrieved from the qcrypto functions remain within the defined
limits.
Please let me know what you 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?
>
Yes, you're correct. We only build the VCB header here, and each VCE is
allocated separately and appended after the header one by one.
I’ll update it to use the VCBHeader struct here to make that clearer.
>> +
>> + 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);
>
> 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
