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

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

[Roger Pau Monne]

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.

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@citrix.com>
---
 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;
+    }
+
     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;
 
-- 
2.48.1

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

[Andrew Cooper]

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@citrix.com>

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

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

[Roger Pau Monné]

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@citrix.com>
> 
> 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.

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

[Jan Beulich]

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@citrix.com>
>>
>> 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

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

[Roger Pau Monné]

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@citrix.com>
> >>
> >> 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.

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

[Jan Beulich]

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@citrix.com>
>>>>
>>>> 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