Re: [PATCH v4 09/28] s390x/diag: Implement DIAG 320 subcode 2

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

On 7/29/25 14:18, Zhuoying Cai wrote:
> Thank you for the feedback!
> 
> On 7/28/25 4:59 PM, Collin Walling wrote:
>> On 7/11/25 17:10, Zhuoying Cai wrote:
> 
> ...
> 

[...]

>>
>> 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.
> 

Sorry, I neglected to mentally trace some different loop variables to
see that these cases are indeed handled.  Thanks for clarifying!

[...]

-- 
Regards,
  Collin