Thank you for the feedback!
On 7/28/25 4:59 PM, Collin Walling wrote:
> On 7/11/25 17:10, Zhuoying Cai wrote:
...
>> +static int handle_diag320_store_vc(S390CPU *cpu, uint64_t addr, uint64_t
>> r1, uintptr_t ra,
>> + S390IPLCertificateStore *qcs)
>> +{
>> + g_autofree VCBlock *vcb = NULL;
>> + size_t vce_offset;
>> + size_t remaining_space;
>> + size_t keyid_buf_size;
>> + size_t hash_buf_size;
>> + size_t cert_buf_size;
>
> The above three fields can go into the respective helper functions that
> I mentioned earlier and reduce some of the bloat here.
>
>> + uint32_t vce_len;
>> + uint16_t first_vc_index;
>> + uint16_t last_vc_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;
>> + }
>> +
>> + in_len = be32_to_cpu(vcb->in_len);
>> + first_vc_index = be16_to_cpu(vcb->first_vc_index);
>> + last_vc_index = be16_to_cpu(vcb->last_vc_index);
>> +
>
> You need a check somewhere for no certs found in either the specified
> range or no certs exist in the store at all:
> - VCB output len = 64
> - stored and remaining count = 0
> - response code 0x0001
>
>> + 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;
>> + }
>> +
>> + if (first_vc_index == 0) {
>> + /*
>> + * Zero is a valid index for the first and last VC index.
>> + * Zero index results in the VCB header and zero certificates
>> returned.
>> + */
>> + if (last_vc_index == 0) {
>> + goto out;
>> + }
>> +
>> + /* DIAG320 certificate store remains a one origin for cert entries
>> */
>> + vcb->first_vc_index = 1;
>> + first_vc_index = 1;
>> + }
>> +
>> + vce_offset = VCB_HEADER_LEN;
>> + vcb->out_len = VCB_HEADER_LEN;
>> + remaining_space = in_len - VCB_HEADER_LEN;
>> +
Re: check for no certs found in either the specified range or no certs
exist.
This case is already handled.
vcb->out_len = VCB_HEADER_LEN is set outside the for loop. If the index
is invalid, the loop won’t execute, and both the stored and remaining VC
counts remain unchanged at 0.
>> + for (int i = first_vc_index - 1; i < last_vc_index && i < qcs->count;
>> i++) {
>> + VCEntry *vce;
>> + S390IPLCertificate qcert = qcs->certs[i];
>> + /*
>> + * Each VCE is word aligned.
>> + * Each variable length field within the VCE is also word aligned.
>> + */
>> + keyid_buf_size = ROUND_UP(qcert.key_id_size, 4);
>> + hash_buf_size = ROUND_UP(qcert.hash_size, 4);
>> + cert_buf_size = ROUND_UP(qcert.der_size, 4);
>> + vce_len = VCE_HEADER_LEN + cert_buf_size + keyid_buf_size +
>> hash_buf_size;
>
> You could define & set the above four lines inside build_vce (or as
> respective fields in the helper functions mentioned above).
>
> The remaining space check could be done after the vce has been built.
>
>> +
>> + /*
>> + * If there is no more space to store the cert,
>> + * set the remaining verification cert count and
>> + * break early.
>> + */
>> + if (remaining_space < vce_len) {
>> + vcb->remain_ct = cpu_to_be16(last_vc_index - i);
>> + break;
>> + }
>
> You also need to check somewhere if there is enough space to store *at
> least* the first cert in the range:
> - VCB output len = 64
> - stored count = 0
> - remaining count = // however are remaining
> - response code 0x0001
>
This case is also covered by the if statement above:
if (remaining_space < vce_len) {
vcb->remain_ct = cpu_to_be16(last_vc_index - i);
break;
}
Response code 0x0001 is returned at the end of the function for both cases.
>> +
>> + vce = build_vce(qcert, vce_len, i, keyid_buf_size, hash_buf_size);
>> + if (vce == NULL) {
>> + continue;
>> + }
>
> See above, there shouldn't be a NULL case.
>
>> +
>> + /* Write VCE */
>> + if (s390_cpu_virt_mem_write(cpu, addr + vce_offset, r1, vce,
>> vce_len)) {
>> + s390_cpu_virt_mem_handle_exc(cpu, ra);
>> + return -1;
>> + }> +
>> + vce_offset += vce_len;
>> + vcb->out_len += vce_len;
>> + remaining_space -= vce_len;
>> + vcb->stored_ct++;
>> +
>> + g_free(vce);
>> + }
>> +
>> + vcb->out_len = cpu_to_be32(vcb->out_len);
>> + vcb->stored_ct = cpu_to_be16(vcb->stored_ct);
>> +
>> +out:
>> + /*
>> + * Write VCB header
>> + * All VCEs have been populated with the latest information
>> + * and write VCB header last.
>> + */
>> + if (s390_cpu_virt_mem_write(cpu, addr, r1, vcb, VCB_HEADER_LEN)) {
>> + s390_cpu_virt_mem_handle_exc(cpu, ra);
>> + return -1;
>> + }
>> +
>> + return DIAG_320_RC_OK;
>> +}
...
