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

[Eric Farman <farman@linux.ibm.com>]

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