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

[Thomas Huth <thuth@redhat.com>]

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.

> -    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.

> +    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