| Message ID | 20200508230823.22956-4-walling@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390: Extended-Length SCCB & DIAGNOSE 0x318 | expand |
On 09.05.20 01:08, Collin Walling wrote: > Let's factor out the SCLP boundary and length checks > into separate functions. > > Signed-off-by: Collin Walling <walling@linux.ibm.com> > --- > hw/s390x/sclp.c | 41 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 35 insertions(+), 6 deletions(-) > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index d08a291e40..470d5da7a2 100644 > --- a/hw/s390x/sclp.c > +++ b/hw/s390x/sclp.c > @@ -49,6 +49,34 @@ static inline bool sclp_command_code_valid(uint32_t code) > return false; > } > > +static bool check_sccb_boundary_valid(uint64_t sccb_addr, uint32_t code, > + SCCB *sccb) I suggest naming this "has_valid_sccb_boundary", then the true/false response is clearer. > +{ > + uint64_t current_len = sccb_addr + be16_to_cpu(sccb->h.length); > + uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE; > + > + switch (code & SCLP_CMD_CODE_MASK) { > + default: > + if (current_len <= allowed_len) { > + return true; > + } > + } > + sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); > + return false; > +} > + > +static bool check_sufficient_sccb_len(SCCB *sccb, int size) "has_sufficient_sccb_len" ? > +{ > + MachineState *ms = MACHINE(qdev_get_machine()); > + int required_len = size + ms->possible_cpus->len * sizeof(CPUEntry); Rather pass in the number of cpus instead. Looking up the machine again in here is ugly. > + > + if (be16_to_cpu(sccb->h.length) < required_len) { > + sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); > + return false; > + } > + return true; > +} > + > static void prepare_cpu_entries(CPUEntry *entry, int *count) > { > MachineState *ms = MACHINE(qdev_get_machine()); > @@ -76,8 +104,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > int rnsize, rnmax; > IplParameterBlock *ipib = s390_ipl_get_iplb(); > > - if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) { > - sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); > + if (!check_sufficient_sccb_len(sccb, sizeof(ReadInfo))) { > return; > } > > @@ -134,8 +161,7 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb) > ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb; > int cpu_count; > > - if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) { > - sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); > + if (!check_sufficient_sccb_len(sccb, sizeof(ReadCpuInfo))) { > return; > } > > @@ -227,6 +253,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, > goto out_write; > } > > + if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) { > + goto out_write; > + } This is not a "factor out". You're adding new code, this needs justification in the patch description. > + > sclp_c->execute(sclp, &work_sccb, code); > out_write: > s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, > @@ -272,8 +302,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)) { > - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); > + if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) { > goto out_write; > } > >
On 5/12/20 3:21 AM, David Hildenbrand wrote: > On 09.05.20 01:08, Collin Walling wrote: >> Let's factor out the SCLP boundary and length checks >> into separate functions. >> >> Signed-off-by: Collin Walling <walling@linux.ibm.com> >> --- >> hw/s390x/sclp.c | 41 +++++++++++++++++++++++++++++++++++------ >> 1 file changed, 35 insertions(+), 6 deletions(-) >> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index d08a291e40..470d5da7a2 100644 >> --- a/hw/s390x/sclp.c >> +++ b/hw/s390x/sclp.c >> @@ -49,6 +49,34 @@ static inline bool sclp_command_code_valid(uint32_t code) >> return false; >> } >> >> +static bool check_sccb_boundary_valid(uint64_t sccb_addr, uint32_t code, >> + SCCB *sccb) > > I suggest naming this > > "has_valid_sccb_boundary", then the true/false response is clearer. > >> +{ >> + uint64_t current_len = sccb_addr + be16_to_cpu(sccb->h.length); >> + uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE; >> + >> + switch (code & SCLP_CMD_CODE_MASK) { >> + default: >> + if (current_len <= allowed_len) { >> + return true; >> + } >> + } >> + sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); >> + return false; >> +} >> + >> +static bool check_sufficient_sccb_len(SCCB *sccb, int size) > > "has_sufficient_sccb_len" ? > >> +{ >> + MachineState *ms = MACHINE(qdev_get_machine()); >> + int required_len = size + ms->possible_cpus->len * sizeof(CPUEntry); > > Rather pass in the number of cpus instead. Looking up the machine again > in here is ugly. prepare_cpu_entries also looks up the machine again. Should I squeeze in a cleanup where we pass the machine to that function too (perhaps in the "remove SCLPDevice" patch)? > >> + >> + if (be16_to_cpu(sccb->h.length) < required_len) { >> + sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); >> + return false; >> + } >> + return true; >> +} >> + >> static void prepare_cpu_entries(CPUEntry *entry, int *count) >> { >> MachineState *ms = MACHINE(qdev_get_machine()); >> @@ -76,8 +104,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) >> int rnsize, rnmax; >> IplParameterBlock *ipib = s390_ipl_get_iplb(); >> >> - if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) { >> - sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); >> + if (!check_sufficient_sccb_len(sccb, sizeof(ReadInfo))) { >> return; >> } >> >> @@ -134,8 +161,7 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb) >> ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb; >> int cpu_count; >> >> - if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) { >> - sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); >> + if (!check_sufficient_sccb_len(sccb, sizeof(ReadCpuInfo))) { >> return; >> } >> >> @@ -227,6 +253,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, >> goto out_write; >> } >> >> + if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) { >> + goto out_write; >> + } > > This is not a "factor out". You're adding new code, this needs > justification in the patch description. True. I'll fix up the message. > >> + >> sclp_c->execute(sclp, &work_sccb, code); >> out_write: >> s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, >> @@ -272,8 +302,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)) { >> - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); >> + if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) { >> goto out_write; >> } >> >> > > Renamed functions. Thanks for the review!
On Tue, 12 May 2020 10:55:56 -0400 Collin Walling <walling@linux.ibm.com> wrote: > On 5/12/20 3:21 AM, David Hildenbrand wrote: > > On 09.05.20 01:08, Collin Walling wrote: > >> +static bool check_sufficient_sccb_len(SCCB *sccb, int size) > > > > "has_sufficient_sccb_len" ? > > > >> +{ > >> + MachineState *ms = MACHINE(qdev_get_machine()); > >> + int required_len = size + ms->possible_cpus->len * sizeof(CPUEntry); > > > > Rather pass in the number of cpus instead. Looking up the machine again > > in here is ugly. > > prepare_cpu_entries also looks up the machine again. Should I squeeze > in a cleanup where we pass the machine to that function too (perhaps > in the "remove SCLPDevice" patch)? sclp_read_cpu_info() does not have the machine handy, so you'd need to move machine lookup there; but I think it's worth getting rid of duplicate lookups. > > > > >> + > >> + if (be16_to_cpu(sccb->h.length) < required_len) { > >> + sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); > >> + return false; > >> + } > >> + return true; > >> +} > >> +
On 5/13/20 3:00 AM, Cornelia Huck wrote: > On Tue, 12 May 2020 10:55:56 -0400 > Collin Walling <walling@linux.ibm.com> wrote: > >> On 5/12/20 3:21 AM, David Hildenbrand wrote: >>> On 09.05.20 01:08, Collin Walling wrote: > >>>> +static bool check_sufficient_sccb_len(SCCB *sccb, int size) >>> >>> "has_sufficient_sccb_len" ? >>> >>>> +{ >>>> + MachineState *ms = MACHINE(qdev_get_machine()); >>>> + int required_len = size + ms->possible_cpus->len * sizeof(CPUEntry); >>> >>> Rather pass in the number of cpus instead. Looking up the machine again >>> in here is ugly. >> >> prepare_cpu_entries also looks up the machine again. Should I squeeze >> in a cleanup where we pass the machine to that function too (perhaps >> in the "remove SCLPDevice" patch)? > > sclp_read_cpu_info() does not have the machine handy, so you'd need to > move machine lookup there; but I think it's worth getting rid of > duplicate lookups. > Sounds good, then. I'll propose a change to patch 1 that removes the unused SCLPDevice param and accepts a MachineState instead. Should make things a bit cleaner :) >> >>> >>>> + >>>> + if (be16_to_cpu(sccb->h.length) < required_len) { >>>> + sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); >>>> + return false; >>>> + } >>>> + return true; >>>> +} >>>> + > >
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c index d08a291e40..470d5da7a2 100644 --- a/hw/s390x/sclp.c +++ b/hw/s390x/sclp.c @@ -49,6 +49,34 @@ static inline bool sclp_command_code_valid(uint32_t code) return false; } +static bool check_sccb_boundary_valid(uint64_t sccb_addr, uint32_t code, + SCCB *sccb) +{ + uint64_t current_len = sccb_addr + be16_to_cpu(sccb->h.length); + uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE; + + switch (code & SCLP_CMD_CODE_MASK) { + default: + if (current_len <= allowed_len) { + return true; + } + } + sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); + return false; +} + +static bool check_sufficient_sccb_len(SCCB *sccb, int size) +{ + MachineState *ms = MACHINE(qdev_get_machine()); + int required_len = size + ms->possible_cpus->len * sizeof(CPUEntry); + + if (be16_to_cpu(sccb->h.length) < required_len) { + sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); + return false; + } + return true; +} + static void prepare_cpu_entries(CPUEntry *entry, int *count) { MachineState *ms = MACHINE(qdev_get_machine()); @@ -76,8 +104,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) int rnsize, rnmax; IplParameterBlock *ipib = s390_ipl_get_iplb(); - if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) { - sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); + if (!check_sufficient_sccb_len(sccb, sizeof(ReadInfo))) { return; } @@ -134,8 +161,7 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb) ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb; int cpu_count; - if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) { - sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); + if (!check_sufficient_sccb_len(sccb, sizeof(ReadCpuInfo))) { return; } @@ -227,6 +253,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, goto out_write; } + if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) { + goto out_write; + } + sclp_c->execute(sclp, &work_sccb, code); out_write: s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, @@ -272,8 +302,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)) { - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); + if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) { goto out_write; }
Let's factor out the SCLP boundary and length checks into separate functions. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- hw/s390x/sclp.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)