| Message ID | 20200515222032.18838-4-walling@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390: Extended-Length SCCB & DIAGNOSE 0x318 | expand |
On 5/16/20 12:20 AM, 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). > > Signed-off-by: Collin Walling <walling@linux.ibm.com> > --- > hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 49 insertions(+), 8 deletions(-) > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code, > + SCCBHeader *header) > +{ > + uint64_t current_len = sccb_addr + be16_to_cpu(header->length); > + uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE; Those are addresses not length indications and the names should reflect that. Also don't we need to use PAGE_SIZE - 1? I'm still trying to wake up, so take this with a grain of salt. > + > + switch (code & SCLP_CMD_CODE_MASK) { > + default: > + if (current_len <= allowed_len) { > + 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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len) > +{ > + int required_len = data_len + 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; > +} Hm, from the function name alone I'd not have expected it to also set the response code. > + > static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count) > { > uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 }; > @@ -66,6 +94,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count) > } > } > > +/* > + * The data length denotes the start of the struct to where the first > + * CPU entry is to be allocated. This value also denotes the offset_cpu > + * field. > + */ > +static int get_read_scp_info_data_len(void) > +{ > + return offsetof(ReadInfo, entries); > +} Not sure what the policy for this is, but maybe this can go into a header file? David and Conny will surely make that clear to me :) > + > /* Provide information about the configuration, CPUs and storage */ > static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > { > @@ -74,16 +112,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > int cpu_count; > int rnsize, rnmax; > IplParameterBlock *ipib = s390_ipl_get_iplb(); > + int data_len = get_read_scp_info_data_len(); > > - 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 (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) { > 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(data_len); > read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1); > > read_info->ibc_val = cpu_to_be32(s390_get_ibc_val()); > @@ -132,16 +170,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb) > { > MachineState *machine = MACHINE(qdev_get_machine()); > ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb; > + int data_len = offsetof(ReadCpuInfo, entries); > 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 (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) { > 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(data_len); > cpu_info->nr_standby = cpu_to_be16(0); > > /* The standby offset is 16-byte for each CPU */ > @@ -227,6 +265,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, > goto out_write; > } > > + if (!sccb_has_valid_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, > @@ -272,8 +314,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_has_valid_boundary(sccb, code, &work_sccb.h)) { > goto out_write; > } > >
On 5/18/20 4:50 AM, Janosch Frank wrote: > On 5/16/20 12:20 AM, 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). >> >> Signed-off-by: Collin Walling <walling@linux.ibm.com> >> --- >> hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 49 insertions(+), 8 deletions(-) >> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code, >> + SCCBHeader *header) >> +{ >> + uint64_t current_len = sccb_addr + be16_to_cpu(header->length); >> + uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE; > > Those are addresses not length indications and the names should reflect > that. True > Also don't we need to use PAGE_SIZE - 1? > Technically we need to -1 on both sides since length denotes the size of the sccb in bytes, not the max address. How about this: s/current_len/sccb_max_addr s/allowed_len/sccb_boundary -1 to sccb_max_addr Change the check to: sccb_max_addr < sccb_boundary ? > I'm still trying to wake up, so take this with a grain of salt. > No worries. I appreciate the review nonetheless :) >> + >> + switch (code & SCLP_CMD_CODE_MASK) { >> + default: >> + if (current_len <= allowed_len) { >> + 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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len) >> +{ >> + int required_len = data_len + 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; >> +} > > Hm, from the function name alone I'd not have expected it to also set > the response code. > It also sets the required length in the header for an extended-length sccb. Perhaps this function name doesn't hold up well. Does sccb_check_sufficient_len make more sense? I think the same could be said of the boundary check function, which also sets the response code. What about setting the response code outside the function, similar to what sclp_comand_code_valid does? >> + >> static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count) >> { >> uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 }; >> @@ -66,6 +94,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count) >> } >> } >> >> +/* >> + * The data length denotes the start of the struct to where the first >> + * CPU entry is to be allocated. This value also denotes the offset_cpu >> + * field. >> + */ >> +static int get_read_scp_info_data_len(void) >> +{ >> + return offsetof(ReadInfo, entries); >> +} > > Not sure what the policy for this is, but maybe this can go into a > header file? > David and Conny will surely make that clear to me :) > Not sure either. If anything it might be a good candidate for an inline function. >> + >> /* Provide information about the configuration, CPUs and storage */ >> static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) >> { >> @@ -74,16 +112,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) >> int cpu_count; >> int rnsize, rnmax; >> IplParameterBlock *ipib = s390_ipl_get_iplb(); >> + int data_len = get_read_scp_info_data_len(); >> >> - 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 (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) { >> 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(data_len); >> read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1); >> >> read_info->ibc_val = cpu_to_be32(s390_get_ibc_val()); >> @@ -132,16 +170,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb) >> { >> MachineState *machine = MACHINE(qdev_get_machine()); >> ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb; >> + int data_len = offsetof(ReadCpuInfo, entries); >> 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 (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) { >> 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(data_len); >> cpu_info->nr_standby = cpu_to_be16(0); >> >> /* The standby offset is 16-byte for each CPU */ >> @@ -227,6 +265,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, >> goto out_write; >> } >> >> + if (!sccb_has_valid_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, >> @@ -272,8 +314,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_has_valid_boundary(sccb, code, &work_sccb.h)) { >> goto out_write; >> } >> >> > >
On Mon, 18 May 2020 11:15:07 -0400 Collin Walling <walling@linux.ibm.com> wrote: > On 5/18/20 4:50 AM, Janosch Frank wrote: > > On 5/16/20 12:20 AM, 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). > >> > >> Signed-off-by: Collin Walling <walling@linux.ibm.com> > >> --- > >> hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++------- > >> 1 file changed, 49 insertions(+), 8 deletions(-) > >> > >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > >> index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code, > >> + SCCBHeader *header) > >> +{ > >> + uint64_t current_len = sccb_addr + be16_to_cpu(header->length); > >> + uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE; > > > > Those are addresses not length indications and the names should reflect > > that. > > True > > > Also don't we need to use PAGE_SIZE - 1? > > > > Technically we need to -1 on both sides since length denotes the size of > the sccb in bytes, not the max address. > > How about this: > > s/current_len/sccb_max_addr > s/allowed_len/sccb_boundary +1, like the names. > > -1 to sccb_max_addr > > Change the check to: sccb_max_addr < sccb_boundary > > ? > > > I'm still trying to wake up, so take this with a grain of salt. > > > > No worries. I appreciate the review nonetheless :) > > >> + > >> + switch (code & SCLP_CMD_CODE_MASK) { > >> + default: > >> + if (current_len <= allowed_len) { > >> + 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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len) > >> +{ > >> + int required_len = data_len + 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; > >> +} > > > > Hm, from the function name alone I'd not have expected it to also set > > the response code. > > > > It also sets the required length in the header for an extended-length > sccb. Perhaps this function name doesn't hold up well. > > Does sccb_check_sufficient_len make more sense? To me it does. > > I think the same could be said of the boundary check function, which > also sets the response code. > > What about setting the response code outside the function, similar to > what sclp_comand_code_valid does? Whatever results in the least code churn to make it consistent ;) > > >> + > >> static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count) > >> { > >> uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 }; > >> @@ -66,6 +94,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count) > >> } > >> } > >> > >> +/* > >> + * The data length denotes the start of the struct to where the first > >> + * CPU entry is to be allocated. This value also denotes the offset_cpu > >> + * field. > >> + */ > >> +static int get_read_scp_info_data_len(void) > >> +{ > >> + return offsetof(ReadInfo, entries); > >> +} > > > > Not sure what the policy for this is, but maybe this can go into a > > header file? > > David and Conny will surely make that clear to me :) > > > > Not sure either. If anything it might be a good candidate for an inline > function. If we don't process read info outside of this file, no need to move it to a header. The compiler is probably also smart enough to inline it on its own, I guess.
On 5/19/20 3:19 PM, Cornelia Huck wrote: > On Mon, 18 May 2020 11:15:07 -0400 > Collin Walling <walling@linux.ibm.com> wrote: > >> On 5/18/20 4:50 AM, Janosch Frank wrote: >>> On 5/16/20 12:20 AM, 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). >>>> >>>> Signed-off-by: Collin Walling <walling@linux.ibm.com> >>>> --- >>>> hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++------- >>>> 1 file changed, 49 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >>>> index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code, >>>> + SCCBHeader *header) >>>> +{ >>>> + uint64_t current_len = sccb_addr + be16_to_cpu(header->length); >>>> + uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE; >>> >>> Those are addresses not length indications and the names should reflect >>> that. >> >> True >> >>> Also don't we need to use PAGE_SIZE - 1? >>> >> >> Technically we need to -1 on both sides since length denotes the size of >> the sccb in bytes, not the max address. >> >> How about this: >> >> s/current_len/sccb_max_addr >> s/allowed_len/sccb_boundary > > +1, like the names. > >> >> -1 to sccb_max_addr >> >> Change the check to: sccb_max_addr < sccb_boundary >> >> ? >> >>> I'm still trying to wake up, so take this with a grain of salt. >>> >> >> No worries. I appreciate the review nonetheless :) >> >>>> + >>>> + switch (code & SCLP_CMD_CODE_MASK) { >>>> + default: >>>> + if (current_len <= allowed_len) { >>>> + 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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len) >>>> +{ >>>> + int required_len = data_len + 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; >>>> +} >>> >>> Hm, from the function name alone I'd not have expected it to also set >>> the response code. >>> >> >> It also sets the required length in the header for an extended-length >> sccb. Perhaps this function name doesn't hold up well. >> >> Does sccb_check_sufficient_len make more sense? > > To me it does. > >> >> I think the same could be said of the boundary check function, which >> also sets the response code. >> >> What about setting the response code outside the function, similar to >> what sclp_comand_code_valid does? > > Whatever results in the least code churn to make it consistent ;) > >> >>>> + >>>> static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count) >>>> { >>>> uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 }; >>>> @@ -66,6 +94,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count) >>>> } >>>> } >>>> >>>> +/* >>>> + * The data length denotes the start of the struct to where the first >>>> + * CPU entry is to be allocated. This value also denotes the offset_cpu >>>> + * field. >>>> + */ >>>> +static int get_read_scp_info_data_len(void) >>>> +{ >>>> + return offsetof(ReadInfo, entries); >>>> +} >>> >>> Not sure what the policy for this is, but maybe this can go into a >>> header file? >>> David and Conny will surely make that clear to me :) >>> >> >> Not sure either. If anything it might be a good candidate for an inline >> function. > > If we don't process read info outside of this file, no need to move it > to a header. The compiler is probably also smart enough to inline it on > its own, I guess. > > I'm also ok with the names and the rest
On 16/05/2020 00.20, 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). > > Signed-off-by: Collin Walling <walling@linux.ibm.com> > --- > hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 49 insertions(+), 8 deletions(-) > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code, > + SCCBHeader *header) > +{ > + uint64_t current_len = sccb_addr + be16_to_cpu(header->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; > + } > + } > + 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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len) > +{ > + int required_len = data_len + 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,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count) > } > } > > +/* > + * The data length denotes the start of the struct to where the first > + * CPU entry is to be allocated. This value also denotes the offset_cpu > + * field. > + */ > +static int get_read_scp_info_data_len(void) > +{ > + return offsetof(ReadInfo, entries); > +} > + > /* Provide information about the configuration, CPUs and storage */ > static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > { > @@ -74,16 +112,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > int cpu_count; > int rnsize, rnmax; > IplParameterBlock *ipib = s390_ipl_get_iplb(); > + int data_len = get_read_scp_info_data_len(); > > - 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 (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) { > 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(data_len); > read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1); > > read_info->ibc_val = cpu_to_be32(s390_get_ibc_val()); > @@ -132,16 +170,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb) > { > MachineState *machine = MACHINE(qdev_get_machine()); > ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb; > + int data_len = offsetof(ReadCpuInfo, entries); Is there a reason for not using get_read_scp_info_data_len() here? Thomas
On 6/11/20 8:56 AM, Thomas Huth wrote: > On 16/05/2020 00.20, 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). >> >> Signed-off-by: Collin Walling <walling@linux.ibm.com> >> --- >> hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 49 insertions(+), 8 deletions(-) >> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code, >> + SCCBHeader *header) >> +{ >> + uint64_t current_len = sccb_addr + be16_to_cpu(header->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; >> + } >> + } >> + 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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len) >> +{ >> + int required_len = data_len + 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,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count) >> } >> } >> >> +/* >> + * The data length denotes the start of the struct to where the first >> + * CPU entry is to be allocated. This value also denotes the offset_cpu >> + * field. >> + */ >> +static int get_read_scp_info_data_len(void) >> +{ >> + return offsetof(ReadInfo, entries); >> +} >> + >> /* Provide information about the configuration, CPUs and storage */ >> static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) >> { >> @@ -74,16 +112,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) >> int cpu_count; >> int rnsize, rnmax; >> IplParameterBlock *ipib = s390_ipl_get_iplb(); >> + int data_len = get_read_scp_info_data_len(); >> >> - 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 (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) { >> 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(data_len); >> read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1); >> >> read_info->ibc_val = cpu_to_be32(s390_get_ibc_val()); >> @@ -132,16 +170,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb) >> { >> MachineState *machine = MACHINE(qdev_get_machine()); >> ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb; >> + int data_len = offsetof(ReadCpuInfo, entries); > > Is there a reason for not using get_read_scp_info_data_len() here? > > Thomas > > That function is for Read SCP Info. Read CPU Info does not face the complications that come with new features intruding on the space used for CPU entries (thankfully), so there's no need for a function to determine its data length.
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code, + SCCBHeader *header) +{ + uint64_t current_len = sccb_addr + be16_to_cpu(header->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; + } + } + 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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len) +{ + int required_len = data_len + 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,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count) } } +/* + * The data length denotes the start of the struct to where the first + * CPU entry is to be allocated. This value also denotes the offset_cpu + * field. + */ +static int get_read_scp_info_data_len(void) +{ + return offsetof(ReadInfo, entries); +} + /* Provide information about the configuration, CPUs and storage */ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) { @@ -74,16 +112,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) int cpu_count; int rnsize, rnmax; IplParameterBlock *ipib = s390_ipl_get_iplb(); + int data_len = get_read_scp_info_data_len(); - 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 (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) { 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(data_len); read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1); read_info->ibc_val = cpu_to_be32(s390_get_ibc_val()); @@ -132,16 +170,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb) { MachineState *machine = MACHINE(qdev_get_machine()); ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb; + int data_len = offsetof(ReadCpuInfo, entries); 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 (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) { 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(data_len); cpu_info->nr_standby = cpu_to_be16(0); /* The standby offset is 16-byte for each CPU */ @@ -227,6 +265,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, goto out_write; } + if (!sccb_has_valid_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, @@ -272,8 +314,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_has_valid_boundary(sccb, code, &work_sccb.h)) { goto out_write; }
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). Signed-off-by: Collin Walling <walling@linux.ibm.com> --- hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 8 deletions(-)