* [PATCH v2 0/1] s390x: css: report errors from ccw_dstream_read/write @ 2021-04-08 16:32 Pierre Morel 2021-04-08 16:32 ` [PATCH v2 1/1] " Pierre Morel 0 siblings, 1 reply; 8+ messages in thread From: Pierre Morel @ 2021-04-08 16:32 UTC (permalink / raw) To: qemu-devel Cc: thuth, frankja, david, cohuck, richard.henderson, pasic, borntraeger, qemu-s390x, mst, pbonzini, marcandre.lureau, imbrenda By checking the results of errors on SSCH in the kvm-unit-tests We noticed that no error was reported when a SSCH is started to access addresses not existing in the guest. For exemple accessing 3G on a guest with 1G memory. If we look at QEMU ccw_dstream_write/write functions we see that they are often not checked for error in various places. It follows that accessing an invalid address does not trigger a subchannel status program check to the guest as it should. Regards, Pierre Pierre Morel (1): s390x: css: report errors from ccw_dstream_read/write hw/char/terminal3270.c | 11 +++++-- hw/s390x/3270-ccw.c | 5 +++- hw/s390x/css.c | 14 +++++---- hw/s390x/virtio-ccw.c | 66 ++++++++++++++++++++++++++++++------------ 4 files changed, 69 insertions(+), 27 deletions(-) -- 2.17.1 changelog: from v1: - handle_payload_3270_read, return CSS error on CSS access errors keep returning -EIO for other 3270 internal errors. (Connie) - css_interpret_ccw, let CSS handle the residual count even on errors it is supposed to do it right. (Connie) ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/1] s390x: css: report errors from ccw_dstream_read/write 2021-04-08 16:32 [PATCH v2 0/1] s390x: css: report errors from ccw_dstream_read/write Pierre Morel @ 2021-04-08 16:32 ` Pierre Morel 2021-04-09 8:38 ` Halil Pasic 2021-04-09 10:27 ` Cornelia Huck 0 siblings, 2 replies; 8+ messages in thread From: Pierre Morel @ 2021-04-08 16:32 UTC (permalink / raw) To: qemu-devel Cc: thuth, frankja, david, cohuck, richard.henderson, pasic, borntraeger, qemu-s390x, mst, pbonzini, marcandre.lureau, imbrenda ccw_dstream_read/write functions returned values are sometime not taking into account and reported back to the upper level of interpretation of CCW instructions. It follows that accessing an invalid address does not trigger a subchannel status program check to the guest as it should. Let's test the return values of ccw_dstream_write[_buf] and ccw_dstream_read[_buf] and report it to the caller. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- hw/char/terminal3270.c | 11 +++++-- hw/s390x/3270-ccw.c | 5 +++- hw/s390x/css.c | 14 +++++---- hw/s390x/virtio-ccw.c | 66 ++++++++++++++++++++++++++++++------------ 4 files changed, 69 insertions(+), 27 deletions(-) diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c index a9a46c8ed3..82e85fac2e 100644 --- a/hw/char/terminal3270.c +++ b/hw/char/terminal3270.c @@ -200,9 +200,13 @@ static int read_payload_3270(EmulatedCcw3270Device *dev) { Terminal3270 *t = TERMINAL_3270(dev); int len; + int ret; len = MIN(ccw_dstream_avail(get_cds(t)), t->in_len); - ccw_dstream_write_buf(get_cds(t), t->inv, len); + ret = ccw_dstream_write_buf(get_cds(t), t->inv, len); + if (ret < 0) { + return ret; + } t->in_len -= len; return len; @@ -260,7 +264,10 @@ static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd) t->outv[out_len++] = cmd; do { - ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len); + retval = ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len); + if (retval < 0) { + return retval; + } count = ccw_dstream_avail(get_cds(t)); out_len += len; diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c index 821319eee6..f3e7342b1e 100644 --- a/hw/s390x/3270-ccw.c +++ b/hw/s390x/3270-ccw.c @@ -31,6 +31,9 @@ static int handle_payload_3270_read(EmulatedCcw3270Device *dev, CCW1 *ccw) } len = ck->read_payload_3270(dev); + if (len < 0) { + return len; + } ccw_dev->sch->curr_status.scsw.count = ccw->count - len; return 0; @@ -50,7 +53,7 @@ static int handle_payload_3270_write(EmulatedCcw3270Device *dev, CCW1 *ccw) len = ck->write_payload_3270(dev, ccw->cmd_code); if (len <= 0) { - return -EIO; + return len ? len : -EIO; } ccw_dev->sch->curr_status.scsw.count = ccw->count - len; diff --git a/hw/s390x/css.c b/hw/s390x/css.c index fe47751df4..4149b8e5a7 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -1055,10 +1055,11 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, } } len = MIN(ccw.count, sizeof(sch->sense_data)); - ccw_dstream_write_buf(&sch->cds, sch->sense_data, len); + ret = ccw_dstream_write_buf(&sch->cds, sch->sense_data, len); sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds); - memset(sch->sense_data, 0, sizeof(sch->sense_data)); - ret = 0; + if (!ret) { + memset(sch->sense_data, 0, sizeof(sch->sense_data)); + } break; case CCW_CMD_SENSE_ID: { @@ -1083,9 +1084,10 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, } else { sense_id[0] = 0; } - ccw_dstream_write_buf(&sch->cds, sense_id, len); - sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds); - ret = 0; + ret = ccw_dstream_write_buf(&sch->cds, sense_id, len); + if (!ret) { + sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds); + } break; } case CCW_CMD_TIC: diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 314ed7b245..8195f3546e 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -288,14 +288,20 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len, return -EFAULT; } if (is_legacy) { - ccw_dstream_read(&sch->cds, linfo); + ret = ccw_dstream_read(&sch->cds, linfo); + if (ret) { + return ret; + } linfo.queue = be64_to_cpu(linfo.queue); linfo.align = be32_to_cpu(linfo.align); linfo.index = be16_to_cpu(linfo.index); linfo.num = be16_to_cpu(linfo.num); ret = virtio_ccw_set_vqs(sch, NULL, &linfo); } else { - ccw_dstream_read(&sch->cds, info); + ret = ccw_dstream_read(&sch->cds, info); + if (ret) { + return ret; + } info.desc = be64_to_cpu(info.desc); info.index = be16_to_cpu(info.index); info.num = be16_to_cpu(info.num); @@ -371,7 +377,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); ccw_dstream_advance(&sch->cds, sizeof(features.features)); - ccw_dstream_read(&sch->cds, features.index); + ret = ccw_dstream_read(&sch->cds, features.index); + if (ret) { + break; + } if (features.index == 0) { if (dev->revision >= 1) { /* Don't offer legacy features for modern devices. */ @@ -392,9 +401,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) } ccw_dstream_rewind(&sch->cds); features.features = cpu_to_le32(features.features); - ccw_dstream_write(&sch->cds, features.features); - sch->curr_status.scsw.count = ccw.count - sizeof(features); - ret = 0; + ret = ccw_dstream_write(&sch->cds, features.features); + if (!ret) { + sch->curr_status.scsw.count = ccw.count - sizeof(features); + } } break; case CCW_CMD_WRITE_FEAT: @@ -411,7 +421,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) if (!ccw.cda) { ret = -EFAULT; } else { - ccw_dstream_read(&sch->cds, features); + ret = ccw_dstream_read(&sch->cds, features); + if (ret) { + break; + } features.features = le32_to_cpu(features.features); if (features.index == 0) { virtio_set_features(vdev, @@ -454,9 +467,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) ret = -EFAULT; } else { virtio_bus_get_vdev_config(&dev->bus, vdev->config); - ccw_dstream_write_buf(&sch->cds, vdev->config, len); - sch->curr_status.scsw.count = ccw.count - len; - ret = 0; + ret = ccw_dstream_write_buf(&sch->cds, vdev->config, len); + if (ret) { + sch->curr_status.scsw.count = ccw.count - len; + } } break; case CCW_CMD_WRITE_CONF: @@ -511,7 +525,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) if (!ccw.cda) { ret = -EFAULT; } else { - ccw_dstream_read(&sch->cds, status); + ret = ccw_dstream_read(&sch->cds, status); + if (ret) { + break; + } if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) { virtio_ccw_stop_ioeventfd(dev); } @@ -554,7 +571,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) if (!ccw.cda) { ret = -EFAULT; } else { - ccw_dstream_read(&sch->cds, indicators); + ret = ccw_dstream_read(&sch->cds, indicators); + if (ret) { + break; + } indicators = be64_to_cpu(indicators); dev->indicators = get_indicator(indicators, sizeof(uint64_t)); sch->curr_status.scsw.count = ccw.count - sizeof(indicators); @@ -575,7 +595,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) if (!ccw.cda) { ret = -EFAULT; } else { - ccw_dstream_read(&sch->cds, indicators); + ret = ccw_dstream_read(&sch->cds, indicators); + if (ret) { + break; + } indicators = be64_to_cpu(indicators); dev->indicators2 = get_indicator(indicators, sizeof(uint64_t)); sch->curr_status.scsw.count = ccw.count - sizeof(indicators); @@ -596,7 +619,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) if (!ccw.cda) { ret = -EFAULT; } else { - ccw_dstream_read(&sch->cds, vq_config.index); + ret = ccw_dstream_read(&sch->cds, vq_config.index); + if (ret) { + break; + } vq_config.index = be16_to_cpu(vq_config.index); if (vq_config.index >= VIRTIO_QUEUE_MAX) { ret = -EINVAL; @@ -605,9 +631,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) vq_config.num_max = virtio_queue_get_num(vdev, vq_config.index); vq_config.num_max = cpu_to_be16(vq_config.num_max); - ccw_dstream_write(&sch->cds, vq_config.num_max); - sch->curr_status.scsw.count = ccw.count - sizeof(vq_config); - ret = 0; + ret = ccw_dstream_write(&sch->cds, vq_config.num_max); + if (!ret) { + sch->curr_status.scsw.count = ccw.count - sizeof(vq_config); + } } break; case CCW_CMD_SET_IND_ADAPTER: @@ -664,7 +691,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) ret = -EFAULT; break; } - ccw_dstream_read_buf(&sch->cds, &revinfo, 4); + ret = ccw_dstream_read_buf(&sch->cds, &revinfo, 4); + if (ret < 0) { + break; + } revinfo.revision = be16_to_cpu(revinfo.revision); revinfo.length = be16_to_cpu(revinfo.length); if (ccw.count < len + revinfo.length || -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] s390x: css: report errors from ccw_dstream_read/write 2021-04-08 16:32 ` [PATCH v2 1/1] " Pierre Morel @ 2021-04-09 8:38 ` Halil Pasic 2021-04-09 8:49 ` Cornelia Huck 2021-04-09 10:27 ` Cornelia Huck 1 sibling, 1 reply; 8+ messages in thread From: Halil Pasic @ 2021-04-09 8:38 UTC (permalink / raw) To: Pierre Morel Cc: thuth, frankja, david, cohuck, richard.henderson, qemu-devel, borntraeger, qemu-s390x, mst, marcandre.lureau, pbonzini, imbrenda On Thu, 8 Apr 2021 18:32:09 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > ccw_dstream_read/write functions returned values are sometime > not taking into account and reported back to the upper level > of interpretation of CCW instructions. > > It follows that accessing an invalid address does not trigger > a subchannel status program check to the guest as it should. > > Let's test the return values of ccw_dstream_write[_buf] and > ccw_dstream_read[_buf] and report it to the caller. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> Acked-by: Halil Pasic <pasic@linux.ibm.com> I did not look into the whole scsw.count stuff or into wether your changes to 3270 (look form <mark></mark> in the diff part) affect more than just ccw_dstream_*. I would have preferred this patch split up based on the intended effect and thus also subsystem (css, virtio-ccw, 3270), but I've alluded to that before, and since we are in a hurry I can live with it as is. Regards, Halil > --- > hw/char/terminal3270.c | 11 +++++-- > hw/s390x/3270-ccw.c | 5 +++- > hw/s390x/css.c | 14 +++++---- > hw/s390x/virtio-ccw.c | 66 ++++++++++++++++++++++++++++++------------ > 4 files changed, 69 insertions(+), 27 deletions(-) > > diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c > index a9a46c8ed3..82e85fac2e 100644 > --- a/hw/char/terminal3270.c > +++ b/hw/char/terminal3270.c > @@ -200,9 +200,13 @@ static int read_payload_3270(EmulatedCcw3270Device *dev) > { > Terminal3270 *t = TERMINAL_3270(dev); > int len; > + int ret; > > len = MIN(ccw_dstream_avail(get_cds(t)), t->in_len); > - ccw_dstream_write_buf(get_cds(t), t->inv, len); > + ret = ccw_dstream_write_buf(get_cds(t), t->inv, len); > + if (ret < 0) { > + return ret; > + } > t->in_len -= len; > > return len; > @@ -260,7 +264,10 @@ static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd) > > t->outv[out_len++] = cmd; > do { > - ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len); > + retval = ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len); > + if (retval < 0) { > + return retval; > + } > count = ccw_dstream_avail(get_cds(t)); > out_len += len; > > diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c > index 821319eee6..f3e7342b1e 100644 > --- a/hw/s390x/3270-ccw.c > +++ b/hw/s390x/3270-ccw.c > @@ -31,6 +31,9 @@ static int handle_payload_3270_read(EmulatedCcw3270Device *dev, CCW1 *ccw) > } > > len = ck->read_payload_3270(dev); <mark> > + if (len < 0) { > + return len; > + } > ccw_dev->sch->curr_status.scsw.count = ccw->count - len; > </mark> Do we eventually update scsw.count? > return 0; > @@ -50,7 +53,7 @@ static int handle_payload_3270_write(EmulatedCcw3270Device *dev, CCW1 *ccw) > len = ck->write_payload_3270(dev, ccw->cmd_code); > > if (len <= 0) { <mark> > - return -EIO; > + return len ? len : -EIO; </mark> > } > > ccw_dev->sch->curr_status.scsw.count = ccw->count - len; > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index fe47751df4..4149b8e5a7 100644 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] s390x: css: report errors from ccw_dstream_read/write 2021-04-09 8:38 ` Halil Pasic @ 2021-04-09 8:49 ` Cornelia Huck 2021-04-09 9:55 ` Pierre Morel 0 siblings, 1 reply; 8+ messages in thread From: Cornelia Huck @ 2021-04-09 8:49 UTC (permalink / raw) To: Halil Pasic Cc: thuth, frankja, Pierre Morel, david, mst, richard.henderson, qemu-devel, borntraeger, qemu-s390x, marcandre.lureau, pbonzini, imbrenda On Fri, 9 Apr 2021 10:38:37 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > On Thu, 8 Apr 2021 18:32:09 +0200 > Pierre Morel <pmorel@linux.ibm.com> wrote: > > > ccw_dstream_read/write functions returned values are sometime > > not taking into account and reported back to the upper level > > of interpretation of CCW instructions. > > > > It follows that accessing an invalid address does not trigger > > a subchannel status program check to the guest as it should. > > > > Let's test the return values of ccw_dstream_write[_buf] and > > ccw_dstream_read[_buf] and report it to the caller. > > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > > Acked-by: Halil Pasic <pasic@linux.ibm.com> > > I did not look into the whole scsw.count stuff or into wether > your changes to 3270 (look form <mark></mark> in the diff part) affect > more than just ccw_dstream_*. > > I would have preferred this patch split up based on the intended effect > and thus also subsystem (css, virtio-ccw, 3270), but I've alluded to > that before, and since we are in a hurry I can live with it as is. > > Regards, > Halil > > > --- > > hw/char/terminal3270.c | 11 +++++-- > > hw/s390x/3270-ccw.c | 5 +++- > > hw/s390x/css.c | 14 +++++---- > > hw/s390x/virtio-ccw.c | 66 ++++++++++++++++++++++++++++++------------ > > 4 files changed, 69 insertions(+), 27 deletions(-) > > > > diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c > > index a9a46c8ed3..82e85fac2e 100644 > > --- a/hw/char/terminal3270.c > > +++ b/hw/char/terminal3270.c > > @@ -200,9 +200,13 @@ static int read_payload_3270(EmulatedCcw3270Device *dev) > > { > > Terminal3270 *t = TERMINAL_3270(dev); > > int len; > > + int ret; > > > > len = MIN(ccw_dstream_avail(get_cds(t)), t->in_len); > > - ccw_dstream_write_buf(get_cds(t), t->inv, len); > > + ret = ccw_dstream_write_buf(get_cds(t), t->inv, len); > > + if (ret < 0) { > > + return ret; > > + } > > t->in_len -= len; > > > > return len; > > @@ -260,7 +264,10 @@ static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd) > > > > t->outv[out_len++] = cmd; > > do { > > - ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len); > > + retval = ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len); > > + if (retval < 0) { > > + return retval; > > + } > > count = ccw_dstream_avail(get_cds(t)); > > out_len += len; > > > > diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c > > index 821319eee6..f3e7342b1e 100644 > > --- a/hw/s390x/3270-ccw.c > > +++ b/hw/s390x/3270-ccw.c > > @@ -31,6 +31,9 @@ static int handle_payload_3270_read(EmulatedCcw3270Device *dev, CCW1 *ccw) > > } > > > > len = ck->read_payload_3270(dev); > > <mark> > > > + if (len < 0) { > > + return len; > > + } > > ccw_dev->sch->curr_status.scsw.count = ccw->count - len; > > > > </mark> > > Do we eventually update scsw.count? I think we can consider the contents of scsw.count 'unpredictable', no? > > > return 0; > > @@ -50,7 +53,7 @@ static int handle_payload_3270_write(EmulatedCcw3270Device *dev, CCW1 *ccw) > > len = ck->write_payload_3270(dev, ccw->cmd_code); > > > > if (len <= 0) { > > <mark> > > - return -EIO; > > + return len ? len : -EIO; > > </mark> > > > } > > > > ccw_dev->sch->curr_status.scsw.count = ccw->count - len; > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > > index fe47751df4..4149b8e5a7 100644 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] s390x: css: report errors from ccw_dstream_read/write 2021-04-09 8:49 ` Cornelia Huck @ 2021-04-09 9:55 ` Pierre Morel 2021-04-09 10:11 ` Cornelia Huck 0 siblings, 1 reply; 8+ messages in thread From: Pierre Morel @ 2021-04-09 9:55 UTC (permalink / raw) To: Cornelia Huck, Halil Pasic Cc: thuth, frankja, david, mst, richard.henderson, qemu-devel, borntraeger, qemu-s390x, marcandre.lureau, pbonzini, imbrenda On 4/9/21 10:49 AM, Cornelia Huck wrote: > On Fri, 9 Apr 2021 10:38:37 +0200 > Halil Pasic <pasic@linux.ibm.com> wrote: > >> On Thu, 8 Apr 2021 18:32:09 +0200 >> Pierre Morel <pmorel@linux.ibm.com> wrote: >> >>> ccw_dstream_read/write functions returned values are sometime >>> not taking into account and reported back to the upper level >>> of interpretation of CCW instructions. >>> >>> It follows that accessing an invalid address does not trigger >>> a subchannel status program check to the guest as it should. >>> >>> Let's test the return values of ccw_dstream_write[_buf] and >>> ccw_dstream_read[_buf] and report it to the caller. >>> >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> >> Acked-by: Halil Pasic <pasic@linux.ibm.com> >> >> I did not look into the whole scsw.count stuff or into wether >> your changes to 3270 (look form <mark></mark> in the diff part) affect >> more than just ccw_dstream_*. >> >> I would have preferred this patch split up based on the intended effect >> and thus also subsystem (css, virtio-ccw, 3270), but I've alluded to >> that before, and since we are in a hurry I can live with it as is. >> >> Regards, >> Halil >> >>> --- >>> hw/char/terminal3270.c | 11 +++++-- >>> hw/s390x/3270-ccw.c | 5 +++- >>> hw/s390x/css.c | 14 +++++---- >>> hw/s390x/virtio-ccw.c | 66 ++++++++++++++++++++++++++++++------------ >>> 4 files changed, 69 insertions(+), 27 deletions(-) >>> >>> diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c >>> index a9a46c8ed3..82e85fac2e 100644 >>> --- a/hw/char/terminal3270.c >>> +++ b/hw/char/terminal3270.c >>> @@ -200,9 +200,13 @@ static int read_payload_3270(EmulatedCcw3270Device *dev) >>> { >>> Terminal3270 *t = TERMINAL_3270(dev); >>> int len; >>> + int ret; >>> >>> len = MIN(ccw_dstream_avail(get_cds(t)), t->in_len); >>> - ccw_dstream_write_buf(get_cds(t), t->inv, len); >>> + ret = ccw_dstream_write_buf(get_cds(t), t->inv, len); >>> + if (ret < 0) { >>> + return ret; >>> + } >>> t->in_len -= len; >>> >>> return len; >>> @@ -260,7 +264,10 @@ static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd) >>> >>> t->outv[out_len++] = cmd; >>> do { >>> - ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len); >>> + retval = ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len); >>> + if (retval < 0) { >>> + return retval; >>> + } >>> count = ccw_dstream_avail(get_cds(t)); >>> out_len += len; >>> >>> diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c >>> index 821319eee6..f3e7342b1e 100644 >>> --- a/hw/s390x/3270-ccw.c >>> +++ b/hw/s390x/3270-ccw.c >>> @@ -31,6 +31,9 @@ static int handle_payload_3270_read(EmulatedCcw3270Device *dev, CCW1 *ccw) >>> } >>> >>> len = ck->read_payload_3270(dev); >> >> <mark> >> >>> + if (len < 0) { >>> + return len; >>> + } >>> ccw_dev->sch->curr_status.scsw.count = ccw->count - len; >>> >> >> </mark> >> >> Do we eventually update scsw.count? > > I think we can consider the contents of scsw.count 'unpredictable', no? I think so, the (len < 0) here will trigger a program check and the POP specifies the count as "not meaningful" in case of a program check. > >> >>> return 0; >>> @@ -50,7 +53,7 @@ static int handle_payload_3270_write(EmulatedCcw3270Device *dev, CCW1 *ccw) >>> len = ck->write_payload_3270(dev, ccw->cmd_code); >>> >>> if (len <= 0) { >> >> <mark> >>> - return -EIO; >>> + return len ? len : -EIO; >> >> </mark> Here we do not change the previous behavior. This problem, if it is one, is not related to not checking the dstream read/write functions. >> >>> } >>> >>> ccw_dev->sch->curr_status.scsw.count = ccw->count - len; >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >>> index fe47751df4..4149b8e5a7 100644 >> > -- Pierre Morel IBM Lab Boeblingen ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] s390x: css: report errors from ccw_dstream_read/write 2021-04-09 9:55 ` Pierre Morel @ 2021-04-09 10:11 ` Cornelia Huck 0 siblings, 0 replies; 8+ messages in thread From: Cornelia Huck @ 2021-04-09 10:11 UTC (permalink / raw) To: Pierre Morel Cc: thuth, frankja, david, mst, richard.henderson, qemu-devel, Halil Pasic, borntraeger, qemu-s390x, marcandre.lureau, pbonzini, imbrenda On Fri, 9 Apr 2021 11:55:56 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 4/9/21 10:49 AM, Cornelia Huck wrote: > > On Fri, 9 Apr 2021 10:38:37 +0200 > > Halil Pasic <pasic@linux.ibm.com> wrote: > > > >> On Thu, 8 Apr 2021 18:32:09 +0200 > >> Pierre Morel <pmorel@linux.ibm.com> wrote: > >> > >>> ccw_dstream_read/write functions returned values are sometime > >>> not taking into account and reported back to the upper level > >>> of interpretation of CCW instructions. > >>> > >>> It follows that accessing an invalid address does not trigger > >>> a subchannel status program check to the guest as it should. > >>> > >>> Let's test the return values of ccw_dstream_write[_buf] and > >>> ccw_dstream_read[_buf] and report it to the caller. > >>> > >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > >> > >> Acked-by: Halil Pasic <pasic@linux.ibm.com> > >> > >> I did not look into the whole scsw.count stuff or into wether > >> your changes to 3270 (look form <mark></mark> in the diff part) affect > >> more than just ccw_dstream_*. > >> > >> I would have preferred this patch split up based on the intended effect > >> and thus also subsystem (css, virtio-ccw, 3270), but I've alluded to > >> that before, and since we are in a hurry I can live with it as is. > >> > >> Regards, > >> Halil > >> > >>> --- > >>> hw/char/terminal3270.c | 11 +++++-- > >>> hw/s390x/3270-ccw.c | 5 +++- > >>> hw/s390x/css.c | 14 +++++---- > >>> hw/s390x/virtio-ccw.c | 66 ++++++++++++++++++++++++++++++------------ > >>> 4 files changed, 69 insertions(+), 27 deletions(-) > >>> > >>> diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c > >>> index a9a46c8ed3..82e85fac2e 100644 > >>> --- a/hw/char/terminal3270.c > >>> +++ b/hw/char/terminal3270.c > >>> @@ -200,9 +200,13 @@ static int read_payload_3270(EmulatedCcw3270Device *dev) > >>> { > >>> Terminal3270 *t = TERMINAL_3270(dev); > >>> int len; > >>> + int ret; > >>> > >>> len = MIN(ccw_dstream_avail(get_cds(t)), t->in_len); > >>> - ccw_dstream_write_buf(get_cds(t), t->inv, len); > >>> + ret = ccw_dstream_write_buf(get_cds(t), t->inv, len); > >>> + if (ret < 0) { > >>> + return ret; > >>> + } > >>> t->in_len -= len; > >>> > >>> return len; > >>> @@ -260,7 +264,10 @@ static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd) > >>> > >>> t->outv[out_len++] = cmd; > >>> do { > >>> - ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len); > >>> + retval = ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len); > >>> + if (retval < 0) { > >>> + return retval; > >>> + } > >>> count = ccw_dstream_avail(get_cds(t)); > >>> out_len += len; > >>> > >>> diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c > >>> index 821319eee6..f3e7342b1e 100644 > >>> --- a/hw/s390x/3270-ccw.c > >>> +++ b/hw/s390x/3270-ccw.c > >>> @@ -31,6 +31,9 @@ static int handle_payload_3270_read(EmulatedCcw3270Device *dev, CCW1 *ccw) > >>> } > >>> > >>> len = ck->read_payload_3270(dev); > >> > >> <mark> > >> > >>> + if (len < 0) { > >>> + return len; > >>> + } > >>> ccw_dev->sch->curr_status.scsw.count = ccw->count - len; > >>> > >> > >> </mark> > >> > >> Do we eventually update scsw.count? > > > > I think we can consider the contents of scsw.count 'unpredictable', no? > > I think so, the (len < 0) here will trigger a program check and the POP > specifies the count as "not meaningful" in case of a program check. Yes, that's what I meant. > > > > > >> > >>> return 0; > >>> @@ -50,7 +53,7 @@ static int handle_payload_3270_write(EmulatedCcw3270Device *dev, CCW1 *ccw) > >>> len = ck->write_payload_3270(dev, ccw->cmd_code); > >>> > >>> if (len <= 0) { > >> > >> <mark> > >>> - return -EIO; > >>> + return len ? len : -EIO; > >> > >> </mark> > > Here we do not change the previous behavior. > This problem, if it is one, is not related to not checking the dstream > read/write functions. I agree. > > >> > >>> } > >>> > >>> ccw_dev->sch->curr_status.scsw.count = ccw->count - len; > >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c > >>> index fe47751df4..4149b8e5a7 100644 > >> > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] s390x: css: report errors from ccw_dstream_read/write 2021-04-08 16:32 ` [PATCH v2 1/1] " Pierre Morel 2021-04-09 8:38 ` Halil Pasic @ 2021-04-09 10:27 ` Cornelia Huck 2021-04-09 10:32 ` Pierre Morel 1 sibling, 1 reply; 8+ messages in thread From: Cornelia Huck @ 2021-04-09 10:27 UTC (permalink / raw) To: Pierre Morel Cc: thuth, frankja, david, mst, richard.henderson, qemu-devel, pasic, borntraeger, qemu-s390x, pbonzini, marcandre.lureau, imbrenda On Thu, 8 Apr 2021 18:32:09 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > ccw_dstream_read/write functions returned values are sometime > not taking into account and reported back to the upper level > of interpretation of CCW instructions. > > It follows that accessing an invalid address does not trigger > a subchannel status program check to the guest as it should. > > Let's test the return values of ccw_dstream_write[_buf] and > ccw_dstream_read[_buf] and report it to the caller. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > hw/char/terminal3270.c | 11 +++++-- > hw/s390x/3270-ccw.c | 5 +++- > hw/s390x/css.c | 14 +++++---- > hw/s390x/virtio-ccw.c | 66 ++++++++++++++++++++++++++++++------------ > 4 files changed, 69 insertions(+), 27 deletions(-) Thanks, queued to s390-fixes (with cc:stable added.) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] s390x: css: report errors from ccw_dstream_read/write 2021-04-09 10:27 ` Cornelia Huck @ 2021-04-09 10:32 ` Pierre Morel 0 siblings, 0 replies; 8+ messages in thread From: Pierre Morel @ 2021-04-09 10:32 UTC (permalink / raw) To: Cornelia Huck Cc: thuth, frankja, david, mst, richard.henderson, qemu-devel, pasic, borntraeger, qemu-s390x, pbonzini, marcandre.lureau, imbrenda On 4/9/21 12:27 PM, Cornelia Huck wrote: > On Thu, 8 Apr 2021 18:32:09 +0200 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> ccw_dstream_read/write functions returned values are sometime >> not taking into account and reported back to the upper level >> of interpretation of CCW instructions. >> >> It follows that accessing an invalid address does not trigger >> a subchannel status program check to the guest as it should. >> >> Let's test the return values of ccw_dstream_write[_buf] and >> ccw_dstream_read[_buf] and report it to the caller. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> hw/char/terminal3270.c | 11 +++++-- >> hw/s390x/3270-ccw.c | 5 +++- >> hw/s390x/css.c | 14 +++++---- >> hw/s390x/virtio-ccw.c | 66 ++++++++++++++++++++++++++++++------------ >> 4 files changed, 69 insertions(+), 27 deletions(-) > > Thanks, queued to s390-fixes (with cc:stable added.) > Thanks, Pierre -- Pierre Morel IBM Lab Boeblingen ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-04-09 10:33 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-08 16:32 [PATCH v2 0/1] s390x: css: report errors from ccw_dstream_read/write Pierre Morel 2021-04-08 16:32 ` [PATCH v2 1/1] " Pierre Morel 2021-04-09 8:38 ` Halil Pasic 2021-04-09 8:49 ` Cornelia Huck 2021-04-09 9:55 ` Pierre Morel 2021-04-09 10:11 ` Cornelia Huck 2021-04-09 10:27 ` Cornelia Huck 2021-04-09 10:32 ` Pierre Morel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.