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

2020-07-20 Thread Collin Walling 
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 
>> Reviewed-by: Thomas Huth 
>> Reviewed-by: Janosch Frank 
>> Reviewed-by: Cornelia Huck 
>> ---
>>  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 
> 

Thanks!

-- 
Regards,
Collin

Stay safe and stay healthy

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

2020-07-20 Thread David Hildenbrand 
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 
> Reviewed-by: Thomas Huth 
> Reviewed-by: Janosch Frank 
> Reviewed-by: Cornelia Huck 
> ---
>  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 

-- 
Thanks,

David / dhildenb

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

2020-06-24 Thread Collin Walling 
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 
Reviewed-by: Thomas Huth 
Reviewed-by: Janosch Frank 
Reviewed-by: Cornelia Huck 
---
 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,
-- 
2.26.2