diff mbox series

[v5,2/8] s390/sclp: rework sclp boundary checks

Message ID 20200910093655.255774-3-walling@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390: Extended-Length SCCB & DIAGNOSE 0x318 | expand

Commit Message

Collin Walling Sept. 10, 2020, 9:36 a.m. UTC 
Rework the SCLP boundary check to account for different SCLP commands
(eventually) allowing different boundary sizes.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Acked-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/sclp.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Comments

Thomas Huth Sept. 10, 2020, 5:45 p.m. UTC | #1 
On 10/09/2020 11.36, Collin Walling wrote:
> Rework the SCLP boundary check to account for different SCLP commands
> (eventually) allowing different boundary sizes.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/sclp.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 28b973de8f..69a8724dc7 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -49,6 +49,18 @@ static inline bool sclp_command_code_valid(uint32_t code)
>      return false;
>  }
>  
> +static bool sccb_verify_boundary(uint64_t sccb_addr, uint16_t len)

Maybe it would be good to add a comment in front of the function to say
that len must be big endian?

 Thomas

> +{
> +    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(len) - 1;
> +    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
> +
> +    if (sccb_max_addr < sccb_boundary) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>  static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>  {
>      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> @@ -229,6 +241,11 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>          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);
> +        goto out_write;
> +    }
> +
>      sclp_c->execute(sclp, &work_sccb, code);
>  out_write:
>      s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
> @@ -274,7 +291,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>          goto out_write;
>      }
>  
> -    if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) + PAGE_SIZE)) {
> +    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;
>      }
>
Cornelia Huck Sept. 11, 2020, 10:24 a.m. UTC | #2 
On Thu, 10 Sep 2020 19:45:01 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 10/09/2020 11.36, Collin Walling wrote:
> > Rework the SCLP boundary check to account for different SCLP commands
> > (eventually) allowing different boundary sizes.
> > 
> > Signed-off-by: Collin Walling <walling@linux.ibm.com>
> > Acked-by: Janosch Frank <frankja@linux.ibm.com>
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/sclp.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> > index 28b973de8f..69a8724dc7 100644
> > --- a/hw/s390x/sclp.c
> > +++ b/hw/s390x/sclp.c
> > @@ -49,6 +49,18 @@ static inline bool sclp_command_code_valid(uint32_t code)
> >      return false;
> >  }
> >  
> > +static bool sccb_verify_boundary(uint64_t sccb_addr, uint16_t len)  
> 
> Maybe it would be good to add a comment in front of the function to say
> that len must be big endian?

What about renaming it to sccb_h_len or so? That would make it more
clear that the parameter is not just some random length.

> 
>  Thomas
> 
> > +{
> > +    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(len) - 1;
> > +    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
> > +
> > +    if (sccb_max_addr < sccb_boundary) {
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> >  static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
> >  {
> >      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> > @@ -229,6 +241,11 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
> >          goto out_write;
> >      }
> >  
> > +    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {

...name inspired by the 'h' in here.

> > +        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> > +        goto out_write;
> > +    }
> > +
> >      sclp_c->execute(sclp, &work_sccb, code);
> >  out_write:
> >      s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
> > @@ -274,7 +291,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
> >          goto out_write;
> >      }
> >  
> > -    if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) + PAGE_SIZE)) {
> > +    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;
> >      }
> >   
>
Collin Walling Sept. 11, 2020, 2:50 p.m. UTC | #3 
On 9/11/20 6:24 AM, Cornelia Huck wrote:
> On Thu, 10 Sep 2020 19:45:01 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 10/09/2020 11.36, Collin Walling wrote:
>>> Rework the SCLP boundary check to account for different SCLP commands
>>> (eventually) allowing different boundary sizes.
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>  hw/s390x/sclp.c | 19 ++++++++++++++++++-
>>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>>> index 28b973de8f..69a8724dc7 100644
>>> --- a/hw/s390x/sclp.c
>>> +++ b/hw/s390x/sclp.c
>>> @@ -49,6 +49,18 @@ static inline bool sclp_command_code_valid(uint32_t code)
>>>      return false;
>>>  }
>>>  
>>> +static bool sccb_verify_boundary(uint64_t sccb_addr, uint16_t len)  
>>
>> Maybe it would be good to add a comment in front of the function to say
>> that len must be big endian?
> 
> What about renaming it to sccb_h_len or so? That would make it more
> clear that the parameter is not just some random length.
> 

I think that makes sense.

>>
>>  Thomas
>>
>>> +{
>>> +    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(len) - 1;
>>> +    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
>>> +
>>> +    if (sccb_max_addr < sccb_boundary) {
>>> +        return true;
>>> +    }
>>> +
>>> +    return false;
>>> +}
>>> +
>>>  static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>>>  {
>>>      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
>>> @@ -229,6 +241,11 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>>>          goto out_write;
>>>      }
>>>  
>>> +    if (!sccb_verify_boundary(sccb, work_sccb.h.length)) {
> 
> ...name inspired by the 'h' in here.
> 
>>> +        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>>> +        goto out_write;
>>> +    }
>>> +
>>>      sclp_c->execute(sclp, &work_sccb, code);
>>>  out_write:
>>>      s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
>>> @@ -274,7 +291,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>>          goto out_write;
>>>      }
>>>  
>>> -    if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) + PAGE_SIZE)) {
>>> +    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;
>>>      }
>>>   
>>
> 
>
diff mbox series

Patch

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 28b973de8f..69a8724dc7 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -49,6 +49,18 @@  static inline bool sclp_command_code_valid(uint32_t code)
     return false;
 }
 
+static bool sccb_verify_boundary(uint64_t sccb_addr, uint16_t len)
+{
+    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(len) - 1;
+    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
+
+    if (sccb_max_addr < sccb_boundary) {
+        return true;
+    }
+
+    return false;
+}
+
 static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
 {
     uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
@@ -229,6 +241,11 @@  int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
         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);
+        goto out_write;
+    }
+
     sclp_c->execute(sclp, &work_sccb, code);
 out_write:
     s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
@@ -274,7 +291,7 @@  int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
         goto out_write;
     }
 
-    if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) + PAGE_SIZE)) {
+    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;
     }