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

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

[Eric Farman]

Hi Christian, Matt,

Here's a small pile of additional hardening fixes for consideration.

Took a stab at what should be cc'd to stable vs what shouldn't be,
but open to other opinions.

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: Expand check against no-data-count CCWs
  s390x/sclp: Reject short headers in PV case

 hw/intc/s390_flic_kvm.c |  4 ++--
 hw/s390x/css.c          | 11 +++++++++--
 hw/s390x/sclp.c         |  6 ++++++
 hw/s390x/virtio-ccw.c   | 20 ++++++++++++++++++--
 hw/vfio/ap.c            |  4 ++++
 hw/vfio/ccw.c           |  2 ++
 6 files changed, 41 insertions(+), 6 deletions(-)

-- 
2.53.0

[PATCH 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.

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

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

[Matthew Rosato]

On 9/16/26 4:32 PM, Eric Farman wrote:
> 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.
> 
> 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];

I think this deserve a fixes/cc stable.  This would have been an
out-of-range access before this patch.

The initial support didn't have the concept of MAX_ISC, it showed up in

dde522bbc5 ("s390x: register I/O adapters per ISC during init")

Frankly I think at that point we should have started fencing
registration > MAX_ISC already, but that patch DID locally fence its new
array indexes against MAX_ISC.  It wasn't until

25a08b8ded ("s390x/css: update css_adapter_interrupt")

when the above array index was added without the fence that you are now
adding.  So I think 25a08b8ded is really the culprit.

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

Is this safe to check before thinint_active?

I guess if we get here before an ISC was ever set it would just be
zero-initialized so it wouldn't erroneously trigger this check.

Assuming your reasoning matches mine:

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

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

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

[Christian Borntraeger]

Am 17.09.26 um 01:15 schrieb Matthew Rosato:
> On 9/16/26 4:32 PM, Eric Farman wrote:
>> 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.
>>
>> 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];
> 
> I think this deserve a fixes/cc stable.  This would have been an
> out-of-range access before this patch.
> 
> The initial support didn't have the concept of MAX_ISC, it showed up in
> 
> dde522bbc5 ("s390x: register I/O adapters per ISC during init")

Yes.

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

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

[Eric Farman]

On 9/16/26 7:15 PM, Matthew Rosato wrote:
> On 9/16/26 4:32 PM, Eric Farman wrote:
>> 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.
>>
>> 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];
> 
> I think this deserve a fixes/cc stable.  This would have been an
> out-of-range access before this patch.

Ack.

> 
> The initial support didn't have the concept of MAX_ISC, it showed up in
> 
> dde522bbc5 ("s390x: register I/O adapters per ISC during init")
> 
> Frankly I think at that point we should have started fencing
> registration > MAX_ISC already, but that patch DID locally fence its new
> array indexes against MAX_ISC.  It wasn't until
> 
> 25a08b8ded ("s390x/css: update css_adapter_interrupt")
> 
> when the above array index was added without the fence that you are now
> adding.  So I think 25a08b8ded is really the culprit.

I was considering 25a08b8ded, since the array indexing was the impetus 
for this patch. So will go with that.

> 
>> +    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;
>> +    }
> 
> Is this safe to check before thinint_active?
> 
> I guess if we get here before an ISC was ever set it would just be
> zero-initialized so it wouldn't erroneously trigger this check.

That was my reasoning. I thought that adding it outside the 
thinint_active block would highlight potential shenanigans, even though 
that variable would end up being ignored in that case. Of course...

> 
> Assuming your reasoning matches mine:
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
>> +
>>       ccw_dev->sch->driver_data = dev;
>>       if (ccw_dev->sch->thinint_active) {
>>           dev->routes.adapter.adapter_id = css_get_adapter_id(

... the above css_get_adapter_id() call will already check that it's 
within MAX_ISC, so it's probably unnecessary.

>> @@ -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 =
> 

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

[Eric Farman]

On 9/17/26 8:09 AM, Eric Farman wrote:
> 
> 
> On 9/16/26 7:15 PM, Matthew Rosato wrote:
>> On 9/16/26 4:32 PM, Eric Farman wrote:
>>> 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.
>>>
>>> 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];
>>
>> I think this deserve a fixes/cc stable.  This would have been an
>> out-of-range access before this patch.
> 
> Ack.
> 
>>
>> The initial support didn't have the concept of MAX_ISC, it showed up in
>>
>> dde522bbc5 ("s390x: register I/O adapters per ISC during init")
>>
>> Frankly I think at that point we should have started fencing
>> registration > MAX_ISC already, but that patch DID locally fence its new
>> array indexes against MAX_ISC.  It wasn't until
>>
>> 25a08b8ded ("s390x/css: update css_adapter_interrupt")
>>
>> when the above array index was added without the fence that you are now
>> adding.  So I think 25a08b8ded is really the culprit.
> 
> I was considering 25a08b8ded, since the array indexing was the impetus 
> for this patch. So will go with that.
> 
>>
>>> +    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;
>>> +    }
>>
>> Is this safe to check before thinint_active?
>>
>> I guess if we get here before an ISC was ever set it would just be
>> zero-initialized so it wouldn't erroneously trigger this check.
> 
> That was my reasoning. I thought that adding it outside the 
> thinint_active block would highlight potential shenanigans, even though 
> that variable would end up being ignored in that case. Of course...
> 
>>
>> Assuming your reasoning matches mine:
>>
>> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Ack; thanks.

>>
>>> +
>>>       ccw_dev->sch->driver_data = dev;
>>>       if (ccw_dev->sch->thinint_active) {
>>>           dev->routes.adapter.adapter_id = css_get_adapter_id(
> 
> ... the above css_get_adapter_id() call will already check that it's 
> within MAX_ISC, so it's probably unnecessary.

Just to revisit this comment of my own, this won't reference outside of 
the array, but it would leave the adapter_id set to -1, which is going 
to be a bit confusing. So blocking it as I have it above is more correct.

> 
>>> @@ -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 =
>>
> 

[PATCH 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. The adapter
indicators path checks that it isn't converting from regular
to thin interrupts, but a check in the reverse direction isn't
performed.

Rather than unwinding the existing allocations to allow additional
assignments, just reject these duplicates.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/s390x/virtio-ccw.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index eb6378ea4c..d700db405b 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -565,6 +565,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             ret = -EINVAL;
             break;
         }
+        if (dev->indicators) {
+            /* Do not set without un-setting first */
+            ret = -ENOSYS;
+            break;
+        }
         if (sch->thinint_active) {
             /* Trigger a command reject. */
             ret = -ENOSYS;
@@ -599,6 +604,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             ret = -EINVAL;
             break;
         }
+        if (dev->indicators2) {
+            /* Do not set without un-setting first */
+            ret = -ENOSYS;
+            break;
+        }
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
@@ -657,7 +667,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         }
         if (!ccw.cda) {
             ret = -EFAULT;
-        } else if (dev->indicators && !sch->thinint_active) {
+        } else if (dev->indicators || dev->summary_indicator) {
             /* Trigger a command reject. */
             ret = -ENOSYS;
         } else {
-- 
2.53.0

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

[Matthew Rosato]

On 9/16/26 4:32 PM, Eric Farman wrote:
> The various indicators pointers allow them to be overwritten,
> but leak any existing structs that might be there. The adapter
> indicators path checks that it isn't converting from regular
> to thin interrupts, but a check in the reverse direction isn't
> performed.

guest-triggered memory leak = fixes/stable?

I think
7bca3892cb ("s390x/virtio-ccw: reference-counted indicators")
when we started doing dynamic allocation

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

> 
> Rather than unwinding the existing allocations to allow additional
> assignments, just reject these duplicates.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  hw/s390x/virtio-ccw.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index eb6378ea4c..d700db405b 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -565,6 +565,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>              ret = -EINVAL;
>              break;
>          }
> +        if (dev->indicators) {
> +            /* Do not set without un-setting first */
> +            ret = -ENOSYS;
> +            break;
> +        }
>          if (sch->thinint_active) {
>              /* Trigger a command reject. */
>              ret = -ENOSYS;
> @@ -599,6 +604,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>              ret = -EINVAL;
>              break;
>          }
> +        if (dev->indicators2) {
> +            /* Do not set without un-setting first */
> +            ret = -ENOSYS;
> +            break;
> +        }
>          if (!ccw.cda) {
>              ret = -EFAULT;
>          } else {
> @@ -657,7 +667,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>          }
>          if (!ccw.cda) {
>              ret = -EFAULT;
> -        } else if (dev->indicators && !sch->thinint_active) {
> +        } else if (dev->indicators || dev->summary_indicator) {
>              /* Trigger a command reject. */
>              ret = -ENOSYS;
>          } else {

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

[Christian Borntraeger]

Am 16.09.26 um 22:32 schrieb Eric Farman:
> The various indicators pointers allow them to be overwritten,
> but leak any existing structs that might be there. The adapter
> indicators path checks that it isn't converting from regular
> to thin interrupts, but a check in the reverse direction isn't
> performed.
> 
> Rather than unwinding the existing allocations to allow additional
> assignments, just reject these duplicates.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   hw/s390x/virtio-ccw.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index eb6378ea4c..d700db405b 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -565,6 +565,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>               ret = -EINVAL;
>               break;
>           }
> +        if (dev->indicators) {
> +            /* Do not set without un-setting first */
> +            ret = -ENOSYS;
> +            break;
> +        }
>           if (sch->thinint_active) {
>               /* Trigger a command reject. */
>               ret = -ENOSYS;
> @@ -599,6 +604,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>               ret = -EINVAL;
>               break;
>           }
> +        if (dev->indicators2) {
> +            /* Do not set without un-setting first */
> +            ret = -ENOSYS;
> +            break;
> +        }
>           if (!ccw.cda) {
>               ret = -EFAULT;
>           } else {
> @@ -657,7 +667,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>           }
>           if (!ccw.cda) {
>               ret = -EFAULT;
> -        } else if (dev->indicators && !sch->thinint_active) {
> +        } else if (dev->indicators || dev->summary_indicator) {
>               /* Trigger a command reject. */
>               ret = -ENOSYS;
>           } else {

I think we have to do the full unwind.
The Linux driver deregisters indicators by issuing SET_IND with address 0,
or SET_IND_ADAPTER with device indicator 0. The virtio drivers reset the
device before del_vqs, so the deregistration lands on a clean state and
QEMU records an indicator at address 0. A later rebind or module reload
does not reset again, so the next SET_IND_ADAPTER and the SET_IND fallback
are both rejected and the probe fails. The old condition on SET_IND_ADAPTER
existed precisely to let the deregistration through while thinint is active.
The fix should release the existing indicator before overwriting, or treat
address 0 as deregistration. Releasing before overwriting also handles the
find_vqs error path in the guest, which deregisters without a reset in between.

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

[Eric Farman]

On 9/17/26 3:35 AM, Christian Borntraeger wrote:
> 
> 
> Am 16.09.26 um 22:32 schrieb Eric Farman:
>> The various indicators pointers allow them to be overwritten,
>> but leak any existing structs that might be there. The adapter
>> indicators path checks that it isn't converting from regular
>> to thin interrupts, but a check in the reverse direction isn't
>> performed.

I missed the check actually exists in the reverse direction,
it's just not as involved and blended in with other checks.

>>
>> Rather than unwinding the existing allocations to allow additional
>> assignments, just reject these duplicates.
>>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   hw/s390x/virtio-ccw.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index eb6378ea4c..d700db405b 100644
>> --- a/hw/s390x/virtio-ccw.c
>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -565,6 +565,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>               ret = -EINVAL;
>>               break;
>>           }
>> +        if (dev->indicators) {
>> +            /* Do not set without un-setting first */
>> +            ret = -ENOSYS;
>> +            break;
>> +        }
>>           if (sch->thinint_active) {
>>               /* Trigger a command reject. */
>>               ret = -ENOSYS;
>> @@ -599,6 +604,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>               ret = -EINVAL;
>>               break;
>>           }
>> +        if (dev->indicators2) {
>> +            /* Do not set without un-setting first */
>> +            ret = -ENOSYS;
>> +            break;
>> +        }
>>           if (!ccw.cda) {
>>               ret = -EFAULT;
>>           } else {
>> @@ -657,7 +667,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>           }
>>           if (!ccw.cda) {
>>               ret = -EFAULT;
>> -        } else if (dev->indicators && !sch->thinint_active) {
>> +        } else if (dev->indicators || dev->summary_indicator) {
>>               /* Trigger a command reject. */
>>               ret = -ENOSYS;
>>           } else {
> 
> I think we have to do the full unwind.

Will do; thanks

> The Linux driver deregisters indicators by issuing SET_IND with address 0,
> or SET_IND_ADAPTER with device indicator 0. The virtio drivers reset the
> device before del_vqs, so the deregistration lands on a clean state and
> QEMU records an indicator at address 0. A later rebind or module reload
> does not reset again, so the next SET_IND_ADAPTER and the SET_IND fallback
> are both rejected and the probe fails. The old condition on SET_IND_ADAPTER
> existed precisely to let the deregistration through while thinint is 
> active.
> The fix should release the existing indicator before overwriting, or treat
> address 0 as deregistration. Releasing before overwriting also handles the
> find_vqs error path in the guest, which deregisters without a reset in 
> between.

[PATCH 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")
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 d700db405b..4622c5d46d 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

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

[Matthew Rosato]

On 9/16/26 4:32 PM, Eric Farman wrote:
> 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")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@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 d700db405b..4622c5d46d 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;
>              }
>          }

[PATCH 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")
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

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

[Matthew Rosato]

On 9/16/26 4:32 PM, Eric Farman wrote:
> 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")
> 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;

Technically this one is unnecessary since we're about to return, but I
like it for future-proofing:

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

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

[Anthony Krowiak]

On 9/16/26 4:32 PM, Eric Farman wrote:
> 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")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Anthony Krowiak <akrowiak@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;

[PATCH 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.

Cc: qemu-stable@nongnu.org
Fixes: 6178d4689a ("vfio-ccw: Permit missing IRQs")
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

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

[Matthew Rosato]

On 9/16/26 4:32 PM, Eric Farman wrote:
> 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.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 6178d4689a ("vfio-ccw: Permit missing IRQs")
> 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;

OK, so this patch is ONLY a future-proofing change since err goes out of
scope right after this.
I'm OK with the change from a future-proofing perspective, but IMO it
does not merit a Fixes or a CC stable.

With those removed:

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

>      }
>  
>      return;

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

[Eric Farman]

On 9/16/26 7:15 PM, Matthew Rosato wrote:
> On 9/16/26 4:32 PM, Eric Farman wrote:
>> 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.
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: 6178d4689a ("vfio-ccw: Permit missing IRQs")
>> 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;
> 
> OK, so this patch is ONLY a future-proofing change since err goes out of
> scope right after this.
> I'm OK with the change from a future-proofing perspective, but IMO it
> does not merit a Fixes or a CC stable.

Fair enough. Gone.

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

Thanks!

> 
>>       }
>>   
>>       return;
> 

[PATCH 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).

Cc: qemu-stable@nongnu.org
Fixes: 9eccb8622c ("s390x/kvm: add interface for clearing IO irqs")
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

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

[Matthew Rosato]

On 9/16/26 4:32 PM, Eric Farman wrote:
> 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).
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 9eccb8622c ("s390x/kvm: add interface for clearing IO irqs")
> 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);

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

But AFAICT this means we've never actually driven
KVM_DEV_FLIC_CLEAR_IO_IRQ and instead always took the -ENOYSYS in
kvm_s390_clear_io_flic() -- so probably want to make sure we test this path.

>  }
>  
>  static void kvm_s390_flic_reset(DeviceState *dev)

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

[Christian Borntraeger]

Am 16.09.26 um 22:32 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).
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 9eccb8622c ("s390x/kvm: add interface for clearing IO irqs")
> 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)

Certainly a correct fix but maybe its not as bad. Aggregates > 8 byte are passed as
reference. Looks like the third argument is a pointer to a stack copy when testing, so
the old code might have worked by accident.

      f8c:       d2 0f f0 a8 f0 d8       mvc     168(16,%r15),216(%r15) <--- stack copy
      f92:       b9 14 00 20             lgfr    %r2,%r0
      f96:       41 40 f0 a8             la      %r4,168(%r15) <-- pointer
      f9a:       c0 31 40 18 ae e3       lgfi    %r3,1075359459
      fa0:       d2 07 f0 b8 f0 e8       mvc     184(8,%r15),232(%r15)
      fa6:       c0 e5 00 00 00 00       brasl   %r14,fa6 <kvm_s390_flic_realize+0x136>
                         fa8: R_390_PLT32DBL     ioctl+0x2
                                             KVM_HAS_DEVICE_ATTR, test_attr);

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

[Eric Farman]

On 9/17/26 3:45 AM, Christian Borntraeger wrote:
> 
> 
> Am 16.09.26 um 22:32 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).
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: 9eccb8622c ("s390x/kvm: add interface for clearing IO irqs")
>> 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)
> 
> Certainly a correct fix but maybe its not as bad. Aggregates > 8 byte 
> are passed as
> reference. Looks like the third argument is a pointer to a stack copy 
> when testing, so
> the old code might have worked by accident.

Ah, thanks for the assembler, I should have looked at that.

As I was testing this, I became worried that this whole part of error 
recovery was broken, but I was finding that clear_io_supported was 
already true even without this patch. I presumed ioctl magic, and that 
seems to be the case.

Maybe drop the stable tags from this one then, since it's working "by 
design"

> 
>       f8c:       d2 0f f0 a8 f0 d8       mvc     168(16,%r15),216(%r15) 
> <--- stack copy
>       f92:       b9 14 00 20             lgfr    %r2,%r0
>       f96:       41 40 f0 a8             la      %r4,168(%r15) <-- pointer
>       f9a:       c0 31 40 18 ae e3       lgfi    %r3,1075359459
>       fa0:       d2 07 f0 b8 f0 e8       mvc     184(8,%r15),232(%r15)
>       fa6:       c0 e5 00 00 00 00       brasl   %r14,fa6 
> <kvm_s390_flic_realize+0x136>
>                          fa8: R_390_PLT32DBL     ioctl+0x2
>                                              KVM_HAS_DEVICE_ATTR, 
> test_attr);
> 

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

[Christian Borntraeger]

Am 17.09.26 um 14:15 schrieb Eric Farman:
> 
> 
> On 9/17/26 3:45 AM, Christian Borntraeger wrote:
>>
>>
>> Am 16.09.26 um 22:32 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).
>>>
>>> Cc: qemu-stable@nongnu.org
>>> Fixes: 9eccb8622c ("s390x/kvm: add interface for clearing IO irqs")
>>> 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)
>>
>> Certainly a correct fix but maybe its not as bad. Aggregates > 8 byte are passed as
>> reference. Looks like the third argument is a pointer to a stack copy when testing, so
>> the old code might have worked by accident.
> 
> Ah, thanks for the assembler, I should have looked at that.
> 
> As I was testing this, I became worried that this whole part of error recovery was broken, but I was finding that clear_io_supported was already true even without this patch. I presumed ioctl magic, and that seems to be the case.
> 
> Maybe drop the stable tags from this one then, since it's working "by design"
working by accident, not by design. Yes we can drop cc stable.


> 
>>
>>       f8c:       d2 0f f0 a8 f0 d8       mvc     168(16,%r15),216(%r15) <--- stack copy
>>       f92:       b9 14 00 20             lgfr    %r2,%r0
>>       f96:       41 40 f0 a8             la      %r4,168(%r15) <-- pointer
>>       f9a:       c0 31 40 18 ae e3       lgfi    %r3,1075359459
>>       fa0:       d2 07 f0 b8 f0 e8       mvc     184(8,%r15),232(%r15)
>>       fa6:       c0 e5 00 00 00 00       brasl   %r14,fa6 <kvm_s390_flic_realize+0x136>
>>                          fa8: R_390_PLT32DBL     ioctl+0x2
>>                                              KVM_HAS_DEVICE_ATTR, test_attr);
>>
> 

[PATCH 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 needs to examine its contents
and not the pointer itself.

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 | 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 5e6d422fba..ce9c720512 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -264,7 +264,7 @@ static int __get_all_irqs(KVMS390FLICState *flic,
         }
         len *= 2;
         *buf = g_try_realloc(*buf, len);
-        if (!buf) {
+        if (!*buf) {
             return -ENOMEM;
         }
     } while (r == -ENOMEM && len <= KVM_S390_FLIC_MAX_BUFFER);
-- 
2.53.0

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

[Matthew Rosato]

On 9/16/26 4:32 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 needs to examine its contents
> and not the pointer itself.
> 
> 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 | 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 5e6d422fba..ce9c720512 100644
> --- a/hw/intc/s390_flic_kvm.c
> +++ b/hw/intc/s390_flic_kvm.c
> @@ -264,7 +264,7 @@ static int __get_all_irqs(KVMS390FLICState *flic,
>          }
>          len *= 2;
>          *buf = g_try_realloc(*buf, len);

Aren't we also leaking the old *buf if this fails?

I think you need a local void *foo here to catch the return..

> -        if (!buf) {
> +        if (!*buf) {
>              return -ENOMEM;
>          }

And then here a *buf = foo; to only overwrite *buf in the success case.

>      } while (r == -ENOMEM && len <= KVM_S390_FLIC_MAX_BUFFER);

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

[Eric Farman]

On 9/16/26 7:16 PM, Matthew Rosato wrote:
> On 9/16/26 4:32 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 needs to examine its contents
>> and not the pointer itself.
>>
>> 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 | 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 5e6d422fba..ce9c720512 100644
>> --- a/hw/intc/s390_flic_kvm.c
>> +++ b/hw/intc/s390_flic_kvm.c
>> @@ -264,7 +264,7 @@ static int __get_all_irqs(KVMS390FLICState *flic,
>>           }
>>           len *= 2;
>>           *buf = g_try_realloc(*buf, len);
> 
> Aren't we also leaking the old *buf if this fails?

Argh, yeah of course, the caller is the one doing the malloc/free, so we 
don't want to silently nuke that on error. Will fix.

> 
> I think you need a local void *foo here to catch the return..
> 
>> -        if (!buf) {
>> +        if (!*buf) {
>>               return -ENOMEM;
>>           }
> 
> And then here a *buf = foo; to only overwrite *buf in the success case.
> 
>>       } while (r == -ENOMEM && len <= KVM_S390_FLIC_MAX_BUFFER);
> 

[PATCH 8/9] s390x/css: Expand check against no-data-count CCWs

[Eric Farman]

The channel subsystem will reject channel programs that incorporate
256 CCWs without a data transfer, but the existing check for this
does so against a data address of zero. Expand the check to include
other CCWs that are valid but would transfer no data.

Fixes: e8601dd5d0 ("s390x/css: catch ccw sequence errors")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/s390x/css.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 9da9128d88..c38549c7be 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1016,7 +1016,9 @@ 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 (!ccw.cda ||
+        (ccw.flags & CCW_FLAG_SKIP) ||
+        (ccw.cmd_code == CCW_CMD_NOOP)) {
         if (sch->ccw_no_data_cnt == 255) {
             return -EINVAL;
         }
-- 
2.53.0

Re: [PATCH 8/9] s390x/css: Expand check against no-data-count CCWs

[Matthew Rosato]

On 9/16/26 4:32 PM, Eric Farman wrote:
> The channel subsystem will reject channel programs that incorporate
> 256 CCWs without a data transfer, but the existing check for this
> does so against a data address of zero. Expand the check to include
> other CCWs that are valid but would transfer no data.
> 
> Fixes: e8601dd5d0 ("s390x/css: catch ccw sequence errors")

guest-triggerable, cc stable

> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  hw/s390x/css.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 9da9128d88..c38549c7be 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1016,7 +1016,9 @@ 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 (!ccw.cda ||
> +        (ccw.flags & CCW_FLAG_SKIP) ||
> +        (ccw.cmd_code == CCW_CMD_NOOP)) {

I'm not sure if this list is complete...

I wonder if we could we key off of the number of bytes transferred e.g.
sch->cds.at_byte == 0 instead?  Would have to happen at the end of this
function e.g. in the ret == 0 check maybe?

>          if (sch->ccw_no_data_cnt == 255) {
>              return -EINVAL;
>          }

Re: [PATCH 8/9] s390x/css: Expand check against no-data-count CCWs

[Eric Farman]

On 9/16/26 7:16 PM, Matthew Rosato wrote:
> On 9/16/26 4:32 PM, Eric Farman wrote:
>> The channel subsystem will reject channel programs that incorporate
>> 256 CCWs without a data transfer, but the existing check for this
>> does so against a data address of zero. Expand the check to include
>> other CCWs that are valid but would transfer no data.
>>
>> Fixes: e8601dd5d0 ("s390x/css: catch ccw sequence errors")
> 
> guest-triggerable, cc stable
> 
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   hw/s390x/css.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 9da9128d88..c38549c7be 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -1016,7 +1016,9 @@ 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 (!ccw.cda ||
>> +        (ccw.flags & CCW_FLAG_SKIP) ||
>> +        (ccw.cmd_code == CCW_CMD_NOOP)) {
> 
> I'm not sure if this list is complete...

Surely the list of opcodes for virtio is far smaller than, say, actual 
DASD, but I'm not sure it's missing much. Maybe ccw.count == 0; that's 
valid for format-1 CCWs (we correctly reject it earlier than here for 
format-0), but would not move data.

> 
> I wonder if we could we key off of the number of bytes transferred e.g.
> sch->cds.at_byte == 0 instead?  Would have to happen at the end of this
> function e.g. in the ret == 0 check maybe?

But wouldn't that only be looking at just a single CCW? That's totally 
fine by itself, whereas this is trying to set an upper bound of chained 
CCWs in a single program that don't do meaningful work.

> 
>>           if (sch->ccw_no_data_cnt == 255) {
>>               return -EINVAL;
>>           }
> 

[PATCH 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>
Fixes: c1db53a591 ("s390/sclp: read sccb from mem based on provided length")
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 9/9] s390x/sclp: Reject short headers in PV case

[Matthew Rosato]

On 9/16/26 4:32 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>
> Fixes: c1db53a591 ("s390/sclp: read sccb from mem based on provided length")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Seems like cc stable would make sense here, it sounds exploitable if the
sanitization assumption were to fall apart.

Reviewed-by: Matthew Rosato <mjrosato@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;
>  }

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

[Christian Borntraeger]

Am 16.09.26 um 22:32 schrieb 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.

I think the ultravisor will already sanitize this, but better save than
sorry

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

> 
> Cc: Janosch Frank <frankja@linux.ibm.com>
> Fixes: c1db53a591 ("s390/sclp: read sccb from mem based on provided length")
> 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;
>   }