| Message ID | 20190426145946.26537-1-ghalat@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vt/fbcon: deinitialize resources in visual_init() after failed memory allocation | expand |
On Fri, 26 Apr 2019 at 16:59, Grzegorz Halat <ghalat@redhat.com> wrote: > > After memory allocation failure vc_allocate() doesn't clean up data > which has been initialized in visual_init(). In case of fbcon this > leads to divide-by-0 in fbcon_init() on next open of the same tty. Hi, A gentle reminder. Could you please review my patch? I've seen two crashes caused by this bug. -- Grzegorz Halat
On 4/26/19 4:59 PM, Grzegorz Halat wrote: > After memory allocation failure vc_allocate() doesn't clean up data > which has been initialized in visual_init(). In case of fbcon this > leads to divide-by-0 in fbcon_init() on next open of the same tty. > > memory allocation in vc_allocate() may fail here: > 1097: vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL); > > on next open() fbcon_init() skips vc_font.data initialization: > 1088: if (!p->fontdata) { > > division by zero in fbcon_init() happens here: > 1149: new_cols /= vc->vc_font.width; > > Additional check is needed in fbcon_deinit() to prevent > usage of uninitialized vc_screenbuf: > > 1251: if (vc->vc_hi_font_mask && vc->vc_screenbuf) > 1252: set_vc_hi_font(vc, false); > > Crash: > > #6 [ffffc90001eafa60] divide_error at ffffffff81a00be4 > [exception RIP: fbcon_init+463] > RIP: ffffffff814b860f RSP: ffffc90001eafb18 RFLAGS: 00010246 > ... > #7 [ffffc90001eafb60] visual_init at ffffffff8154c36e > #8 [ffffc90001eafb80] vc_allocate at ffffffff8154f53c > #9 [ffffc90001eafbc8] con_install at ffffffff8154f624 > ... > > Signed-off-by: Grzegorz Halat <ghalat@redhat.com> Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On Thu, May 16, 2019 at 04:33:40PM +0200, Grzegorz Halat wrote: > On Fri, 26 Apr 2019 at 16:59, Grzegorz Halat <ghalat@redhat.com> wrote: > > > > After memory allocation failure vc_allocate() doesn't clean up data > > which has been initialized in visual_init(). In case of fbcon this > > leads to divide-by-0 in fbcon_init() on next open of the same tty. > > Hi, > A gentle reminder. Could you please review my patch? I've seen two > crashes caused by this bug. How? How are you triggering a memory allocation failure in a "normal" system? Anyway, I'll queue this up, but it really does not seem like anything anyone would see "in the wild". thanks, greg k-h
On Fri, 24 May 2019 at 10:06, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > How? How are you triggering a memory allocation failure in a "normal" > system? > Anyway, I'll queue this up, but it really does not seem like anything > anyone would see "in the wild" I've seen this crash twice in ours customer environment under low memory conditions. There is a report in Debian bug tracker: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=804443 and LKML bug report: https://lkml.org/lkml/2017/12/18/591 -- Grzegorz
On Fri, May 24, 2019 at 03:52:31PM +0200, Grzegorz Halat wrote: > On Fri, 24 May 2019 at 10:06, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > How? How are you triggering a memory allocation failure in a "normal" > > system? > > Anyway, I'll queue this up, but it really does not seem like anything > > anyone would see "in the wild" > > I've seen this crash twice in ours customer environment under low > memory conditions. > There is a report in Debian bug tracker: > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=804443 > and LKML bug report: > https://lkml.org/lkml/2017/12/18/591 Ok, now queued up to go to Linus for 5.2-final. thanks, greg k-h
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c index 650c66886c80..ec85d195678f 100644 --- a/drivers/tty/vt/vt.c +++ b/drivers/tty/vt/vt.c @@ -1056,6 +1056,13 @@ static void visual_init(struct vc_data *vc, int num, int init) vc->vc_screenbuf_size = vc->vc_rows * vc->vc_size_row; } + +static void visual_deinit(struct vc_data *vc) +{ + vc->vc_sw->con_deinit(vc); + module_put(vc->vc_sw->owner); +} + int vc_allocate(unsigned int currcons) /* return 0 on success */ { struct vt_notifier_param param; @@ -1103,6 +1110,7 @@ int vc_allocate(unsigned int currcons) /* return 0 on success */ return 0; err_free: + visual_deinit(vc); kfree(vc); vc_cons[currcons].d = NULL; return -ENOMEM; @@ -1331,9 +1339,8 @@ struct vc_data *vc_deallocate(unsigned int currcons) param.vc = vc = vc_cons[currcons].d; atomic_notifier_call_chain(&vt_notifier_list, VT_DEALLOCATE, ¶m); vcs_remove_sysfs(currcons); - vc->vc_sw->con_deinit(vc); + visual_deinit(vc); put_pid(vc->vt_pid); - module_put(vc->vc_sw->owner); vc_uniscr_set(vc, NULL); kfree(vc->vc_screenbuf); vc_cons[currcons].d = NULL; diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index cd059a801662..c59b23f6e9ba 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -1248,7 +1248,7 @@ static void fbcon_deinit(struct vc_data *vc) if (free_font) vc->vc_font.data = NULL; - if (vc->vc_hi_font_mask) + if (vc->vc_hi_font_mask && vc->vc_screenbuf) set_vc_hi_font(vc, false); if (!con_is_bound(&fb_con))
After memory allocation failure vc_allocate() doesn't clean up data which has been initialized in visual_init(). In case of fbcon this leads to divide-by-0 in fbcon_init() on next open of the same tty. memory allocation in vc_allocate() may fail here: 1097: vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL); on next open() fbcon_init() skips vc_font.data initialization: 1088: if (!p->fontdata) { division by zero in fbcon_init() happens here: 1149: new_cols /= vc->vc_font.width; Additional check is needed in fbcon_deinit() to prevent usage of uninitialized vc_screenbuf: 1251: if (vc->vc_hi_font_mask && vc->vc_screenbuf) 1252: set_vc_hi_font(vc, false); Crash: #6 [ffffc90001eafa60] divide_error at ffffffff81a00be4 [exception RIP: fbcon_init+463] RIP: ffffffff814b860f RSP: ffffc90001eafb18 RFLAGS: 00010246 ... #7 [ffffc90001eafb60] visual_init at ffffffff8154c36e #8 [ffffc90001eafb80] vc_allocate at ffffffff8154f53c #9 [ffffc90001eafbc8] con_install at ffffffff8154f624 ... Signed-off-by: Grzegorz Halat <ghalat@redhat.com> --- drivers/tty/vt/vt.c | 11 +++++++++-- drivers/video/fbdev/core/fbcon.c | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-)