On 24/04/2025 11:38 am, Roger Pau Monne wrote:
> There's an off-by-one when calculating the last byte in the input array to
> bitmap_to_xenctl_bitmap(), which leads to bitmaps with sizes multiple of 8
> to over-read and incorrectly use a byte past the end of the array.
/sigh
> While there also ensure that bitmap_to_xenctl_bitmap() is not called with a
> bitmap of 0 length.
>
> Fixes: 288c4641c80d ('xen: simplify bitmap_to_xenctl_bitmap for little
> endian')
> Signed-off-by: Roger Pau Monné <roger....@citrix.com>
You ought to note that this is only not getting an XSA because
288c4641c80d isn't in a released Xen yet.
> ---
> xen/common/bitmap.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/bitmap.c b/xen/common/bitmap.c
> index bf1a7fd91e36..415d6bc074f6 100644
> --- a/xen/common/bitmap.c
> +++ b/xen/common/bitmap.c
> @@ -369,6 +369,12 @@ int bitmap_to_xenctl_bitmap(struct xenctl_bitmap
> *xenctl_bitmap,
> const uint8_t *bytemap;
> uint8_t last, *buf = NULL;
>
> + if ( !nbits )
> + {
> + ASSERT_UNREACHABLE();
> + return -EILSEQ;
> + }
I don't see any hypercalls performing a bits==0 check, so I expect this
is reachable.
> +
> if ( !IS_ENABLED(LITTLE_ENDIAN) )
> {
> buf = xmalloc_array(uint8_t, xen_bytes);
> @@ -396,7 +402,7 @@ int bitmap_to_xenctl_bitmap(struct xenctl_bitmap
> *xenctl_bitmap,
> * their loops to 8 bits. Ensure we clear those left over bits so as to
> * prevent surprises.
> */
> - last = bytemap[nbits / 8];
> + last = bytemap[(nbits - 1) / 8];
> if ( nbits % 8 )
> last &= (1U << (nbits % 8)) - 1;
>
This (preexisting) logic is mad. The overwhelming majority of cases are
going to be a multiple of 8, and as you notice, the 0 case can't be
fixed like this.
It should all be inside a copy_bytes conditional as is done in
xenctl_bitmap_to_bitmap().
~Andrew
