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

[Christian Borntraeger]

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?

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