Re: [PATCH v3 3/8] s390/vfio_ccw: fix out of bounds check on CCW array
From: Eric Farman
Date: Fri Jul 24 2026 - 13:58:55 EST
On 7/24/26 12:49 PM, Matthew Rosato wrote:
On 7/23/26 1:47 PM, Eric Farman wrote:
The routine ccwchain_calc_length() counts the number of channel
command words (CCWs) that are chained together in a single channel
program, and rejects anything larger than CCWCHAIN_LEN_MAX (256) CCWs.
The loop itself is "do..while (count < 257)", and while the logic in
is_cpa_within_range() correctly adjusts between the 0-index array of
CCWs and the count of CCWs starting at 1, this means it would look
at a possible 257th CCW before ending the loop and (correctly)
returning an error.
Fix this by restructuring the loop to break as soon as 256 CCWs
(thus indexes 0-255) are examined, without looking at memory
outside the range.
Fixes: 0a19e61e6d4c ("vfio: ccw: introduce channel program interfaces")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
---
drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
So, I think your approach is correct, but I tried for a bit to come up
with a more concise solution because the while(1) was bothering me.
Ah, yes, it was bothering me too, but I guess I forgot about it.
The best I've managed to cook up is something like:
static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
{
struct ccw1 *ccw = cp->guest_cp;
int cnt;
for (cnt = 1; cnt <= CCWCHAIN_LEN_MAX; cnt++, ccw++) {
if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
return cnt;
}
return -EINVAL;
}
This seems reasonable, and is definitely nicer to read than what I had before (which contained artifacts of the old/busted setup). I'm running tests on this now, but will include in a v4.
Assume CCWCHAIN_LEN_MAX=2
We enter with cnt=1, ccw=guest_cp[0], and 1 <=2 so we enter loop
either return 1 or loop, assume loop so we increment
now cnt=2, ccw=guest_cp[1], 2 <= 2 so we enter loop
either return 2 or loop, assume loop so we increment
now cnt=3, ccw=guest_cp[2]*, 3 <= 2 so we break out of loop and return -EINVAL
*So the only catch is that we will increment ccw past the end, but unlike the
original implementation we will break out of the loop before ever attempting to
use that ccw pointer at that point.
What do you think?
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 1c2890d139c6..258a99930c6a 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -393,11 +393,14 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
break;
- ccw++;
- } while (cnt < CCWCHAIN_LEN_MAX + 1);
+ /* Exit the loop when we reach the maximum */
+ if (cnt >= CCWCHAIN_LEN_MAX) {
+ cnt = -EINVAL;
+ break;
+ }
- if (cnt == CCWCHAIN_LEN_MAX + 1)
- cnt = -EINVAL;
+ ccw++;
+ } while (1);
return cnt;
}