Re: [PATCH v5 3/8] s390/sclp: read sccb from mem based on provided length

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

On 9/10/20 1:50 PM, Thomas Huth wrote:
> On 10/09/2020 11.36, Collin Walling wrote:
>> The header contained within the SCCB passed to the SCLP service call
>> contains the actual length of the SCCB. Instead of allocating a static
>> 4K size for the work sccb, let's allow for a variable size determined
>> by the value 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>
>> ---
>>  hw/s390x/event-facility.c |  2 +-
>>  hw/s390x/sclp.c           | 58 ++++++++++++++++++++++-----------------
>>  include/hw/s390x/sclp.h   |  2 +-
>>  3 files changed, 35 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
>> index 645b4080c5..ed92ce510d 100644
>> --- a/hw/s390x/event-facility.c
>> +++ b/hw/s390x/event-facility.c
>> @@ -213,7 +213,7 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
>>  
>>      event_buf = &red->ebh;
>>      event_buf->length = 0;
>> -    slen = sizeof(sccb->data);
>> +    slen = sccb_data_len(sccb);
>>  
>>      rc = SCLP_RC_NO_EVENT_BUFFERS_STORED;
>>  
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 69a8724dc7..cb8e2e8ec3 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -231,25 +231,30 @@ 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);
>> +    SCCBHeader header;
>> +    SCCB *work_sccb;
> 
> I'd maybe use "g_autofree SCCB *work_sccb = NULL" so you don't have to
> worry about doing the g_free() later.

Can do.

> 
>> -    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader));
>> +
>> +    work_sccb = g_malloc0(header.length);
> 
> Please use be16_to_cpu(header.length) here as well.

Good catch, thanks!

> 
>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, work_sccb,
>> +                         be16_to_cpu(header.length));
>>  
>>      if (!sclp_command_code_valid(code)) {
>> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>> +        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>>          goto out_write;
>>      }
>>  
>> -    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
>> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>> +    if (!sccb_verify_boundary(sccb, work_sccb->h.length)) {
>> +        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>>          goto out_write;
>>      }
>>  
>> -    sclp_c->execute(sclp, &work_sccb, code);
>> +    sclp_c->execute(sclp, work_sccb, code);
>>  out_write:
>> -    s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
>> -                          be16_to_cpu(work_sccb.h.length));
>> +    s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb,
>> +                          be16_to_cpu(work_sccb->h.length));
>> +    g_free(work_sccb);
>>      sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
>>      return 0;
>>  }
>> @@ -258,9 +263,8 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>  {
>>      SCLPDevice *sclp = get_sclp_device();
>>      SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>> -    SCCB work_sccb;
>> -
>> -    hwaddr sccb_len = sizeof(SCCB);
>> +    SCCBHeader header;
>> +    SCCB *work_sccb;
> 
> Maybe g_autofree again?
> 
>>      /* first some basic checks on program checks */
>>      if (env->psw.mask & PSW_MASK_PSTATE) {
>> @@ -274,33 +278,37 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>          return -PGM_SPECIFICATION;
>>      }
>>  
>> +    /* the header contains the actual length of the sccb */
>> +    cpu_physical_memory_read(sccb, &header, sizeof(SCCBHeader));
>> +
>> +    /* Valid sccb sizes */
>> +    if (be16_to_cpu(header.length) < sizeof(SCCBHeader)) {
>> +        return -PGM_SPECIFICATION;
>> +    }
>> +
>>      /*
>>       * we want to work on a private copy of the sccb, to prevent guests
>>       * 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);
>> -
>> -    /* Valid sccb sizes */
>> -    if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
>> -        return -PGM_SPECIFICATION;
>> -    }
>> +    work_sccb = g_malloc0(header.length);
> 
> Needs be16_to_cpu again.
> 
>> +    cpu_physical_memory_read(sccb, work_sccb, be16_to_cpu(header.length));
>>  
>>      if (!sclp_command_code_valid(code)) {
>> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>> +        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>>          goto out_write;
>>      }
>>  
>> -    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
>> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>> +    if (!sccb_verify_boundary(sccb, work_sccb->h.length)) {
>> +        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>>          goto out_write;
>>      }
>>  
>> -    sclp_c->execute(sclp, &work_sccb, code);
>> +    sclp_c->execute(sclp, work_sccb, code);
>>  out_write:
>> -    cpu_physical_memory_write(sccb, &work_sccb,
>> -                              be16_to_cpu(work_sccb.h.length));
>> -
>> +    cpu_physical_memory_write(sccb, work_sccb,
>> +                              be16_to_cpu(work_sccb->h.length));
>> +    g_free(work_sccb);
>>      sclp_c->service_interrupt(sclp, sccb);
>>  
>>      return 0;
> 
>  Thomas
> 
> 

Thanks, Thomas!

-- 
Regards,
Collin

Stay safe and stay healthy