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

[Peter Maydell]

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).

> +        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