| Message ID | 20200515222032.18838-3-walling@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390: Extended-Length SCCB & DIAGNOSE 0x318 | expand |
On 5/16/20 12:20 AM, Collin Walling wrote: > The SCCB must be checked for a sufficient length before it is filled > with any data. If the length is insufficient, then the SCLP command > is suppressed and the proper response code is set in the SCCB header. > > Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") > Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > --- > hw/s390x/sclp.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index 61e2e2839c..2bd618515e 100644 > --- a/hw/s390x/sclp.c > +++ b/hw/s390x/sclp.c > @@ -75,6 +75,11 @@ 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); > + return; > + } > + > /* CPU information */ > prepare_cpu_entries(machine, read_info->entries, &cpu_count); > read_info->entries_cpu = cpu_to_be16(cpu_count); > @@ -83,12 +88,6 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > > read_info->ibc_val = cpu_to_be32(s390_get_ibc_val()); > > - 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); > - return; > - } > - > /* Configuration Characteristic (Extension) */ > s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR, > read_info->conf_char); > @@ -135,17 +134,16 @@ 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); > + 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->nr_standby = cpu_to_be16(0); > > - 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); > - return; > - } > - > /* The standby offset is 16-byte for each CPU */ > cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured > + cpu_info->nr_configured*sizeof(CPUEntry)); >
On 16.05.20 00:20, Collin Walling wrote: > The SCCB must be checked for a sufficient length before it is filled > with any data. If the length is insufficient, then the SCLP command > is suppressed and the proper response code is set in the SCCB header. > > Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") > Signed-off-by: Collin Walling <walling@linux.ibm.com> > --- > hw/s390x/sclp.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index 61e2e2839c..2bd618515e 100644 > --- a/hw/s390x/sclp.c > +++ b/hw/s390x/sclp.c > @@ -75,6 +75,11 @@ 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); > + return; > + } > + (replied to v1 by mistake) Lines too long. Please run scripts/checkpatch.pl before submitting.
On 5/18/20 7:46 AM, David Hildenbrand wrote: > On 16.05.20 00:20, Collin Walling wrote: >> The SCCB must be checked for a sufficient length before it is filled >> with any data. If the length is insufficient, then the SCLP command >> is suppressed and the proper response code is set in the SCCB header. >> >> Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") >> Signed-off-by: Collin Walling <walling@linux.ibm.com> >> --- >> hw/s390x/sclp.c | 22 ++++++++++------------ >> 1 file changed, 10 insertions(+), 12 deletions(-) >> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index 61e2e2839c..2bd618515e 100644 >> --- a/hw/s390x/sclp.c >> +++ b/hw/s390x/sclp.c >> @@ -75,6 +75,11 @@ 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); >> + return; >> + } >> + > > (replied to v1 by mistake) > > Lines too long. > > Please run scripts/checkpatch.pl before submitting. > I do. The changes in this patch are replaced by the #3. I opted to be sloppy here for ease of readability. But if it's truly an issue I can clean it up for next round.
On 18.05.20 16:32, Collin Walling wrote: > On 5/18/20 7:46 AM, David Hildenbrand wrote: >> On 16.05.20 00:20, Collin Walling wrote: >>> The SCCB must be checked for a sufficient length before it is filled >>> with any data. If the length is insufficient, then the SCLP command >>> is suppressed and the proper response code is set in the SCCB header. >>> >>> Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") >>> Signed-off-by: Collin Walling <walling@linux.ibm.com> >>> --- >>> hw/s390x/sclp.c | 22 ++++++++++------------ >>> 1 file changed, 10 insertions(+), 12 deletions(-) >>> >>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >>> index 61e2e2839c..2bd618515e 100644 >>> --- a/hw/s390x/sclp.c >>> +++ b/hw/s390x/sclp.c >>> @@ -75,6 +75,11 @@ 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); >>> + return; >>> + } >>> + >> >> (replied to v1 by mistake) >> >> Lines too long. >> >> Please run scripts/checkpatch.pl before submitting. >> > > I do. The changes in this patch are replaced by the #3. I opted to be > sloppy here for ease of readability. > > But if it's truly an issue I can clean it up for next round. No good reason to be sloppy and make checkpatch (+ David) complain ;)
On 5/18/20 11:43 AM, David Hildenbrand wrote: > On 18.05.20 16:32, Collin Walling wrote: >> On 5/18/20 7:46 AM, David Hildenbrand wrote: >>> On 16.05.20 00:20, Collin Walling wrote: >>>> The SCCB must be checked for a sufficient length before it is filled >>>> with any data. If the length is insufficient, then the SCLP command >>>> is suppressed and the proper response code is set in the SCCB header. >>>> >>>> Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") >>>> Signed-off-by: Collin Walling <walling@linux.ibm.com> >>>> --- >>>> hw/s390x/sclp.c | 22 ++++++++++------------ >>>> 1 file changed, 10 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >>>> index 61e2e2839c..2bd618515e 100644 >>>> --- a/hw/s390x/sclp.c >>>> +++ b/hw/s390x/sclp.c >>>> @@ -75,6 +75,11 @@ 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); >>>> + return; >>>> + } >>>> + >>> >>> (replied to v1 by mistake) >>> >>> Lines too long. >>> >>> Please run scripts/checkpatch.pl before submitting. >>> >> >> I do. The changes in this patch are replaced by the #3. I opted to be >> sloppy here for ease of readability. >> >> But if it's truly an issue I can clean it up for next round. > > No good reason to be sloppy and make checkpatch (+ David) complain ;) > > Fair enough. I'll make the fix!
On 16/05/2020 00.20, Collin Walling wrote: > The SCCB must be checked for a sufficient length before it is filled > with any data. If the length is insufficient, then the SCLP command > is suppressed and the proper response code is set in the SCCB header. > > Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") > Signed-off-by: Collin Walling <walling@linux.ibm.com> > --- > hw/s390x/sclp.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index 61e2e2839c..2bd618515e 100644 > --- a/hw/s390x/sclp.c > +++ b/hw/s390x/sclp.c > @@ -75,6 +75,11 @@ 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))) { You are using cpu_count here ... > + sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); > + return; > + } > + > /* CPU information */ > prepare_cpu_entries(machine, read_info->entries, &cpu_count); ... but it is only initialized here. Even if you replace the code in a later patch, please fix this here, too, to maintain bisectability of the code. > read_info->entries_cpu = cpu_to_be16(cpu_count); > @@ -83,12 +88,6 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > > read_info->ibc_val = cpu_to_be32(s390_get_ibc_val()); > > - 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); > - return; > - } > - > /* Configuration Characteristic (Extension) */ > s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR, > read_info->conf_char); > @@ -135,17 +134,16 @@ 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))) { dito! > + sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); > + 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->nr_standby = cpu_to_be16(0); > > - 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); > - return; > - } > - > /* The standby offset is 16-byte for each CPU */ > cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured > + cpu_info->nr_configured*sizeof(CPUEntry)); Thanks, Thomas
On 6/11/20 8:01 AM, Thomas Huth wrote: > On 16/05/2020 00.20, Collin Walling wrote: >> The SCCB must be checked for a sufficient length before it is filled >> with any data. If the length is insufficient, then the SCLP command >> is suppressed and the proper response code is set in the SCCB header. >> >> Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") >> Signed-off-by: Collin Walling <walling@linux.ibm.com> >> --- >> hw/s390x/sclp.c | 22 ++++++++++------------ >> 1 file changed, 10 insertions(+), 12 deletions(-) >> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index 61e2e2839c..2bd618515e 100644 >> --- a/hw/s390x/sclp.c >> +++ b/hw/s390x/sclp.c >> @@ -75,6 +75,11 @@ 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))) { > > You are using cpu_count here ... > >> + sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); >> + return; >> + } >> + >> /* CPU information */ >> prepare_cpu_entries(machine, read_info->entries, &cpu_count); > > ... but it is only initialized here. > > Even if you replace the code in a later patch, please fix this here, > too, to maintain bisectability of the code. > >> read_info->entries_cpu = cpu_to_be16(cpu_count); >> @@ -83,12 +88,6 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) >> >> read_info->ibc_val = cpu_to_be32(s390_get_ibc_val()); >> >> - 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); >> - return; >> - } >> - >> /* Configuration Characteristic (Extension) */ >> s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR, >> read_info->conf_char); >> @@ -135,17 +134,16 @@ 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))) { > > dito! > >> + sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH); >> + 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->nr_standby = cpu_to_be16(0); >> >> - 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); >> - return; >> - } >> - >> /* The standby offset is 16-byte for each CPU */ >> cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured >> + cpu_info->nr_configured*sizeof(CPUEntry)); > > Thanks, > Thomas > I'll rework this patch. Thanks for the reviews!
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c index 61e2e2839c..2bd618515e 100644 --- a/hw/s390x/sclp.c +++ b/hw/s390x/sclp.c @@ -75,6 +75,11 @@ 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); + return; + } + /* CPU information */ prepare_cpu_entries(machine, read_info->entries, &cpu_count); read_info->entries_cpu = cpu_to_be16(cpu_count); @@ -83,12 +88,6 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) read_info->ibc_val = cpu_to_be32(s390_get_ibc_val()); - 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); - return; - } - /* Configuration Characteristic (Extension) */ s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR, read_info->conf_char); @@ -135,17 +134,16 @@ 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); + 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->nr_standby = cpu_to_be16(0); - 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); - return; - } - /* The standby offset is 16-byte for each CPU */ cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured + cpu_info->nr_configured*sizeof(CPUEntry));
The SCCB must be checked for a sufficient length before it is filled with any data. If the length is insufficient, then the SCLP command is suppressed and the proper response code is set in the SCCB header. Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") Signed-off-by: Collin Walling <walling@linux.ibm.com> --- hw/s390x/sclp.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)