| Message ID | cad03d25-0ea0-32c4-8173-fd1895314bce@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fbcon: Use kzalloc() in fbcon_prepare_logo() | expand |
On Fri, Nov 18, 2022 at 12:27:58AM +0900, Tetsuo Handa wrote: > A kernel built with syzbot's config file reported that > > scr_memcpyw(q, save, array3_size(logo_lines, new_cols, 2)) > > causes uninitialized "save" to be copied. > > ---------- > [drm] Initialized vgem 1.0.0 20120112 for vgem on minor 0 > [drm] Initialized vkms 1.0.0 20180514 for vkms on minor 1 > Console: switching to colour frame buffer device 128x48 > ===================================================== > BUG: KMSAN: uninit-value in do_update_region+0x4b8/0xba0 > do_update_region+0x4b8/0xba0 > update_region+0x40d/0x840 > fbcon_switch+0x3364/0x35e0 > redraw_screen+0xae3/0x18a0 > do_bind_con_driver+0x1cb3/0x1df0 > do_take_over_console+0x11cb/0x13f0 > fbcon_fb_registered+0xacc/0xfd0 > register_framebuffer+0x1179/0x1320 > __drm_fb_helper_initial_config_and_unlock+0x23ad/0x2b40 > drm_fbdev_client_hotplug+0xbea/0xda0 > drm_fbdev_generic_setup+0x65e/0x9d0 > vkms_init+0x9f3/0xc76 > (...snipped...) > > Uninit was stored to memory at: > fbcon_prepare_logo+0x143b/0x1940 > fbcon_init+0x2c1b/0x31c0 > visual_init+0x3e7/0x820 > do_bind_con_driver+0x14a4/0x1df0 > do_take_over_console+0x11cb/0x13f0 > fbcon_fb_registered+0xacc/0xfd0 > register_framebuffer+0x1179/0x1320 > __drm_fb_helper_initial_config_and_unlock+0x23ad/0x2b40 > drm_fbdev_client_hotplug+0xbea/0xda0 > drm_fbdev_generic_setup+0x65e/0x9d0 > vkms_init+0x9f3/0xc76 > (...snipped...) > > Uninit was created at: > __kmem_cache_alloc_node+0xb69/0x1020 > __kmalloc+0x379/0x680 > fbcon_prepare_logo+0x704/0x1940 > fbcon_init+0x2c1b/0x31c0 > visual_init+0x3e7/0x820 > do_bind_con_driver+0x14a4/0x1df0 > do_take_over_console+0x11cb/0x13f0 > fbcon_fb_registered+0xacc/0xfd0 > register_framebuffer+0x1179/0x1320 > __drm_fb_helper_initial_config_and_unlock+0x23ad/0x2b40 > drm_fbdev_client_hotplug+0xbea/0xda0 > drm_fbdev_generic_setup+0x65e/0x9d0 > vkms_init+0x9f3/0xc76 > (...snipped...) > > CPU: 2 PID: 1 Comm: swapper/0 Not tainted 6.1.0-rc4-00356-g8f2975c2bb4c #924 > Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 > ---------- > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Thanks for your patch, pushed to drm-misc-fixes. -Daniel > --- > drivers/video/fbdev/core/fbcon.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > index 098b62f7b701..c0143d38df83 100644 > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > @@ -577,7 +577,7 @@ static void fbcon_prepare_logo(struct vc_data *vc, struct fb_info *info, > if (scr_readw(r) != vc->vc_video_erase_char) > break; > if (r != q && new_rows >= rows + logo_lines) { > - save = kmalloc(array3_size(logo_lines, new_cols, 2), > + save = kzalloc(array3_size(logo_lines, new_cols, 2), > GFP_KERNEL); > if (save) { > int i = min(cols, new_cols); > -- > 2.34.1
Hi Handa-san, On Thu, Nov 17, 2022 at 4:32 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > A kernel built with syzbot's config file reported that > > scr_memcpyw(q, save, array3_size(logo_lines, new_cols, 2)) > > causes uninitialized "save" to be copied. > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Thanks for your patch, which is now commit a6a00d7e8ffd78d1 ("fbcon: Use kzalloc() in fbcon_prepare_logo()") in v6.1-rc7, and which is being backported to stable. > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > @@ -577,7 +577,7 @@ static void fbcon_prepare_logo(struct vc_data *vc, struct fb_info *info, > if (scr_readw(r) != vc->vc_video_erase_char) > break; > if (r != q && new_rows >= rows + logo_lines) { > - save = kmalloc(array3_size(logo_lines, new_cols, 2), > + save = kzalloc(array3_size(logo_lines, new_cols, 2), > GFP_KERNEL); > if (save) { > int i = min(cols, new_cols); The next line is: scr_memsetw(save, erase, array3_size(logo_lines, new_cols, 2)); So how can this turn out to be uninitialized later below? scr_memcpyw(q, save, array3_size(logo_lines, new_cols, 2)); What am I missing? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 2022/12/15 18:36, Geert Uytterhoeven wrote: > The next line is: > > scr_memsetw(save, erase, array3_size(logo_lines, new_cols, 2)); > > So how can this turn out to be uninitialized later below? > > scr_memcpyw(q, save, array3_size(logo_lines, new_cols, 2)); > > What am I missing? Good catch. It turned out that this was a KMSAN problem (i.e. a false positive report). On x86_64, scr_memsetw() is implemented as static inline void scr_memsetw(u16 *s, u16 c, unsigned int count) { memset16(s, c, count / 2); } and memset16() is implemented as static inline void *memset16(uint16_t *s, uint16_t v, size_t n) { long d0, d1; asm volatile("rep\n\t" "stosw" : "=&c" (d0), "=&D" (d1) : "a" (v), "1" (s), "0" (n) : "memory"); return s; } . Plain memset() in arch/x86/include/asm/string_64.h is redirected to __msan_memset() but memsetXX() are not redirected to __msan_memsetXX(). That is, memory initialization via memsetXX() results in KMSAN's shadow memory being not updated. KMSAN folks, how should we fix this problem? Redirect assembly-implemented memset16(size) to memset(size*2) if KMSAN is enabled?
On Fri, Dec 16, 2022 at 3:03 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2022/12/15 18:36, Geert Uytterhoeven wrote: > > The next line is: > > > > scr_memsetw(save, erase, array3_size(logo_lines, new_cols, 2)); > > > > So how can this turn out to be uninitialized later below? > > > > scr_memcpyw(q, save, array3_size(logo_lines, new_cols, 2)); > > > > What am I missing? > > Good catch. It turned out that this was a KMSAN problem (i.e. a false positive report). > > On x86_64, scr_memsetw() is implemented as > > static inline void scr_memsetw(u16 *s, u16 c, unsigned int count) > { > memset16(s, c, count / 2); > } > > and memset16() is implemented as > > static inline void *memset16(uint16_t *s, uint16_t v, size_t n) > { > long d0, d1; > asm volatile("rep\n\t" > "stosw" > : "=&c" (d0), "=&D" (d1) > : "a" (v), "1" (s), "0" (n) > : "memory"); > return s; > } > > . Plain memset() in arch/x86/include/asm/string_64.h is redirected to __msan_memset() > but memsetXX() are not redirected to __msan_memsetXX(). That is, memory initialization > via memsetXX() results in KMSAN's shadow memory being not updated. > > KMSAN folks, how should we fix this problem? > Redirect assembly-implemented memset16(size) to memset(size*2) if KMSAN is enabled? > I think the easiest way to fix it would be disable memsetXX asm implementations by something like: ------------------------------------------------------------------------------------------------- diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h index 888731ccf1f67..5fb330150a7d1 100644 --- a/arch/x86/include/asm/string_64.h +++ b/arch/x86/include/asm/string_64.h @@ -33,6 +33,7 @@ void *memset(void *s, int c, size_t n); #endif void *__memset(void *s, int c, size_t n); +#if !defined(__SANITIZE_MEMORY__) #define __HAVE_ARCH_MEMSET16 static inline void *memset16(uint16_t *s, uint16_t v, size_t n) { @@ -68,6 +69,7 @@ static inline void *memset64(uint64_t *s, uint64_t v, size_t n) : "memory"); return s; } +#endif #define __HAVE_ARCH_MEMMOVE #if defined(__SANITIZE_MEMORY__) && defined(__NO_FORTIFY) ------------------------------------------------------------------------------------------------- This way we'll just pick the existing C implementations instead of reinventing them.
On Fri, Dec 16, 2022 at 04:52:14PM +0100, Alexander Potapenko wrote: > On Fri, Dec 16, 2022 at 3:03 PM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > > > On 2022/12/15 18:36, Geert Uytterhoeven wrote: > > > The next line is: > > > > > > scr_memsetw(save, erase, array3_size(logo_lines, new_cols, 2)); > > > > > > So how can this turn out to be uninitialized later below? > > > > > > scr_memcpyw(q, save, array3_size(logo_lines, new_cols, 2)); > > > > > > What am I missing? > > > > Good catch. It turned out that this was a KMSAN problem (i.e. a false positive report). > > > > On x86_64, scr_memsetw() is implemented as > > > > static inline void scr_memsetw(u16 *s, u16 c, unsigned int count) > > { > > memset16(s, c, count / 2); > > } > > > > and memset16() is implemented as > > > > static inline void *memset16(uint16_t *s, uint16_t v, size_t n) > > { > > long d0, d1; > > asm volatile("rep\n\t" > > "stosw" > > : "=&c" (d0), "=&D" (d1) > > : "a" (v), "1" (s), "0" (n) > > : "memory"); > > return s; > > } > > > > . Plain memset() in arch/x86/include/asm/string_64.h is redirected to __msan_memset() > > but memsetXX() are not redirected to __msan_memsetXX(). That is, memory initialization > > via memsetXX() results in KMSAN's shadow memory being not updated. > > > > KMSAN folks, how should we fix this problem? > > Redirect assembly-implemented memset16(size) to memset(size*2) if KMSAN is enabled? > > > > I think the easiest way to fix it would be disable memsetXX asm > implementations by something like: > > ------------------------------------------------------------------------------------------------- > diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h > index 888731ccf1f67..5fb330150a7d1 100644 > --- a/arch/x86/include/asm/string_64.h > +++ b/arch/x86/include/asm/string_64.h > @@ -33,6 +33,7 @@ void *memset(void *s, int c, size_t n); > #endif > void *__memset(void *s, int c, size_t n); > > +#if !defined(__SANITIZE_MEMORY__) > #define __HAVE_ARCH_MEMSET16 > static inline void *memset16(uint16_t *s, uint16_t v, size_t n) > { > @@ -68,6 +69,7 @@ static inline void *memset64(uint64_t *s, uint64_t > v, size_t n) > : "memory"); > return s; > } > +#endif So ... what should I do here? Can someone please send me a revert or patch to apply. I don't think I should do this, since I already tossed my credit for not looking at stuff carefully enough into the wind :-) -Daniel > > #define __HAVE_ARCH_MEMMOVE > #if defined(__SANITIZE_MEMORY__) && defined(__NO_FORTIFY) > ------------------------------------------------------------------------------------------------- > > This way we'll just pick the existing C implementations instead of > reinventing them. > > > -- > Alexander Potapenko > Software Engineer > > Google Germany GmbH > Erika-Mann-Straße, 33 > 80636 München > > Geschäftsführer: Paul Manicle, Liana Sebastian > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg
On 2023/01/05 20:54, Daniel Vetter wrote: >>> . Plain memset() in arch/x86/include/asm/string_64.h is redirected to __msan_memset() >>> but memsetXX() are not redirected to __msan_memsetXX(). That is, memory initialization >>> via memsetXX() results in KMSAN's shadow memory being not updated. >>> >>> KMSAN folks, how should we fix this problem? >>> Redirect assembly-implemented memset16(size) to memset(size*2) if KMSAN is enabled? >>> >> >> I think the easiest way to fix it would be disable memsetXX asm >> implementations by something like: >> >> ------------------------------------------------------------------------------------------------- >> diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h >> index 888731ccf1f67..5fb330150a7d1 100644 >> --- a/arch/x86/include/asm/string_64.h >> +++ b/arch/x86/include/asm/string_64.h >> @@ -33,6 +33,7 @@ void *memset(void *s, int c, size_t n); >> #endif >> void *__memset(void *s, int c, size_t n); >> >> +#if !defined(__SANITIZE_MEMORY__) >> #define __HAVE_ARCH_MEMSET16 >> static inline void *memset16(uint16_t *s, uint16_t v, size_t n) >> { >> @@ -68,6 +69,7 @@ static inline void *memset64(uint64_t *s, uint64_t >> v, size_t n) >> : "memory"); >> return s; >> } >> +#endif > > So ... what should I do here? Can someone please send me a revert or patch > to apply. I don't think I should do this, since I already tossed my credit > for not looking at stuff carefully enough into the wind :-) > -Daniel > >> >> #define __HAVE_ARCH_MEMMOVE >> #if defined(__SANITIZE_MEMORY__) && defined(__NO_FORTIFY) >> ------------------------------------------------------------------------------------------------- >> >> This way we'll just pick the existing C implementations instead of >> reinventing them. >> I'd like to avoid touching per-arch asm/string.h files if possible. Can't we do like below (i.e. keep asm implementations as-is, but automatically redirect to __msan_memset()) ? If yes, we could move all __msan_*() redirection from per-arch asm/string.h files to the common linux/string.h file? diff --git a/include/linux/string.h b/include/linux/string.h index c062c581a98b..403813b04e00 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -360,4 +360,15 @@ static __always_inline size_t str_has_prefix(const char *str, const char *prefix return strncmp(str, prefix, len) == 0 ? len : 0; } +#if defined(__SANITIZE_MEMORY__) && defined(__NO_FORTIFY) +#undef memset +#define memset(dest, src, count) __msan_memset((dest), (src), (count)) +#undef memset16 +#define memset16(dest, src, count) __msan_memset((dest), (src), (count) << 1) +#undef memset32 +#define memset32(dest, src, count) __msan_memset((dest), (src), (count) << 2) +#undef memset64 +#define memset64(dest, src, count) __msan_memset((dest), (src), (count) << 3) +#endif + #endif /* _LINUX_STRING_H_ */
On Thu, Jan 05, 2023 at 10:17:24PM +0900, Tetsuo Handa wrote: > On 2023/01/05 20:54, Daniel Vetter wrote: > >>> . Plain memset() in arch/x86/include/asm/string_64.h is redirected to __msan_memset() > >>> but memsetXX() are not redirected to __msan_memsetXX(). That is, memory initialization > >>> via memsetXX() results in KMSAN's shadow memory being not updated. > >>> > >>> KMSAN folks, how should we fix this problem? > >>> Redirect assembly-implemented memset16(size) to memset(size*2) if KMSAN is enabled? > >>> > >> > >> I think the easiest way to fix it would be disable memsetXX asm > >> implementations by something like: > >> > >> ------------------------------------------------------------------------------------------------- > >> diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h > >> index 888731ccf1f67..5fb330150a7d1 100644 > >> --- a/arch/x86/include/asm/string_64.h > >> +++ b/arch/x86/include/asm/string_64.h > >> @@ -33,6 +33,7 @@ void *memset(void *s, int c, size_t n); > >> #endif > >> void *__memset(void *s, int c, size_t n); > >> > >> +#if !defined(__SANITIZE_MEMORY__) > >> #define __HAVE_ARCH_MEMSET16 > >> static inline void *memset16(uint16_t *s, uint16_t v, size_t n) > >> { > >> @@ -68,6 +69,7 @@ static inline void *memset64(uint64_t *s, uint64_t > >> v, size_t n) > >> : "memory"); > >> return s; > >> } > >> +#endif > > > > So ... what should I do here? Can someone please send me a revert or patch > > to apply. I don't think I should do this, since I already tossed my credit > > for not looking at stuff carefully enough into the wind :-) > > -Daniel > > > >> > >> #define __HAVE_ARCH_MEMMOVE > >> #if defined(__SANITIZE_MEMORY__) && defined(__NO_FORTIFY) > >> ------------------------------------------------------------------------------------------------- > >> > >> This way we'll just pick the existing C implementations instead of > >> reinventing them. > >> > > I'd like to avoid touching per-arch asm/string.h files if possible. > > Can't we do like below (i.e. keep asm implementations as-is, but > automatically redirect to __msan_memset()) ? If yes, we could move all > __msan_*() redirection from per-arch asm/string.h files to the common > linux/string.h file? Oh I was more asking about the fbdev patch. This here sounds a lot more something that needs to be discussed with kmsan people, that's definitely not my area. -Daniel > > diff --git a/include/linux/string.h b/include/linux/string.h > index c062c581a98b..403813b04e00 100644 > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -360,4 +360,15 @@ static __always_inline size_t str_has_prefix(const char *str, const char *prefix > return strncmp(str, prefix, len) == 0 ? len : 0; > } > > +#if defined(__SANITIZE_MEMORY__) && defined(__NO_FORTIFY) > +#undef memset > +#define memset(dest, src, count) __msan_memset((dest), (src), (count)) > +#undef memset16 > +#define memset16(dest, src, count) __msan_memset((dest), (src), (count) << 1) > +#undef memset32 > +#define memset32(dest, src, count) __msan_memset((dest), (src), (count) << 2) > +#undef memset64 > +#define memset64(dest, src, count) __msan_memset((dest), (src), (count) << 3) > +#endif > + > #endif /* _LINUX_STRING_H_ */ > >
On 2023/01/05 22:22, Daniel Vetter wrote: > Oh I was more asking about the fbdev patch. This here sounds a lot more > something that needs to be discussed with kmsan people, that's definitely > not my area. > -Daniel Commit a6a00d7e8ffd ("fbcon: Use kzalloc() in fbcon_prepare_logo()") was redundant but not reverting that commit is harmless. You don't need to worry about this problem. This is a problem for KMSAN people.
> > I'd like to avoid touching per-arch asm/string.h files if possible. > > Can't we do like below (i.e. keep asm implementations as-is, but > automatically redirect to __msan_memset()) ? If yes, we could move all > __msan_*() redirection from per-arch asm/string.h files to the common > linux/string.h file? > > diff --git a/include/linux/string.h b/include/linux/string.h > index c062c581a98b..403813b04e00 100644 > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -360,4 +360,15 @@ static __always_inline size_t str_has_prefix(const char *str, const char *prefix > return strncmp(str, prefix, len) == 0 ? len : 0; > } > > +#if defined(__SANITIZE_MEMORY__) && defined(__NO_FORTIFY) > +#undef memset > +#define memset(dest, src, count) __msan_memset((dest), (src), (count)) > +#undef memset16 > +#define memset16(dest, src, count) __msan_memset((dest), (src), (count) << 1) > +#undef memset32 > +#define memset32(dest, src, count) __msan_memset((dest), (src), (count) << 2) > +#undef memset64 > +#define memset64(dest, src, count) __msan_memset((dest), (src), (count) << 3) > +#endif The problem with this approach is that it can only work for memset/memcpy/memmove, whereas any function that is implemented in lib/string.c may require undefining the respective __HAVE_ARCH_FNAME so that KMSAN can instrument it.
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 098b62f7b701..c0143d38df83 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -577,7 +577,7 @@ static void fbcon_prepare_logo(struct vc_data *vc, struct fb_info *info, if (scr_readw(r) != vc->vc_video_erase_char) break; if (r != q && new_rows >= rows + logo_lines) { - save = kmalloc(array3_size(logo_lines, new_cols, 2), + save = kzalloc(array3_size(logo_lines, new_cols, 2), GFP_KERNEL); if (save) { int i = min(cols, new_cols);
A kernel built with syzbot's config file reported that scr_memcpyw(q, save, array3_size(logo_lines, new_cols, 2)) causes uninitialized "save" to be copied. ---------- [drm] Initialized vgem 1.0.0 20120112 for vgem on minor 0 [drm] Initialized vkms 1.0.0 20180514 for vkms on minor 1 Console: switching to colour frame buffer device 128x48 ===================================================== BUG: KMSAN: uninit-value in do_update_region+0x4b8/0xba0 do_update_region+0x4b8/0xba0 update_region+0x40d/0x840 fbcon_switch+0x3364/0x35e0 redraw_screen+0xae3/0x18a0 do_bind_con_driver+0x1cb3/0x1df0 do_take_over_console+0x11cb/0x13f0 fbcon_fb_registered+0xacc/0xfd0 register_framebuffer+0x1179/0x1320 __drm_fb_helper_initial_config_and_unlock+0x23ad/0x2b40 drm_fbdev_client_hotplug+0xbea/0xda0 drm_fbdev_generic_setup+0x65e/0x9d0 vkms_init+0x9f3/0xc76 (...snipped...) Uninit was stored to memory at: fbcon_prepare_logo+0x143b/0x1940 fbcon_init+0x2c1b/0x31c0 visual_init+0x3e7/0x820 do_bind_con_driver+0x14a4/0x1df0 do_take_over_console+0x11cb/0x13f0 fbcon_fb_registered+0xacc/0xfd0 register_framebuffer+0x1179/0x1320 __drm_fb_helper_initial_config_and_unlock+0x23ad/0x2b40 drm_fbdev_client_hotplug+0xbea/0xda0 drm_fbdev_generic_setup+0x65e/0x9d0 vkms_init+0x9f3/0xc76 (...snipped...) Uninit was created at: __kmem_cache_alloc_node+0xb69/0x1020 __kmalloc+0x379/0x680 fbcon_prepare_logo+0x704/0x1940 fbcon_init+0x2c1b/0x31c0 visual_init+0x3e7/0x820 do_bind_con_driver+0x14a4/0x1df0 do_take_over_console+0x11cb/0x13f0 fbcon_fb_registered+0xacc/0xfd0 register_framebuffer+0x1179/0x1320 __drm_fb_helper_initial_config_and_unlock+0x23ad/0x2b40 drm_fbdev_client_hotplug+0xbea/0xda0 drm_fbdev_generic_setup+0x65e/0x9d0 vkms_init+0x9f3/0xc76 (...snipped...) CPU: 2 PID: 1 Comm: swapper/0 Not tainted 6.1.0-rc4-00356-g8f2975c2bb4c #924 Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 ---------- Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/video/fbdev/core/fbcon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)