Re: [PATCH v3 9/9] s390x/sclp: Reject short headers in PV case

[Eric Farman <farman@linux.ibm.com>]

On 9/25/26 3:58 AM, Janosch Frank wrote:
> On 9/24/26 4:39 PM, Christian Borntraeger wrote:
>>
>>
>> Am 24.09.26 um 16:36 schrieb Janosch Frank:
>>> On 9/22/26 10:15 PM, Eric Farman wrote:
>>>> An invalid length in an SCCB header correctly generates a specification
>>>> exception for the regular (non-PV) codepath that processes an SCLP
>>>> instruction intercept. The PV case doesn't, even though such a thing
>>>> should already be sanitized. Since a short length will cause problems
> 
> ...even though such a thing should already be handled by the UV.
> 
>>>> later down the line with the storage that will be allocated based on
>>>> that size, put up a defensive check and return if it is too short.
>>>>
>>>> Cc: Janosch Frank <frankja@linux.ibm.com>
>>>> Cc: qemu-stable@nongnu.org
>>>> Fixes: c1db53a591 ("s390/sclp: read sccb from mem based on provided 
>>>> length")
>>>> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>>> Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
>>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>>> ---
>>>
>>> How would you hit that if the architecture says FW will automatically 
>>> inject a spec exception without exiting to the hypervisor if length < 8?
>>
>> As I said in my review, current UV will sanitize this, so this is just 
>> defence in depth in case
>> there would be an implementation error.
> 
> That got lost in my mailbox overflow.
> 
> Alright, then let's extend the comment and commit description before 
> someone deletes this as dead code.

Works for me. I'll fold the above/below comments in when I apply.

> 
>>
>>>
>>> Did you witness this happening?
>>> Am I missing something?
>>>
>>>>    hw/s390x/sclp.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>>>> index d8b655efcc..ad8129a5d8 100644
>>>> --- a/hw/s390x/sclp.c
>>>> +++ b/hw/s390x/sclp.c
>>>> @@ -277,6 +277,11 @@ int sclp_service_call_protected(S390CPU *cpu, 
>>>> uint64_t sccb, uint32_t code)
>>>>        s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, 
>>>> sizeof(SCCBHeader));
>>>> +    /* Do nothing if the header appears malformed */
> 
> ... We should never end up here due to UV checks but let's be sure.
> 
>>>> +    if (be16_to_cpu(header.length) < sizeof(SCCBHeader)) {
>>>> +        goto out_no_write;
>>>> +    }
>>>> +
>>>>        work_sccb = g_malloc0(be16_to_cpu(header.length));
>>>>        s390_cpu_pv_mem_read(env_archcpu(env), 0, work_sccb,
>>>>                             be16_to_cpu(header.length));
>>>> @@ -290,6 +295,7 @@ int sclp_service_call_protected(S390CPU *cpu, 
>>>> uint64_t sccb, uint32_t code)
>>>>    out_write:
>>>>        s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb,
>>>>                              be16_to_cpu(header.length));
>>>> +out_no_write:
>>>>        sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
>>>>        return 0;
>>>>    }
>>>
>>
>