Re: [PATCH v1 2/8] s390/sclp: check sccb len before filling in data

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

On 5/13/20 4:16 AM, Cornelia Huck wrote:
> On Wed, 13 May 2020 09:43:37 +0200
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 5/12/20 6:01 PM, Cornelia Huck wrote:
>>> On Mon, 11 May 2020 17:02:06 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> On 11.05.20 16:50, Janosch Frank wrote:  
>>>>> On 5/11/20 4:44 PM, David Hildenbrand wrote:    
>>>>>> On 11.05.20 16:36, Janosch Frank wrote:    
>>>>>>> On 5/9/20 1:08 AM, Collin Walling wrote:    
>>>>>>>> The SCCB must be checked for a sufficient length before it is filled
>>>>>>>> with any data. If the length is insufficient, then the SCLP command
>>>>>>>> is suppressed and the proper response code is set in the SCCB header.
>>>>>>>>
>>>>>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>    
>>>>>>>
>>>>>>> Fixes tag?  
>>>
>>> Probably
>>>
>>> Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length")
>>>
>>> ?
>>>   
>>>>>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>    
>>>>>>
>>>>>> This is not a fix AFAIKs.
>>>>>> sclp_service_call()/sclp_service_call_protected() always supplies a full
>>>>>> SCCB of exactly 4k size.
>>>>>>    
>>>>>
>>>>> We don't check for QEMU's 4k buffer here, but for the length that was
>>>>> specified by the guest.
>>>>>
>>>>> It's valid for the guest to request cpu info and state that its buffer
>>>>> is only 1k. We can't write everything in 1k if we have ~200 cpus, so
>>>>> we'll report the insufficient length rc.
>>>>>
>>>>> What he fixes here is the time of the length check, it should be done
>>>>> before any changes are being done to the work_sccb.    
>>>>
>>>> I don't have access to the spec, especially, if the guest can expect
>>>> nothing else in the sccb to change in case we report an error code. So
>>>> whatever you tell me, I have to trust you :)  
>>>
>>> Same here. Sounds plausible, but I have to trust the folks with the
>>> documentation :)
>>>   
>>
>> The AR states that:
>> * Command validity check (has prio over length, as length is dependent
>> on command)
>> * boundary (if extended-length is not available)
>> * Sufficient length check
>>
>> are done before "any other command action is taken".
>> If a test fails the command is suppressed.
> 
> Thanks, makes sense.
> 

Thanks, Janosch! (I suppose I could've said the same as well. Sorry
about that).

-- 
--
Regards,
Collin

Stay safe and stay healthy