| Message ID | 20200624202312.28349-4-walling@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390: Extended-Length SCCB & DIAGNOSE 0x318 | expand |
On 24/06/2020 22.23, Collin Walling wrote: > Rework the SCLP boundary check to account for different SCLP commands > (eventually) allowing different boundary sizes. > > Move the length check code into a separate function, and introduce a > new function to determine the length of the read SCP data (i.e. the size > from the start of the struct to where the CPU entries should begin). > > The format of read CPU info is unlikely to change in the future, > so we do not require a separate function to calculate its length. > > 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 | 54 ++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 44 insertions(+), 10 deletions(-) Reviewed-by: Thomas Huth <thuth@redhat.com>
On 24.06.20 22:23, Collin Walling wrote: > Rework the SCLP boundary check to account for different SCLP commands > (eventually) allowing different boundary sizes. > > Move the length check code into a separate function, and introduce a > new function to determine the length of the read SCP data (i.e. the size > from the start of the struct to where the CPU entries should begin). > > The format of read CPU info is unlikely to change in the future, > so we do not require a separate function to calculate its length. > > 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 | 54 ++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 44 insertions(+), 10 deletions(-) > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index 181ce04007..5899c1e3b8 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 sccb_verify_boundary(uint64_t sccb_addr, uint32_t code, > + SCCBHeader *header) > +{ > + uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(header->length) - 1; > + uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE; > + > + switch (code & SCLP_CMD_CODE_MASK) { > + default: > + if (sccb_max_addr < sccb_boundary) { > + return true; > + } > + } ^ what is that? if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) { return true; } > + header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); > + return false; So we return "false" on success? At least I consider that weird when returning the bool type. Maybe make it clearer what the function indicates "sccb_boundary_is_invalid" or leave it named as is and switch from return value "bool" to "int", using "0" on success and "-EINVAL" on error. > +} > + > +/* Calculates sufficient SCCB length to store a full Read SCP/CPU response */ > +static bool sccb_verify_length(SCCB *sccb, int num_cpus, int offset_cpu) > +{ > + int required_len = offset_cpu + num_cpus * 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(MachineState *ms, CPUEntry *entry, int *count) > { > uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 }; > @@ -66,6 +94,11 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count) > } > } > > +static inline int get_read_scp_info_offset_cpu(void) > +{ > + return offsetof(ReadInfo, entries); > +} > + > /* Provide information about the configuration, CPUs and storage */ > static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > { > @@ -74,17 +107,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > int cpu_count; > int rnsize, rnmax; > IplParameterBlock *ipib = s390_ipl_get_iplb(); > + int offset_cpu = get_read_scp_info_offset_cpu(); > > - if (be16_to_cpu(sccb->h.length) < > - (sizeof(ReadInfo) + machine->possible_cpus->len * sizeof(CPUEntry))) { > - sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); > + if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) { > return; > } > > /* CPU information */ > prepare_cpu_entries(machine, read_info->entries, &cpu_count); > read_info->entries_cpu = cpu_to_be16(cpu_count); > - read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries)); > + read_info->offset_cpu = cpu_to_be16(offset_cpu); > read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1); > > read_info->ibc_val = cpu_to_be32(s390_get_ibc_val()); > @@ -133,17 +165,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb) > { > MachineState *machine = MACHINE(qdev_get_machine()); > ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb; > + int offset_cpu = offsetof(ReadCpuInfo, entries); > int cpu_count; > > - if (be16_to_cpu(sccb->h.length) < > - (sizeof(ReadInfo) + machine->possible_cpus->len * sizeof(CPUEntry))) { > - sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); > + if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) { > return; > } > > prepare_cpu_entries(machine, cpu_info->entries, &cpu_count); > cpu_info->nr_configured = cpu_to_be16(cpu_count); > - cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries)); > + cpu_info->offset_configured = cpu_to_be16(offset_cpu); > cpu_info->nr_standby = cpu_to_be16(0); > > /* The standby offset is 16-byte for each CPU */ > @@ -229,6 +260,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, > goto out_write; > } > > + if (!sccb_verify_boundary(sccb, code, &work_sccb.h)) { > + goto out_write; > + } > + > sclp_c->execute(sclp, &work_sccb, code); > out_write: > s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, > @@ -274,8 +309,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 (!sccb_verify_boundary(sccb, code, &work_sccb.h)) { > goto out_write; > } > >
On 7/20/20 4:17 AM, David Hildenbrand wrote: > On 24.06.20 22:23, Collin Walling wrote: >> Rework the SCLP boundary check to account for different SCLP commands >> (eventually) allowing different boundary sizes. >> >> Move the length check code into a separate function, and introduce a >> new function to determine the length of the read SCP data (i.e. the size >> from the start of the struct to where the CPU entries should begin). >> >> The format of read CPU info is unlikely to change in the future, >> so we do not require a separate function to calculate its length. >> >> 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 | 54 ++++++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 44 insertions(+), 10 deletions(-) >> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index 181ce04007..5899c1e3b8 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 sccb_verify_boundary(uint64_t sccb_addr, uint32_t code, >> + SCCBHeader *header) >> +{ >> + uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(header->length) - 1; >> + uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE; >> + >> + switch (code & SCLP_CMD_CODE_MASK) { >> + default: >> + if (sccb_max_addr < sccb_boundary) { >> + return true; >> + } >> + } > > ^ what is that? > > if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) { > return true; > } > I agree it looks pointless in this patch, but it makes more sense in patch #6 where we introduce cases for the SCLP commands that bypass these checks if the extended-length sccb feature is enabled. >> + header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); >> + return false; > > So we return "false" on success? At least I consider that weird when > returning the bool type. Maybe make it clearer what the function indicates > Hmmm... I figured since there were more paths that can lead to success (i.e. when I introduce the feat check in a later patch), then it made more sense to to return false at the end. sclp_command_code_valid has similar logic. But if boolean functions traditionally return true as the last return value, I can rework it to align to coding preferences / standards. > "sccb_boundary_is_invalid" > Unless it's simply the name that is confusing? > or leave it named as is and switch from return value "bool" to "int", > using "0" on success and "-EINVAL" on error. > Is the switch statement an overkill? I thought of it as a cleaner way to later show which commands have a special conditions (introduced in patch 6 for the ELS stuff) instead of a nasty long if statement. The alternative... /* Comment explaining this check */ if ((code & SCLP_CMD_CODE_MASK) & (SCLP_CMDW_READ_SCP_INFO | SCLP_CMDW_READ_SCP_INFO_FORCED | SCLP_CMDW_READ_CPU_INFO) && s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) { return true; } if (sccb_max_addr < sccb_boundary) { return true; } header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); return false; [...]
[...] >>> + switch (code & SCLP_CMD_CODE_MASK) { >>> + default: >>> + if (sccb_max_addr < sccb_boundary) { >>> + return true; >>> + } >>> + } >> >> ^ what is that? >> >> if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) { >> return true; >> } Oh, my tired eyes missed that it's actually only if (sccb_max_addr < sccb_boundary) :) >> > > I agree it looks pointless in this patch, but it makes more sense in > patch #6 where we introduce cases for the SCLP commands that bypass > these checks if the extended-length sccb feature is enabled. I am really a friend of introducing stuff where needed. Just use a simple "if" here and convert to the switch in patch #6. > >>> + header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); >>> + return false; >> >> So we return "false" on success? At least I consider that weird when >> returning the bool type. Maybe make it clearer what the function indicates >> > > Hmmm... I figured since there were more paths that can lead to success > (i.e. when I introduce the feat check in a later patch), then it made > more sense to to return false at the end. sclp_command_code_valid has > similar logic. > > But if boolean functions traditionally return true as the last return > value, I can rework it to align to coding preferences / standards. > >> "sccb_boundary_is_invalid" >> > > Unless it's simply the name that is confusing? The options I would support are 1. "sccb_boundary_is_valid" which returns "true" if valid 2. "sccb_boundary_is_invalid" which returns "true" if invalid 3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not. Which makes reading this code a bit easier. > >> or leave it named as is and switch from return value "bool" to "int", >> using "0" on success and "-EINVAL" on error. >> > > Is the switch statement an overkill? I thought of it as a cleaner way to > later show which commands have a special conditions (introduced in patch > 6 for the ELS stuff) instead of a nasty long if statement. I think the switch make sense in patch #6.
On 7/21/20 4:41 AM, David Hildenbrand wrote: > [...] > >>>> + switch (code & SCLP_CMD_CODE_MASK) { >>>> + default: >>>> + if (sccb_max_addr < sccb_boundary) { >>>> + return true; >>>> + } >>>> + } >>> >>> ^ what is that? >>> >>> if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) { >>> return true; >>> } > > Oh, my tired eyes missed that it's actually only > > if (sccb_max_addr < sccb_boundary) :) > >>> >> >> I agree it looks pointless in this patch, but it makes more sense in >> patch #6 where we introduce cases for the SCLP commands that bypass >> these checks if the extended-length sccb feature is enabled. > > I am really a friend of introducing stuff where needed. Just use a > simple "if" here and convert to the switch in patch #6. > I can understand that. This follows the whole "each patch should be sufficient on its own" way of thinking. A switch with no cases and a default _does_ look silly. >> >>>> + header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); >>>> + return false; >>> >>> So we return "false" on success? At least I consider that weird when >>> returning the bool type. Maybe make it clearer what the function indicates >>> >> >> Hmmm... I figured since there were more paths that can lead to success >> (i.e. when I introduce the feat check in a later patch), then it made >> more sense to to return false at the end. sclp_command_code_valid has >> similar logic. >> >> But if boolean functions traditionally return true as the last return >> value, I can rework it to align to coding preferences / standards. >> >>> "sccb_boundary_is_invalid" >>> >> >> Unless it's simply the name that is confusing? > > The options I would support are > > 1. "sccb_boundary_is_valid" which returns "true" if valid > 2. "sccb_boundary_is_invalid" which returns "true" if invalid > 3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not. > > Which makes reading this code a bit easier. > Sounds good. I'll takes this into consideration for the next round. (I may wait just a little longer for that to allow more reviews to come in from whoever has the time, if that's okay.) >> >>> or leave it named as is and switch from return value "bool" to "int", >>> using "0" on success and "-EINVAL" on error. >>> >> >> Is the switch statement an overkill? I thought of it as a cleaner way to >> later show which commands have a special conditions (introduced in patch >> 6 for the ELS stuff) instead of a nasty long if statement. > > I think the switch make sense in patch #6. >
On Tue, 21 Jul 2020 14:40:14 -0400 Collin Walling <walling@linux.ibm.com> wrote: > On 7/21/20 4:41 AM, David Hildenbrand wrote: > > The options I would support are > > > > 1. "sccb_boundary_is_valid" which returns "true" if valid > > 2. "sccb_boundary_is_invalid" which returns "true" if invalid > > 3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not. > > > > Which makes reading this code a bit easier. > > Of these, I like option 1 best. > > Sounds good. I'll takes this into consideration for the next round. (I > may wait just a little longer for that to allow more reviews to come in > from whoever has the time, if that's okay.) We have to wait for (a) QEMU to do a release and (b) the Linux changes to merge upstream anyway, so we're not in a hurry :) As said before, it already looked good from my side, but the suggested changes are fine with me as well.
On 7/23/20 2:26 AM, Cornelia Huck wrote: > On Tue, 21 Jul 2020 14:40:14 -0400 > Collin Walling <walling@linux.ibm.com> wrote: > >> On 7/21/20 4:41 AM, David Hildenbrand wrote: > >>> The options I would support are >>> >>> 1. "sccb_boundary_is_valid" which returns "true" if valid >>> 2. "sccb_boundary_is_invalid" which returns "true" if invalid >>> 3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not. >>> >>> Which makes reading this code a bit easier. >>> > > Of these, I like option 1 best. > >> >> Sounds good. I'll takes this into consideration for the next round. (I >> may wait just a little longer for that to allow more reviews to come in >> from whoever has the time, if that's okay.) > > We have to wait for (a) QEMU to do a release and (b) the Linux changes > to merge upstream anyway, so we're not in a hurry :) > > As said before, it already looked good from my side, but the suggested > changes are fine with me as well. > > Okay, thanks for the info. I do want to send out a v5 of these patches. While working with someone who is implementing the kernel support for the extended-length SCCB, we found some snags. I'll highlight these changes/fixes in the respective patches on the next version. Thanks!
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c index 181ce04007..5899c1e3b8 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 sccb_verify_boundary(uint64_t sccb_addr, uint32_t code, + SCCBHeader *header) +{ + uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(header->length) - 1; + uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE; + + switch (code & SCLP_CMD_CODE_MASK) { + default: + if (sccb_max_addr < sccb_boundary) { + return true; + } + } + header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); + return false; +} + +/* Calculates sufficient SCCB length to store a full Read SCP/CPU response */ +static bool sccb_verify_length(SCCB *sccb, int num_cpus, int offset_cpu) +{ + int required_len = offset_cpu + num_cpus * 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(MachineState *ms, CPUEntry *entry, int *count) { uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 }; @@ -66,6 +94,11 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count) } } +static inline int get_read_scp_info_offset_cpu(void) +{ + return offsetof(ReadInfo, entries); +} + /* Provide information about the configuration, CPUs and storage */ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) { @@ -74,17 +107,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) int cpu_count; int rnsize, rnmax; IplParameterBlock *ipib = s390_ipl_get_iplb(); + int offset_cpu = get_read_scp_info_offset_cpu(); - if (be16_to_cpu(sccb->h.length) < - (sizeof(ReadInfo) + machine->possible_cpus->len * sizeof(CPUEntry))) { - sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); + if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) { return; } /* CPU information */ prepare_cpu_entries(machine, read_info->entries, &cpu_count); read_info->entries_cpu = cpu_to_be16(cpu_count); - read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries)); + read_info->offset_cpu = cpu_to_be16(offset_cpu); read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1); read_info->ibc_val = cpu_to_be32(s390_get_ibc_val()); @@ -133,17 +165,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb) { MachineState *machine = MACHINE(qdev_get_machine()); ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb; + int offset_cpu = offsetof(ReadCpuInfo, entries); int cpu_count; - if (be16_to_cpu(sccb->h.length) < - (sizeof(ReadInfo) + machine->possible_cpus->len * sizeof(CPUEntry))) { - sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); + if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) { return; } prepare_cpu_entries(machine, cpu_info->entries, &cpu_count); cpu_info->nr_configured = cpu_to_be16(cpu_count); - cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries)); + cpu_info->offset_configured = cpu_to_be16(offset_cpu); cpu_info->nr_standby = cpu_to_be16(0); /* The standby offset is 16-byte for each CPU */ @@ -229,6 +260,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, goto out_write; } + if (!sccb_verify_boundary(sccb, code, &work_sccb.h)) { + goto out_write; + } + sclp_c->execute(sclp, &work_sccb, code); out_write: s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, @@ -274,8 +309,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 (!sccb_verify_boundary(sccb, code, &work_sccb.h)) { goto out_write; }