[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 3/4] s390/ipl: sync back loadparm
From: |
Halil Pasic |
Subject: |
Re: [PULL 3/4] s390/ipl: sync back loadparm |
Date: |
Fri, 20 Mar 2020 15:02:00 +0100 |
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