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

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

On Fri, Apr 25, 2025 at 10:54:36AM +0200, Jan Beulich wrote:
> On 24.04.2025 15:04, Roger Pau Monné wrote:
> > 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.
> 
> Still I don't think there should be an assertion here, not even an error.
> As much as memcpy(x, y, 0) is okay, it ought to be okay to invoke this
> function for entirely void bitmaps.

OK, are we fine then with just returning early if bits == 0?  No
assert and no error.

Thanks, Roger.