Re: [PATCH v5 2/8] s390/sclp: rework sclp boundary checks
On 9/11/20 6:24 AM, Cornelia Huck wrote: > On Thu, 10 Sep 2020 19:45:01 +0200 > Thomas Huth 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 >>> Acked-by: Janosch Frank >>> Reviewed-by: Cornelia Huck >>> --- >>> 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, _sccb, code); >>> out_write: >>> s390_cpu_pv_mem_write(env_archcpu(env), 0, _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; >>> } >>> >> > > -- Regards, Collin Stay safe and stay healthy
Re: [PATCH v5 2/8] s390/sclp: rework sclp boundary checks
On Thu, 10 Sep 2020 19:45:01 +0200 Thomas Huth 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 > > Acked-by: Janosch Frank > > Reviewed-by: Cornelia Huck > > --- > > 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, _sccb, code); > > out_write: > > s390_cpu_pv_mem_write(env_archcpu(env), 0, _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; > > } > > >
Re: [PATCH v5 2/8] s390/sclp: rework sclp boundary checks
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 > Acked-by: Janosch Frank > Reviewed-by: Cornelia Huck > --- > 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, _sccb, code); > out_write: > s390_cpu_pv_mem_write(env_archcpu(env), 0, _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; > } >