Re: [PATCH v6 12/28] s390x/diag: Implement DIAG 508 subcode 1 for signature verification

[Thomas Huth <thuth@redhat.com>]

On 10/10/2025 18.37, Zhuoying Cai wrote:
> On 10/7/25 6:27 AM, Thomas Huth wrote:
>> On 18/09/2025 01.21, Zhuoying Cai wrote:
>>> From: Collin Walling <walling@linux.ibm.com>
>>>
>>> DIAG 508 subcode 1 performs signature-verification on signed components.
>>> A signed component may be a Linux kernel image, or any other signed
>>> binary. **Verification of initrd is not supported.**
>>>
>>> The instruction call expects two item-pairs: an address of a device
>>> component, an address of the analogous signature file (in PKCS#7 DER format),
>>> and their respective lengths. All of this data should be encapsulated
>>> within a Diag508SigVerifBlock.
>>>
>>> The DIAG handler will read from the provided addresses
>>> to retrieve the necessary data, parse the signature file, then
>>> perform the signature-verification. Because there is no way to
>>> correlate a specific certificate to a component, each certificate
>>> in the store is tried until either verification succeeds, or all
>>> certs have been exhausted.
>>>
>>> The subcode value is denoted by setting the second-to-left-most bit of
>>> a 2-byte field.
>>>
>>> A return code of 1 indicates success, and the index and length of the
>>> corresponding certificate will be set in the Diag508SigVerifBlock.
>>> The following values indicate failure:
>>>
>>> 	0x0102: certificate not available
>>> 	0x0202: component data is invalid
>>> 	0x0302: signature is not in PKCS#7 format
>>> 	0x0402: signature-verification failed
>>> 	0x0502: length of Diag508SigVerifBlock is invalid
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> 
> [...]
> 
>>> +
>>> +static int handle_diag508_sig_verif(uint64_t addr, size_t svb_size,
>>> +                                    S390IPLCertificateStore *qcs)
>>> +{
>>> +    int rc;
>>> +    int verified;
>>> +    uint32_t svb_len;
>>> +    uint64_t comp_len, comp_addr;
>>> +    uint64_t sig_len, sig_addr;
>>> +    g_autofree uint8_t *svb_comp = NULL;
>>> +    g_autofree uint8_t *svb_sig = NULL;
>>> +    g_autofree Diag508SigVerifBlock *svb = NULL;
>>> +
>>> +    if (!qcs || !qcs->count) {
>>> +        return DIAG_508_RC_NO_CERTS;
>>> +    }
>>> +
>>> +    svb = g_new0(Diag508SigVerifBlock, 1);
>>> +    cpu_physical_memory_read(addr, svb, svb_size);
>>> +
>>> +    svb_len = be32_to_cpu(svb->length);
>>> +    if (svb_len != svb_size) {
>>> +        return DIAG_508_RC_INVAL_LEN;
>>> +    }
>>> +
>>> +    comp_len = be64_to_cpu(svb->comp_len);
>>> +    comp_addr = be64_to_cpu(svb->comp_addr);
>>> +    sig_len = be64_to_cpu(svb->sig_len);
>>> +    sig_addr = be64_to_cpu(svb->sig_addr);
>>> +
>>> +    if (!comp_len || !comp_addr) {
>>> +        return DIAG_508_RC_INVAL_COMP_DATA;
>>> +    }
>>> +
>>> +    if (!sig_len || !sig_addr) {
>>> +        return DIAG_508_RC_INVAL_PKCS7_SIG;
>>> +    }
>>
>> I think there should also be something like an upper limit for comp_len and
>> sign_len here. Otherwise a malicious guest could force QEMU into allocating
>> giga- or terabytes of memory here to cause out-of-memory situations in the host.
>>
> 
> Thank you for the suggestion. I agree that setting an upper limit would
> help prevent unreasonable memory requests. I think it makes sense to
> choose a reasonable value so we don't have to adjust it too often, but
> I'm not entirely sure how to determine an appropriate upper bound.
> 
> Re: sig_len - the signature length can vary depending on the
> cryptographic algorithm, and I don't believe there's a strict limit.
> (FYI, in a somewhat similar situation, we haven't enforced a maximum
> size on certificate files when loading them into memory, since they're
> assumed to be trusted, as Daniel previously suggested -
> https://lists.gnu.org/archive/html/qemu-s390x/2025-06/msg00049.html).
> 
> If we'd like to set an upper limit for sig_len, the largest signature
> I've tested is 1165 bytes, signed with an RSA certificate using an
> 8192-bit key. Would 4096 be a reasonable upper bound?

Sounds reasonable, yes.

> Re: comp_len - the size of the guest kernel I'm currently using is
> 14,184,448 (0xD87000). When I built a kernel with make allyesconfig, the
> size can reach 261,005,383 (0xF8EA047). Based on this value, would
> 262,000,000 (0xF9DCD80) an appropriate upper limit?
Make it 0x10000000 = 268435456 Bytes, that feels like a round boundary ;-)

  Thomas