On Fri, 20 Mar 2020 10:23:03 +0100 Christian Borntraeger <borntrae...@de.ibm.com> wrote:
> > > On 19.03.20 21:31, Peter Maydell wrote: > > On Tue, 10 Mar 2020 at 15:09, Christian Borntraeger > > <borntrae...@de.ibm.com> wrote: > >> > >> From: Halil Pasic <pa...@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