Re: [PULL 3/4] s390/ipl: sync back loadparm

[Halil Pasic]

On Fri, 20 Mar 2020 10:23:03 +0100
Christian Borntraeger <address@hidden> wrote:

> 
> 
> On 19.03.20 21:31, Peter Maydell wrote:
> > On Tue, 10 Mar 2020 at 15:09, Christian Borntraeger
> > <address@hidden> wrote:
> >>
> >> From: Halil Pasic <address@hidden>
> >>
> >> 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