| Message ID | e54979f9ce16c254c78e4a48e3e5c0eb223f6dac.1557154206.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes for large framebuffer, placed above 4GB | expand |
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
>>> 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
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.
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 */
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(+)