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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Apr 24, 2025 at 12:41:43PM +0100, Andrew Cooper wrote:
> 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.

Yeah, I did explicitly check this wasn't backported to any stable
branches.

> > ---
> >  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.

bitmap_to_xenctl_bitmap() has just two callers, one passes nr_cpu_ids,
the other MAX_NUMNODES.  I think there are no callers that pass 0,
much less from hypercall provided values.

> > +
> >      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's indeed a weird logic.  The usage of last should be restricted to
nbits % 8 != 0, and the rest of the cases handled directly by the
copy_to_guest() call above this logic.

> It should all be inside a copy_bytes conditional as is done in
> xenctl_bitmap_to_bitmap().

I could do it like that, but seeing the values passed by the only two
callers it seemed less churn to add early check and assert.

Thanks, Roger.