| Message ID | 20200310150947.3510824-4-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,1/4] pc-bios: s390x: Save iplb location in lowcore | expand |
On Tue, 10 Mar 2020 at 15:09, Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > From: Halil Pasic <pasic@linux.ibm.com> > > We expose loadparm as a r/w machine property, but if loadparm is set by > the guest via DIAG 308, we don't update the property. Having a > disconnect between the guest view and the QEMU property is not nice in > itself, but things get even worse for SCSI, where under certain > circumstances (see 789b5a401b "s390: Ensure IPL from SCSI works as > expected" for details) we call s390_gen_initial_iplb() on resets > effectively overwriting the guest/user supplied loadparm with the stale > value. Hi; Coverity points out (CID 1421966) that you have a buffer overrun here: > +static void update_machine_ipl_properties(IplParameterBlock *iplb) > +{ > + Object *machine = qdev_get_machine(); > + Error *err = NULL; > + > + /* Sync loadparm */ > + if (iplb->flags & DIAG308_FLAGS_LP_VALID) { > + uint8_t *ebcdic_loadparm = iplb->loadparm; > + char ascii_loadparm[8]; This array is 8 bytes... > + int i; > + > + for (i = 0; i < 8 && ebcdic_loadparm[i]; i++) { > + ascii_loadparm[i] = ebcdic2ascii[(uint8_t) ebcdic_loadparm[i]]; > + } > + ascii_loadparm[i] = 0; ...but you can write 9 bytes into it (8 from the guest-controlled iplb_loadparm buffer plus one for the trailing NUL). > + object_property_set_str(machine, ascii_loadparm, "loadparm", &err); > + } else { > + object_property_set_str(machine, "", "loadparm", &err); > + } > + if (err) { > + warn_report_err(err); > + } > +} thanks -- PMM
On 19.03.20 21:31, Peter Maydell wrote: > On Tue, 10 Mar 2020 at 15:09, Christian Borntraeger > <borntraeger@de.ibm.com> wrote: >> >> From: Halil Pasic <pasic@linux.ibm.com> >> >> We expose loadparm as a r/w machine property, but if loadparm is set by >> the guest via DIAG 308, we don't update the property. Having a >> disconnect between the guest view and the QEMU property is not nice in >> itself, but things get even worse for SCSI, where under certain >> circumstances (see 789b5a401b "s390: Ensure IPL from SCSI works as >> expected" for details) we call s390_gen_initial_iplb() on resets >> effectively overwriting the guest/user supplied loadparm with the stale >> value. > > Hi; Coverity points out (CID 1421966) that you have a buffer overrun here: > >> +static void update_machine_ipl_properties(IplParameterBlock *iplb) >> +{ >> + Object *machine = qdev_get_machine(); >> + Error *err = NULL; >> + >> + /* Sync loadparm */ >> + if (iplb->flags & DIAG308_FLAGS_LP_VALID) { >> + uint8_t *ebcdic_loadparm = iplb->loadparm; >> + char ascii_loadparm[8]; > > This array is 8 bytes... > >> + int i; >> + >> + for (i = 0; i < 8 && ebcdic_loadparm[i]; i++) { >> + ascii_loadparm[i] = ebcdic2ascii[(uint8_t) ebcdic_loadparm[i]]; >> + } >> + ascii_loadparm[i] = 0; > > ...but you can write 9 bytes into it (8 from the guest-controlled > iplb_loadparm buffer plus one for the trailing NUL). Right, so ascii_loadparm needs to be 9 bytes as this needs the trailing 0. Halil, can you spin up a fix patch? > >> + object_property_set_str(machine, ascii_loadparm, "loadparm", &err); >> + } else { >> + object_property_set_str(machine, "", "loadparm", &err); >> + } >> + if (err) { >> + warn_report_err(err); >> + } >> +} > > thanks > -- PMM >
On Fri, 20 Mar 2020 10:23:03 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > On 19.03.20 21:31, Peter Maydell wrote: > > On Tue, 10 Mar 2020 at 15:09, Christian Borntraeger > > <borntraeger@de.ibm.com> wrote: > >> > >> From: Halil Pasic <pasic@linux.ibm.com> > >> > >> We expose loadparm as a r/w machine property, but if loadparm is set by > >> the guest via DIAG 308, we don't update the property. Having a > >> disconnect between the guest view and the QEMU property is not nice in > >> itself, but things get even worse for SCSI, where under certain > >> circumstances (see 789b5a401b "s390: Ensure IPL from SCSI works as > >> expected" for details) we call s390_gen_initial_iplb() on resets > >> effectively overwriting the guest/user supplied loadparm with the stale > >> value. > > > > Hi; Coverity points out (CID 1421966) that you have a buffer overrun here: > > > >> +static void update_machine_ipl_properties(IplParameterBlock *iplb) > >> +{ > >> + Object *machine = qdev_get_machine(); > >> + Error *err = NULL; > >> + > >> + /* Sync loadparm */ > >> + if (iplb->flags & DIAG308_FLAGS_LP_VALID) { > >> + uint8_t *ebcdic_loadparm = iplb->loadparm; > >> + char ascii_loadparm[8]; > > > > This array is 8 bytes... > > > >> + int i; > >> + > >> + for (i = 0; i < 8 && ebcdic_loadparm[i]; i++) { > >> + ascii_loadparm[i] = ebcdic2ascii[(uint8_t) ebcdic_loadparm[i]]; > >> + } > >> + ascii_loadparm[i] = 0; > > > > ...but you can write 9 bytes into it (8 from the guest-controlled > > iplb_loadparm buffer plus one for the trailing NUL). > > Right, so ascii_loadparm needs to be 9 bytes as this needs the trailing 0. > Halil, can you spin up a fix patch? Sure! Regards, Halil
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index 9c1ecd423c23..b81942e1e6f9 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -538,6 +538,30 @@ static bool is_virtio_scsi_device(IplParameterBlock *iplb) return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_SCSI); } +static void update_machine_ipl_properties(IplParameterBlock *iplb) +{ + Object *machine = qdev_get_machine(); + Error *err = NULL; + + /* Sync loadparm */ + if (iplb->flags & DIAG308_FLAGS_LP_VALID) { + uint8_t *ebcdic_loadparm = iplb->loadparm; + char ascii_loadparm[8]; + int i; + + for (i = 0; i < 8 && ebcdic_loadparm[i]; i++) { + ascii_loadparm[i] = ebcdic2ascii[(uint8_t) ebcdic_loadparm[i]]; + } + ascii_loadparm[i] = 0; + object_property_set_str(machine, ascii_loadparm, "loadparm", &err); + } else { + object_property_set_str(machine, "", "loadparm", &err); + } + if (err) { + warn_report_err(err); + } +} + void s390_ipl_update_diag308(IplParameterBlock *iplb) { S390IPLState *ipl = get_ipl_device(); @@ -545,6 +569,7 @@ void s390_ipl_update_diag308(IplParameterBlock *iplb) ipl->iplb = *iplb; ipl->iplb_valid = true; ipl->netboot = is_virtio_net_device(iplb); + update_machine_ipl_properties(iplb); } IplParameterBlock *s390_ipl_get_iplb(void)