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