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 <[email protected]>
> ---
> 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 <[email protected]>
> +
> 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 =
