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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan