Re: [PATCH v4 4/8] s390/sclp: read sccb from mem based on sccb length

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

On 7/20/20 4:19 AM, David Hildenbrand wrote:
> On 24.06.20 22:23, Collin Walling wrote:
>> The header of the SCCB contains the actual length of the SCCB. Instead
>> of using a static 4K size, let's allow for a variable size determined
>> by the value set in the header. The proper checks are already in place
>> to ensure the SCCB length is sufficent to store a full response, and
>> that the length does not cross any explicitly-set boundaries.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  hw/s390x/sclp.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 5899c1e3b8..1feba6f692 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -251,9 +251,8 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>>      SCLPDevice *sclp = get_sclp_device();
>>      SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>>      SCCB work_sccb;
>> -    hwaddr sccb_len = sizeof(SCCB);
>>  
>> -    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sizeof(SCCBHeader));
>>  
>>      if (!sclp_command_code_valid(code)) {
>>          work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>> @@ -264,6 +263,9 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>>          goto out_write;
>>      }
>>  
>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb,
>> +                         be16_to_cpu(work_sccb.h.length));
>> +
>>      sclp_c->execute(sclp, &work_sccb, code);
>>  out_write:
>>      s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
>> @@ -278,8 +280,6 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>      SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>>      SCCB work_sccb;
>>  
>> -    hwaddr sccb_len = sizeof(SCCB);
>> -
>>      /* first some basic checks on program checks */
>>      if (env->psw.mask & PSW_MASK_PSTATE) {
>>          return -PGM_PRIVILEGED;
>> @@ -297,7 +297,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>       * from playing dirty tricks by modifying the memory content after
>>       * the host has checked the values
>>       */
>> -    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
>> +    cpu_physical_memory_read(sccb, &work_sccb, sizeof(SCCBHeader));
>>  
>>      /* Valid sccb sizes */
>>      if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
>> @@ -313,6 +313,9 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>          goto out_write;
>>      }
>>  
>> +    /* the header contains the actual length of the sccb */
>> +    cpu_physical_memory_read(sccb, &work_sccb, be16_to_cpu(work_sccb.h.length));
>> +
>>      sclp_c->execute(sclp, &work_sccb, code);
>>  out_write:
>>      cpu_physical_memory_write(sccb, &work_sccb,
>>
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 

Thanks!

-- 
Regards,
Collin

Stay safe and stay healthy