On 6/8/26 10:16 AM, Jared Rossi wrote:
>
>
> On 6/5/26 3:20 PM, Zhuoying Cai wrote:
>> 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.
> After taking another look, I realize this check is not redundant in the
> way I
> thought it was. As far as I can tell, when building the cert store, the
> size
> of each entry is checked using the largest_cert_size variable, but we never
> validate that this value is within our max cert size. So the check here is
> actually the first time we validate the total size.
>
> In my opinion we should check this in the update_cert_store() function
> that was
> introduced in Patch 4 and gets called as the cert store is populated. I
> guess
> this ties back in to some of the other comments about defining
> MAX_ENTRY_SIZE
> earlier in the series.
>
> Maybe we could detect invalid certs earlier if we add a secondary check that
> data_buf_size <= CERT_BUF_MAX_LEN when updating largest_cert_size. I
> think both
> update_cert_store() and build_vce_data() are doing the same size
> calculations,
> right?
>
> I think we have all of this information available to reject certs before
> they
> are added to the cert store.
>
> Regards,
> Jared Rossi
Thanks for taking another look.
The current version I’m working on defines MAX_ENTRY_SIZE in the current
patch (s390x/diag: Implement DIAG 320 subcode 2), since that’s where it
is used. In the next patch (hw/s390x: Define maximum certificate buffer
length), I define CERT_BUF_MAX_LEN (MAX_ENTRY_SIZE -
sizeof(VCEntryHeader)) and add a data_buf_size <= CERT_BUF_MAX_LEN check
before updating largest_cert_size in update_cert_store().
update_cert_store() uses expected size values, whereas build_vce_data()
uses actual size values, but these should match. With the
CERT_BUF_MAX_LEN check in update_cert_store(), invalid certs would be
rejected before being added to the cert store.
I thought it might still be good practice to keep both checks to ensure
the size from build_vce_data() stays within bounds, but I may have
overthought that.
If you think the check in build_vce_data() is redundant once we add the
validation in update_cert_store(), I can remove it. Should I also remove
the checks in the handle_* functions as well?
