Re: [PATCH] xen: fix buffer over-read in bitmap_to_xenctl_bitmap()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.pau@xxxxxxxxxx>

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