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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.04.2025 11:04, Roger Pau Monné wrote:
> 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.

I'd be fine with that, yes.

Jan