[PATCH v2 0/9] s390x: even more hardening fixes

[PATCH v2 0/9] s390x: even more hardening fixes

[Eric Farman]

A small pile of additional hardening fixes.

v1->v2:
 - Collect reviews/acks (Thanks Matt, Christian, Tony!)
 - [MR] Add fixes/stable tags to patches 1, 2, 8
 - [CB, MR] Drop fixes/stable tags from patches 5, 6
 - [CB] Patch 2: Unwind indicators upon re-set, instead of rejecting it
 - [MR] Patch 7: Use a temporary variable on g_try_realloc() call
 - [MR] Patch 8: Use CCW datastream to increment no-data count
v1: https://lore.kernel.org/qemu-devel/20260916203242.2509787-1-farman@linux.ibm.com/

Eric Farman (9):
  virtio-ccw: Limit ISC to MAX_ISC
  virtio-ccw: Do not overwrite existing indicators
  virtio-ccw: Fix rc check in read configuration path
  vfio/ap: Clear error object pointer after non-fatal errors
  vfio/ccw: Clear error object pointer after non-fatal errors
  hw/intc/s390x: Fix payload to KVM_HAS_DEVICE_ATTR ioctl
  hw/intc/s390x: Fix check after g_try_realloc
  s390x/css: Use datastream pointer for checking no-data CCWs
  s390x/sclp: Reject short headers in PV case

 hw/intc/s390_flic_kvm.c |  8 +++++---
 hw/s390x/css.c          | 27 +++++++++++++++++++--------
 hw/s390x/sclp.c         |  6 ++++++
 hw/s390x/virtio-ccw.c   | 29 ++++++++++++++++++++++++++++-
 hw/vfio/ap.c            |  4 ++++
 hw/vfio/ccw.c           |  2 ++
 6 files changed, 64 insertions(+), 12 deletions(-)

-- 
2.53.0

[PATCH v2 1/9] virtio-ccw: Limit ISC to MAX_ISC

[Eric Farman]

The interrupt subclass is often defined as a uint8, though in practice
it will be within the range of just 0-7. Ensure that a guest-supplied
subclass does not extend beyond its expected range, especially when
used as an array index.

Cc: qemu-stable@nongnu.org
Fixes: 25a08b8ded ("s390x/css: update css_adapter_interrupt")
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/s390x/css.c        | 7 ++++++-
 hw/s390x/virtio-ccw.c | 6 ++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 76dbca3bb9..9da9128d88 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -654,8 +654,13 @@ void css_adapter_interrupt(CssIoAdapterType type, uint8_t isc)
     S390FLICState *fs = s390_get_flic();
     S390FLICStateClass *fsc = s390_get_flic_class(fs);
     uint32_t io_int_word = (isc << 27) | IO_INT_WORD_AI;
-    IoAdapter *adapter = channel_subsys.io_adapters[type][isc];
+    IoAdapter *adapter;
+
+    if (type >= CSS_IO_ADAPTER_TYPE_NUMS || isc > MAX_ISC) {
+        return;
+    }
 
+    adapter = channel_subsys.io_adapters[type][isc];
     if (!adapter) {
         return;
     }
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 30fb2c0681..eb6378ea4c 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -46,6 +46,10 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id)
     CcwDevice *ccw_dev = CCW_DEVICE(dev);
     CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
 
+    if (dev->thinint_isc > MAX_ISC) {
+        return -EINVAL;
+    }
+
     ccw_dev->sch->driver_data = dev;
     if (ccw_dev->sch->thinint_active) {
         dev->routes.adapter.adapter_id = css_get_adapter_id(
@@ -659,6 +663,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         } else {
             if (ccw_dstream_read(&sch->cds, thinint)) {
                 ret = -EFAULT;
+            } else if (thinint.isc > MAX_ISC) {
+                ret = -ENOSYS;
             } else {
                 thinint.ind_bit = be64_to_cpu(thinint.ind_bit);
                 thinint.summary_indicator =
-- 
2.53.0

[PATCH v2 2/9] virtio-ccw: Do not overwrite existing indicators

[Eric Farman]

The various indicators pointers allow them to be overwritten,
but leak any existing structs that might be there. If indicators
exist, ensure that they are released before setting the new ones.

Both the regular and adapter interrupt paths check that they
aren't being asked to enable interrupts when the other one is
enabled, so just do this release once all the potential checks
are completed.

Cc: qemu-stable@nongnu.org
Fixes: 7bca3892cb ("s390x/virtio-ccw: reference-counted indicators")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/s390x/virtio-ccw.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index eb6378ea4c..f46f936150 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -582,6 +582,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             if (ret) {
                 break;
             }
+            if (dev->indicators) {
+                /* Need to remove existing indicators first */
+                release_indicator(&dev->routes.adapter, dev->indicators);
+                dev->indicators = NULL;
+            }
             indicators = be64_to_cpu(indicators);
             dev->indicators = get_indicator(indicators, sizeof(uint64_t));
             sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
@@ -606,6 +611,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             if (ret) {
                 break;
             }
+            if (dev->indicators2) {
+                /* Need to remove existing indicators first */
+                release_indicator(&dev->routes.adapter, dev->indicators2);
+                dev->indicators2 = NULL;
+            }
             indicators = be64_to_cpu(indicators);
             dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));
             sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
@@ -666,6 +676,17 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             } else if (thinint.isc > MAX_ISC) {
                 ret = -ENOSYS;
             } else {
+                if (dev->indicators) {
+                    /* Need to remove existing indicators first */
+                    release_indicator(&dev->routes.adapter, dev->indicators);
+                    dev->indicators = NULL;
+                }
+                if (dev->summary_indicator) {
+                    /* Need to remove existing indicators first */
+                    release_indicator(&dev->routes.adapter,
+                                      dev->summary_indicator);
+                    dev->summary_indicator = NULL;
+                }
                 thinint.ind_bit = be64_to_cpu(thinint.ind_bit);
                 thinint.summary_indicator =
                     be64_to_cpu(thinint.summary_indicator);
-- 
2.53.0

[PATCH v2 3/9] virtio-ccw: Fix rc check in read configuration path

[Eric Farman]

Both the read and write config paths use wrappers around cds->op_handler,
depending on the direction the I/O is flowing. But they expect opposite
values in the returned variable to indicate success.

Fix this by unifying those checks, where CCW_CMD_READ_CONF needs to
see !ret as success. The end result is some erroneous data in the
SCSW response data, but the overall success/failure is correct.

Cc: qemu-stable@nongnu.org
Fixes: d895d25ae2 ("s390x: css: report errors from ccw_dstream_read/write")
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/s390x/virtio-ccw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index f46f936150..11441e56c1 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -475,7 +475,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         } else {
             virtio_bus_get_vdev_config(&dev->bus, vdev->config);
             ret = ccw_dstream_write_buf(&sch->cds, vdev->config, len);
-            if (ret) {
+            if (!ret) {
                 sch->curr_status.scsw.count = ccw.count - len;
             }
         }
-- 
2.53.0

[PATCH v2 4/9] vfio/ap: Clear error object pointer after non-fatal errors

[Eric Farman]

Registering a new IRQ notifier will allocate an error object if needed,
and warn_report_err() will release that memory once it's finished.
The pointer remains, however, so subsequent errors after the non-fatal
errors in vfio_ap_realize() would cause an assertion failure for a
non-NULL pointer.

Fix this by clearing the pointer once the warning has been reported,
to reset the state of the error objects.

Cc: Anthony Krowiak <akrowiak@linux.ibm.com>
Cc: qemu-stable@nongnu.org
Fixes: 1360b2ad1f ("s390x/ap: Wire up the device request notifier interface")
Fixes: 0fb8a62fe4 ("hw/vfio/ap: notification handler for AP config changed event")
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/vfio/ap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 6e2a1223ea..2926e04ad4 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -247,8 +247,10 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
         /*
          * Report this error, but do not make it a failing condition.
          * Lack of this IRQ in the host does not prevent normal operation.
+         * Clear the pointer, since the warning will release it.
          */
         warn_report_err(err);
+        err = NULL;
     }
 
     if (!vfio_ap_register_irq_notifier(vapdev, VFIO_AP_CFG_CHG_IRQ_INDEX, &err))
@@ -256,8 +258,10 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
         /*
          * Report this error, but do not make it a failing condition.
          * Lack of this IRQ in the host does not prevent normal operation.
+         * Clear the pointer, since the warning will release it.
          */
         warn_report_err(err);
+        err = NULL;
     }
 
     return;
-- 
2.53.0

[PATCH v2 5/9] vfio/ccw: Clear error object pointer after non-fatal errors

[Eric Farman]

Just like in vfio/ap, there is a non-fatal message that could be
generated, with a stale pointer in the error object variable.

Unlike vfio/ap, there is no secondary message that could attempt
to use it. Clear it in the same way, in case that ever changes.

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/vfio/ccw.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index c3dc7c1962..8e29a3ae61 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -604,8 +604,10 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
         /*
          * Report this error, but do not make it a failing condition.
          * Lack of this IRQ in the host does not prevent normal operation.
+         * Clear the pointer, since the warning will release it.
          */
         warn_report_err(err);
+        err = NULL;
     }
 
     return;
-- 
2.53.0

[PATCH v2 6/9] hw/intc/s390x: Fix payload to KVM_HAS_DEVICE_ATTR ioctl

[Eric Farman]

The ioctl to determine whether KVM_DEV_FLIC_CLEAR_IO_IRQ is present
receives the contents of the attribute struct as its payload,
rather than a pointer to the payload itself as is convention.

This may result in erroneous errors from the ioctl, which would
result in clear_io_supported being false when it otherwise could
be used (which is helpful in device recovery).

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/intc/s390_flic_kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index f833a3996a..5e6d422fba 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -633,7 +633,7 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
     /* Check clear_io_irq support */
     test_attr.group = KVM_DEV_FLIC_CLEAR_IO_IRQ;
     flic_state->clear_io_supported = !ioctl(flic_state->fd,
-                                            KVM_HAS_DEVICE_ATTR, test_attr);
+                                            KVM_HAS_DEVICE_ATTR, &test_attr);
 }
 
 static void kvm_s390_flic_reset(DeviceState *dev)
-- 
2.53.0

[PATCH v2 7/9] hw/intc/s390x: Fix check after g_try_realloc

[Eric Farman]

The loop to store pending IRQs uses g_try_realloc() to expand the
buffer being used, but the returned address is stored in a
pointer-to-pointer, thus the check is supposed to be examining
the contents of the pointer, and not the pointer itself.

Define a temporary variable to catch and test the returned pointer.
If it's NULL, return to the caller to perform the free on the
unmodified buffer. If it's not NULL, store it in the caller's
pointer and restart the loop against the new struct.

Cc: qemu-stable@nongnu.org
Fixes: 3a553fc658 ("s390x/kvm: implement floating-interrupt controller device")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/intc/s390_flic_kvm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index 5e6d422fba..ebee6bca9c 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -254,6 +254,7 @@ static int __get_all_irqs(KVMS390FLICState *flic,
                           void **buf, int len)
 {
     int r;
+    void *realloc = NULL;
 
     do {
         /* returns -ENOMEM if buffer is too small and number
@@ -263,10 +264,11 @@ static int __get_all_irqs(KVMS390FLICState *flic,
             break;
         }
         len *= 2;
-        *buf = g_try_realloc(*buf, len);
-        if (!buf) {
+        realloc = g_try_realloc(*buf, len);
+        if (!realloc) {
             return -ENOMEM;
         }
+        *buf = realloc;
     } while (r == -ENOMEM && len <= KVM_S390_FLIC_MAX_BUFFER);
 
     return r;
-- 
2.53.0

[PATCH v2 8/9] s390x/css: Use datastream pointer for checking no-data CCWs

[Eric Farman]

The channel subsystem will reject channel programs that incorporate
256 CCWs without a data transfer, but the existing check to account
for this only does so for CCWs with a data address of zero. Other
valid CCWs that wouldn't transfer data are not accounted for.

Rather than re-inventing the wheel, use the output of the datastream
at the end of the entire CCW parsing tree to indicate whether data
was moved or not.

Cc: qemu-stable@nongnu.org
Fixes: e8601dd5d0 ("s390x/css: catch ccw sequence errors")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/s390x/css.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 9da9128d88..10bebdcb1a 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1016,13 +1016,6 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
 
     check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC));
 
-    if (!ccw.cda) {
-        if (sch->ccw_no_data_cnt == 255) {
-            return -EINVAL;
-        }
-        sch->ccw_no_data_cnt++;
-    }
-
     /* Look at the command. */
     ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
     switch (ccw.cmd_code) {
@@ -1110,6 +1103,19 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
         }
     }
 
+    /*
+     * A CCW that transfers no data is allowed, but ensure an upper limit
+     * is established to prevent long-running channel programs that don't
+     * move actual data.
+     */
+    if (ret == 0 && sch->cds.at_byte == 0) {
+        if (sch->ccw_no_data_cnt == 255) {
+            ret = -EINVAL;
+        } else {
+            sch->ccw_no_data_cnt++;
+        }
+    }
+
     return ret;
 }
 
-- 
2.53.0

[PATCH v2 9/9] s390x/sclp: Reject short headers in PV case

[Eric Farman]

An invalid length in an SCCB header correctly generates a specification
exception for the regular (non-PV) codepath that processes an SCLP
instruction intercept. The PV case doesn't, even though such a thing
should already be sanitized. Since a short length will cause problems
later down the line with the storage that will be allocated based on
that size, put up a defensive check and return if it is too short.

Cc: Janosch Frank <frankja@linux.ibm.com>
Cc: qemu-stable@nongnu.org
Fixes: c1db53a591 ("s390/sclp: read sccb from mem based on provided length")
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/s390x/sclp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index d8b655efcc..ad8129a5d8 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -277,6 +277,11 @@ int sclp_service_call_protected(S390CPU *cpu, uint64_t sccb, uint32_t code)
 
     s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader));
 
+    /* Do nothing if the header appears malformed */
+    if (be16_to_cpu(header.length) < sizeof(SCCBHeader)) {
+        goto out_no_write;
+    }
+
     work_sccb = g_malloc0(be16_to_cpu(header.length));
     s390_cpu_pv_mem_read(env_archcpu(env), 0, work_sccb,
                          be16_to_cpu(header.length));
@@ -290,6 +295,7 @@ int sclp_service_call_protected(S390CPU *cpu, uint64_t sccb, uint32_t code)
 out_write:
     s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb,
                           be16_to_cpu(header.length));
+out_no_write:
     sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
     return 0;
 }
-- 
2.53.0

Re: [PATCH v2 7/9] hw/intc/s390x: Fix check after g_try_realloc

[Christian Borntraeger]

Am 21.09.26 um 20:43 schrieb Eric Farman:
> The loop to store pending IRQs uses g_try_realloc() to expand the
> buffer being used, but the returned address is stored in a
> pointer-to-pointer, thus the check is supposed to be examining
> the contents of the pointer, and not the pointer itself.
> 
> Define a temporary variable to catch and test the returned pointer.
> If it's NULL, return to the caller to perform the free on the
> unmodified buffer. If it's not NULL, store it in the caller's
> pointer and restart the loop against the new struct.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 3a553fc658 ("s390x/kvm: implement floating-interrupt controller device")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   hw/intc/s390_flic_kvm.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> index 5e6d422fba..ebee6bca9c 100644
> --- a/hw/intc/s390_flic_kvm.c
> +++ b/hw/intc/s390_flic_kvm.c
> @@ -254,6 +254,7 @@ static int __get_all_irqs(KVMS390FLICState *flic,
>                             void **buf, int len)
>   {
>       int r;
> +    void *realloc = NULL;

Ideally use a different name to avoid shadowing the libc function.  (e.g. new_buf)
Other than that

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

>   
>       do {
>           /* returns -ENOMEM if buffer is too small and number
> @@ -263,10 +264,11 @@ static int __get_all_irqs(KVMS390FLICState *flic,
>               break;
>           }
>           len *= 2;
> -        *buf = g_try_realloc(*buf, len);
> -        if (!buf) {
> +        realloc = g_try_realloc(*buf, len);
> +        if (!realloc) {
>               return -ENOMEM;
>           }
> +        *buf = realloc;
>       } while (r == -ENOMEM && len <= KVM_S390_FLIC_MAX_BUFFER);
>   
>       return r;

Re: [PATCH v2 8/9] s390x/css: Use datastream pointer for checking no-data CCWs

[Christian Borntraeger]

Am 21.09.26 um 20:43 schrieb Eric Farman:
> The channel subsystem will reject channel programs that incorporate
> 256 CCWs without a data transfer, but the existing check to account
> for this only does so for CCWs with a data address of zero. Other
> valid CCWs that wouldn't transfer data are not accounted for.
> 
> Rather than re-inventing the wheel, use the output of the datastream
> at the end of the entire CCW parsing tree to indicate whether data
> was moved or not.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: e8601dd5d0 ("s390x/css: catch ccw sequence errors")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   hw/s390x/css.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 9da9128d88..10bebdcb1a 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1016,13 +1016,6 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>   
>       check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC));
>   
> -    if (!ccw.cda) {
> -        if (sch->ccw_no_data_cnt == 255) {
> -            return -EINVAL;
> -        }
> -        sch->ccw_no_data_cnt++;
> -    }
> -
>       /* Look at the command. */
>       ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
>       switch (ccw.cmd_code) {
> @@ -1110,6 +1103,19 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>           }
>       }
>   

These two context lines are the end of

     if (ret == 0) {
         if (ccw.flags & CCW_FLAG_CC) {
             sch->channel_prog += 8;
             ret = -EAGAIN;
         }
     }

so by the time we get here, every successfully executed CCW that has
command chaining set already has ret == -EAGAIN.

> +    /*
> +     * A CCW that transfers no data is allowed, but ensure an upper limit
> +     * is established to prevent long-running channel programs that don't
> +     * move actual data.
> +     */
> +    if (ret == 0 && sch->cds.at_byte == 0) {
> +        if (sch->ccw_no_data_cnt == 255) {
> +            ret = -EINVAL;
> +        } else {
> +            sch->ccw_no_data_cnt++;
> +        }
> +    }
> +
>       return ret;
>   }
>   

Which means this never fires for a chained CCW. The only CCW that can
still be counted is the last one of the program (no CC), and
ccw_no_data_cnt is reset on every SSCH, so the counter can never reach
255. A chain of 100000 NOOPs with CC now runs to completion, while HEAD
rejects it at the 256th CCW when cda is zero. As posted this removes the
protection from e8601dd5d0 rather than extending it.

Moving the check above the CC block (right after last_cmd_valid = true)
should do it, no?

Re: [PATCH v2 6/9] hw/intc/s390x: Fix payload to KVM_HAS_DEVICE_ATTR ioctl

[Christian Borntraeger]

Am 21.09.26 um 20:43 schrieb Eric Farman:
> The ioctl to determine whether KVM_DEV_FLIC_CLEAR_IO_IRQ is present
> receives the contents of the attribute struct as its payload,
> rather than a pointer to the payload itself as is convention.
> 
> This may result in erroneous errors from the ioctl, which would
> result in clear_io_supported being false when it otherwise could
> be used (which is helpful in device recovery).
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   hw/intc/s390_flic_kvm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> index f833a3996a..5e6d422fba 100644
> --- a/hw/intc/s390_flic_kvm.c
> +++ b/hw/intc/s390_flic_kvm.c
> @@ -633,7 +633,7 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
>       /* Check clear_io_irq support */
>       test_attr.group = KVM_DEV_FLIC_CLEAR_IO_IRQ;
>       flic_state->clear_io_supported = !ioctl(flic_state->fd,
> -                                            KVM_HAS_DEVICE_ATTR, test_attr);
> +                                            KVM_HAS_DEVICE_ATTR, &test_attr);
>   }
>   
>   static void kvm_s390_flic_reset(DeviceState *dev)

Re: [PATCH v2 8/9] s390x/css: Use datastream pointer for checking no-data CCWs

[Eric Farman]

On 9/22/26 4:35 AM, Christian Borntraeger wrote:
> Am 21.09.26 um 20:43 schrieb Eric Farman:
>> The channel subsystem will reject channel programs that incorporate
>> 256 CCWs without a data transfer, but the existing check to account
>> for this only does so for CCWs with a data address of zero. Other
>> valid CCWs that wouldn't transfer data are not accounted for.
>>
>> Rather than re-inventing the wheel, use the output of the datastream
>> at the end of the entire CCW parsing tree to indicate whether data
>> was moved or not.
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: e8601dd5d0 ("s390x/css: catch ccw sequence errors")
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   hw/s390x/css.c | 20 +++++++++++++-------
>>   1 file changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 9da9128d88..10bebdcb1a 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -1016,13 +1016,6 @@ static int css_interpret_ccw(SubchDev *sch, 
>> hwaddr ccw_addr,
>>       check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & 
>> CCW_FLAG_DC));
>> -    if (!ccw.cda) {
>> -        if (sch->ccw_no_data_cnt == 255) {
>> -            return -EINVAL;
>> -        }
>> -        sch->ccw_no_data_cnt++;
>> -    }
>> -
>>       /* Look at the command. */
>>       ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
>>       switch (ccw.cmd_code) {
>> @@ -1110,6 +1103,19 @@ static int css_interpret_ccw(SubchDev *sch, 
>> hwaddr ccw_addr,
>>           }
>>       }
> 
> These two context lines are the end of
> 
>      if (ret == 0) {
>          if (ccw.flags & CCW_FLAG_CC) {
>              sch->channel_prog += 8;
>              ret = -EAGAIN;
>          }
>      }
> 
> so by the time we get here, every successfully executed CCW that has
> command chaining set already has ret == -EAGAIN.
> 
>> +    /*
>> +     * A CCW that transfers no data is allowed, but ensure an upper 
>> limit
>> +     * is established to prevent long-running channel programs that 
>> don't
>> +     * move actual data.
>> +     */
>> +    if (ret == 0 && sch->cds.at_byte == 0) {
>> +        if (sch->ccw_no_data_cnt == 255) {
>> +            ret = -EINVAL;
>> +        } else {
>> +            sch->ccw_no_data_cnt++;
>> +        }
>> +    }
>> +
>>       return ret;
>>   }
> 
> Which means this never fires for a chained CCW. The only CCW that can
> still be counted is the last one of the program (no CC), and
> ccw_no_data_cnt is reset on every SSCH, so the counter can never reach
> 255. A chain of 100000 NOOPs with CC now runs to completion, while HEAD
> rejects it at the 256th CCW when cda is zero. As posted this removes the
> protection from e8601dd5d0 rather than extending it.
> 
> Moving the check above the CC block (right after last_cmd_valid = true)
> should do it, no?
> 

Oops, I looked at that and still missed it... Will fix on a v3.

Re: [PATCH v2 2/9] virtio-ccw: Do not overwrite existing indicators

[Matthew Rosato]

On 9/21/26 2:42 PM, Eric Farman wrote:
> The various indicators pointers allow them to be overwritten,
> but leak any existing structs that might be there. If indicators
> exist, ensure that they are released before setting the new ones.
> 
> Both the regular and adapter interrupt paths check that they
> aren't being asked to enable interrupts when the other one is
> enabled, so just do this release once all the potential checks
> are completed.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 7bca3892cb ("s390x/virtio-ccw: reference-counted indicators")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Re: [PATCH v2 7/9] hw/intc/s390x: Fix check after g_try_realloc

[Matthew Rosato]

On 9/21/26 2:43 PM, Eric Farman wrote:
> The loop to store pending IRQs uses g_try_realloc() to expand the
> buffer being used, but the returned address is stored in a
> pointer-to-pointer, thus the check is supposed to be examining
> the contents of the pointer, and not the pointer itself.
> 
> Define a temporary variable to catch and test the returned pointer.
> If it's NULL, return to the caller to perform the free on the
> unmodified buffer. If it's not NULL, store it in the caller's
> pointer and restart the loop against the new struct.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 3a553fc658 ("s390x/kvm: implement floating-interrupt controller device")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
With Christian's suggestion addressed:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>