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

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

[Eric Farman]

A small pile of additional hardening fixes.

v2->v3:
 - Collect reviews/acks (Thanks Christian, Matt!)
 - [CB] Patch 7: Rename realloc variable to new_buf
 - [CB] Patch 8: Move check of at_byte before check of command chaining flag

v1->v2: https://lore.kernel.org/qemu-devel/20260921184305.3791311-1-farman@linux.ibm.com/
 - 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          | 28 ++++++++++++++++++++--------
 hw/s390x/sclp.c         |  6 ++++++
 hw/s390x/virtio-ccw.c   | 29 ++++++++++++++++++++++++++++-
 hw/vfio/ap.c            |  4 ++++
 hw/vfio/ccw.c           |  2 ++
 6 files changed, 65 insertions(+), 12 deletions(-)

-- 
2.53.0

[PATCH v3 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 v3 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")
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
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 v3 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 v3 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 v3 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 v3 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>
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)
-- 
2.53.0

[PATCH v3 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")
Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
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..63c6ce81d9 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 *new_buf = 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) {
+        new_buf = g_try_realloc(*buf, len);
+        if (!new_buf) {
             return -ENOMEM;
         }
+        *buf = new_buf;
     } while (r == -ENOMEM && len <= KVM_S390_FLIC_MAX_BUFFER);
 
     return r;
-- 
2.53.0

[PATCH v3 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 | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 9da9128d88..65341c68c3 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) {
@@ -1103,6 +1096,20 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
     }
     sch->last_cmd = ccw;
     sch->last_cmd_valid = true;
+
+    /*
+     * 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++;
+        }
+    }
+
     if (ret == 0) {
         if (ccw.flags & CCW_FLAG_CC) {
             sch->channel_prog += 8;
-- 
2.53.0

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

[Matthew Rosato]

On 9/22/26 4:15 PM, Eric Farman wrote:
> 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>

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

[PATCH v3 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 v3 9/9] s390x/sclp: Reject short headers in PV case

[Janosch Frank]

On 9/22/26 10:15 PM, Eric Farman wrote:
> 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>
> ---

How would you hit that if the architecture says FW will automatically 
inject a spec exception without exiting to the hypervisor if length < 8?

Did you witness this happening?
Am I missing something?

>   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;
>   }

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

[Christian Borntraeger]

Am 24.09.26 um 16:36 schrieb Janosch Frank:
> On 9/22/26 10:15 PM, Eric Farman wrote:
>> 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>
>> ---
> 
> How would you hit that if the architecture says FW will automatically inject a spec exception without exiting to the hypervisor if length < 8?

As I said in my review, current UV will sanitize this, so this is just defence in depth in case
there would be an implementation error.

> 
> Did you witness this happening?
> Am I missing something?
> 
>>   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;
>>   }
> 

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

[Janosch Frank]

On 9/24/26 4:39 PM, Christian Borntraeger wrote:
> 
> 
> Am 24.09.26 um 16:36 schrieb Janosch Frank:
>> On 9/22/26 10:15 PM, Eric Farman wrote:
>>> 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

...even though such a thing should already be handled by the UV.

>>> 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>
>>> ---
>>
>> How would you hit that if the architecture says FW will automatically inject a spec exception without exiting to the hypervisor if length < 8?
> 
> As I said in my review, current UV will sanitize this, so this is just defence in depth in case
> there would be an implementation error.

That got lost in my mailbox overflow.

Alright, then let's extend the comment and commit description before 
someone deletes this as dead code.

> 
>>
>> Did you witness this happening?
>> Am I missing something?
>>
>>>    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 */

... We should never end up here due to UV checks but let's be sure.

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

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

[Eric Farman]

On 9/25/26 3:58 AM, Janosch Frank wrote:
> On 9/24/26 4:39 PM, Christian Borntraeger wrote:
>>
>>
>> Am 24.09.26 um 16:36 schrieb Janosch Frank:
>>> On 9/22/26 10:15 PM, Eric Farman wrote:
>>>> 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
> 
> ...even though such a thing should already be handled by the UV.
> 
>>>> 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>
>>>> ---
>>>
>>> How would you hit that if the architecture says FW will automatically 
>>> inject a spec exception without exiting to the hypervisor if length < 8?
>>
>> As I said in my review, current UV will sanitize this, so this is just 
>> defence in depth in case
>> there would be an implementation error.
> 
> That got lost in my mailbox overflow.
> 
> Alright, then let's extend the comment and commit description before 
> someone deletes this as dead code.

Works for me. I'll fold the above/below comments in when I apply.

> 
>>
>>>
>>> Did you witness this happening?
>>> Am I missing something?
>>>
>>>>    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 */
> 
> ... We should never end up here due to UV checks but let's be sure.
> 
>>>> +    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;
>>>>    }
>>>
>>
> 

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

[Christian Borntraeger]

Am 22.09.26 um 22:15 schrieb Eric Farman:
> A small pile of additional hardening fixes.
> 
> v2->v3:
>   - Collect reviews/acks (Thanks Christian, Matt!)
>   - [CB] Patch 7: Rename realloc variable to new_buf
>   - [CB] Patch 8: Move check of at_byte before check of command chaining flag
> 
> v1->v2: https://lore.kernel.org/qemu-devel/20260921184305.3791311-1-farman@linux.ibm.com/
>   - 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
Series looks sane to me.

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

[Eric Farman]

On 9/22/26 4:15 PM, Eric Farman wrote:
> A small pile of additional hardening fixes.
> 
> v2->v3:
>   - Collect reviews/acks (Thanks Christian, Matt!)
>   - [CB] Patch 7: Rename realloc variable to new_buf
>   - [CB] Patch 8: Move check of at_byte before check of command chaining flag
> 
> v1->v2: https://lore.kernel.org/qemu-devel/20260921184305.3791311-1-farman@linux.ibm.com/
>   - 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          | 28 ++++++++++++++++++++--------
>   hw/s390x/sclp.c         |  6 ++++++
>   hw/s390x/virtio-ccw.c   | 29 ++++++++++++++++++++++++++++-
>   hw/vfio/ap.c            |  4 ++++
>   hw/vfio/ccw.c           |  2 ++
>   6 files changed, 65 insertions(+), 12 deletions(-)
> 

Applied for 11.2; thank you!