[Xen-devel] [PATCH 1/5] xen/bitmap: fix bitmap_fill with zero-sized bitmap

Marek Marczykowski-Górecki posted 5 patches 5 weeks ago

[Xen-devel] [PATCH 1/5] xen/bitmap: fix bitmap_fill with zero-sized bitmap

Posted by Marek Marczykowski-Górecki 5 weeks ago
When bitmap_fill(..., 0) is called, do not try to write anything. Before
this patch, it tried to write almost LONG_MAX, surely overwriting
something.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Found while debugging framebuffer located above 4GB. In that case 32bit
variable for it overflows and framebuffer initialization zeroed
unrelated memory. Specifically, it hit mbi->mods_count, so later on
bitmap_fill(module_map, mbi->mods_count) in __start_xen() crashed.
---
 xen/include/xen/bitmap.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h
index fe3c720..0430c1c 100644
--- a/xen/include/xen/bitmap.h
+++ b/xen/include/xen/bitmap.h
@@ -126,6 +126,8 @@ static inline void bitmap_fill(unsigned long *dst, int nbits)
 	size_t nlongs = BITS_TO_LONGS(nbits);
 
 	switch (nlongs) {
+	case 0:
+		break;
 	default:
 		memset(dst, -1, (nlongs - 1) * sizeof(unsigned long));
 		/* fall through */
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/5] xen/bitmap: fix bitmap_fill with zero-sized bitmap

Posted by Jan Beulich 5 weeks ago
>>> On 06.05.19 at 16:50, <marmarek@invisiblethingslab.com> wrote:
> When bitmap_fill(..., 0) is called, do not try to write anything. Before
> this patch, it tried to write almost LONG_MAX, surely overwriting
> something.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

I'm embarrassed, seeing that commit d8a7694e5a ("bitmap_*() should
cope with zero size bitmaps") changed the code to its present shape,
but left the issue un-addressed here despite its title.

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> Found while debugging framebuffer located above 4GB. In that case 32bit
> variable for it overflows and framebuffer initialization zeroed
> unrelated memory. Specifically, it hit mbi->mods_count, so later on
> bitmap_fill(module_map, mbi->mods_count) in __start_xen() crashed.

The origin of your problem being a truncation one, it seems pretty
clear to me that if we want to be able to gracefully handle that,
then we need to stop using plain int in all the involved functions.
I'm curious though which bitmap_fill() it was that you saw misbehave:
There's no such call at all in xen/drivers/video/, and I'm also having
a hard time seeing how the address (rather than the size) of the
frame buffer could be involved here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/5] xen/bitmap: fix bitmap_fill with zero-sized bitmap

Posted by Marek Marczykowski 5 weeks ago
On Tue, May 07, 2019 at 02:10:20AM -0600, Jan Beulich wrote:
> >>> On 06.05.19 at 16:50, <marmarek@invisiblethingslab.com> wrote:
> > Found while debugging framebuffer located above 4GB. In that case 32bit
> > variable for it overflows and framebuffer initialization zeroed
> > unrelated memory. Specifically, it hit mbi->mods_count, so later on
> > bitmap_fill(module_map, mbi->mods_count) in __start_xen() crashed.
> 
> The origin of your problem being a truncation one, it seems pretty
> clear to me that if we want to be able to gracefully handle that,
> then we need to stop using plain int in all the involved functions.
> I'm curious though which bitmap_fill() it was that you saw misbehave:
> There's no such call at all in xen/drivers/video/, and I'm also having
> a hard time seeing how the address (rather than the size) of the
> frame buffer could be involved here.

Truncated framebuffer address (0x0) caused memset() in vesa_init() to
zero (among other things) mbi->mods_count. This triggered the crash as
described above.
Obviously, bitmap_fill() crash was just a fallout here, not the root
cause.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/5] xen/bitmap: fix bitmap_fill with zero-sized bitmap

Posted by Andrew Cooper 5 weeks ago
On 06/05/2019 15:50, Marek Marczykowski-Górecki wrote:
> When bitmap_fill(..., 0) is called, do not try to write anything. Before
> this patch, it tried to write almost LONG_MAX, surely overwriting
> something.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

It looks like all other operations do cope correctly with nbits being 0.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel