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

Commit Message

Christian Borntraeger March 10, 2020, 3:09 p.m. UTC

From: Halil Pasic <[e-mail]>

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.

Signed-off-by: Halil Pasic <[e-mail]>
Fixes: 7104bae9de ("hw/s390x: provide loadparm property for the machine")
Reported-by: Marc Hartmayer <[e-mail]>
Reviewed-by: Janosch Frank <[e-mail]>
Reviewed-by: Viktor Mihajlovski <[e-mail]>
Tested-by: Marc Hartmayer <[e-mail]>
Reviewed-by: David Hildenbrand <[e-mail]>
Message-Id: <[e-mail]>
[[e-mail]: use reverse xmas tree]
Signed-off-by: Christian Borntraeger <[e-mail]>
---
 hw/s390x/ipl.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

Peter Maydell March 19, 2020, 8:31 p.m. UTC | #1

On Tue, 10 Mar 2020 at 15:09, Christian Borntraeger
<[e-mail]> wrote:
>
> From: Halil Pasic <[e-mail]>
>
> 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

Christian Borntraeger March 20, 2020, 9:23 a.m. UTC | #2

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

Halil Pasic March 20, 2020, 2:02 p.m. UTC | #3

On Fri, 20 Mar 2020 10:23:03 +0100
Christian Borntraeger <[e-mail]> wrote:

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

Patch

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 9c1ecd423c23..b81942e1e6f9 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -538,6 +538,30 @@  static bool is_virtio_scsi_device(IplParameterBlock *iplb)
     return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_SCSI);
 }
 
+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];
+        int i;
+
+        for (i = 0; i < 8 && ebcdic_loadparm[i]; i++) {
+            ascii_loadparm[i] = ebcdic2ascii[(uint8_t) ebcdic_loadparm[i]];
+        }
+        ascii_loadparm[i] = 0;
+        object_property_set_str(machine, ascii_loadparm, "loadparm", &err);
+    } else {
+        object_property_set_str(machine, "", "loadparm", &err);
+    }
+    if (err) {
+        warn_report_err(err);
+    }
+}
+
 void s390_ipl_update_diag308(IplParameterBlock *iplb)
 {
     S390IPLState *ipl = get_ipl_device();
@@ -545,6 +569,7 @@  void s390_ipl_update_diag308(IplParameterBlock *iplb)
     ipl->iplb = *iplb;
     ipl->iplb_valid = true;
     ipl->netboot = is_virtio_net_device(iplb);
+    update_machine_ipl_properties(iplb);
 }
 
 IplParameterBlock *s390_ipl_get_iplb(void)