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

[David Hildenbrand]

On 09.05.20 01:08, 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 <address@hidden>
> ---
>  hw/s390x/sclp.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 470d5da7a2..748d04a0e2 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -244,15 +244,16 @@ 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);
>          goto out_write;
>      }
>  
> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, 
> work_sccb.h.length);

be16_to_cpu() necessary.

> +
>      if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) {
>          goto out_write;
>      }
> @@ -271,8 +272,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;
> @@ -290,13 +289,16 @@ 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)) {
>          return -PGM_SPECIFICATION;
>      }
>  
> +    /* the header contains the actual length of the sccb */
> +    cpu_physical_memory_read(sccb, &work_sccb, work_sccb.h.length);

be16_to_cpu() necessary.

> +
>      if (!sclp_command_code_valid(code)) {
>          work_sccb.h.response_code = 
> cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>          goto out_write;
> 

-- 
Thanks,

David / dhildenb