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