[PATCH] vt: Handle recursion in vc_do_resize().

* [PATCH] vt: Handle recursion in vc_do_resize().
@ 2020-07-29  5:30 Tetsuo Handa
  2020-07-29  8:18 ` Daniel Vetter
  0 siblings, 1 reply; 8+ messages in thread
From: Tetsuo Handa @ 2020-07-29  5:30 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: dri-devel, linux-fbdev, linux-kernel, Bartlomiej Zolnierkiewicz,
	Tetsuo Handa, syzbot

syzbot is reporting OOB read bug in vc_do_resize() [1] caused by memcpy()
based on outdated old_{rows,row_size} values, for resize_screen() can
recurse into vc_do_resize() which changes vc->vc_{cols,rows} that outdates
old_{rows,row_size} values which were read before calling resize_screen().

Minimal fix might be to read vc->vc_{rows,size_row} after resize_screen().
A different fix might be to forbid recursive vc_do_resize() request.
I can't tell which fix is the better.

But since I guess that new_cols == vc->vc_cols && new_rows == vc->vc_rows
check could become true after returning from resize_screen(), and I assume
that not calling clear_selection() when resize_screen() will return error
is harmless, let's redo the check by moving resize_screen() earlier.

[1] https://syzkaller.appspot.com/bug?id=c70c88cfd16dcf6e1d3c7f0ab8648b3144b5b25e

Reported-by: syzbot <syzbot+c37a14770d51a085a520@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/tty/vt/vt.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 42d8c67..952a067 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1217,7 +1217,24 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
 
 	if (new_cols == vc->vc_cols && new_rows == vc->vc_rows)
 		return 0;
+	if (new_screen_size > KMALLOC_MAX_SIZE || !new_screen_size)
+		return -EINVAL;
 
+	/*
+	 * Since fbcon_resize() from resize_screen() can recurse into
+	 * this function via fb_set_var(), handle recursion now.
+	 */
+	err = resize_screen(vc, new_cols, new_rows, user);
+	if (err)
+		return err;
+	/* Reload values in case recursion changed vc->vc_{cols,rows}. */
+	new_cols = (cols ? cols : vc->vc_cols);
+	new_rows = (lines ? lines : vc->vc_rows);
+	new_row_size = new_cols << 1;
+	new_screen_size = new_row_size * new_rows;
+
+	if (new_cols == vc->vc_cols && new_rows == vc->vc_rows)
+		return 0;
 	if (new_screen_size > KMALLOC_MAX_SIZE || !new_screen_size)
 		return -EINVAL;
 	newscreen = kzalloc(new_screen_size, GFP_USER);
@@ -1238,13 +1255,6 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
 	old_rows = vc->vc_rows;
 	old_row_size = vc->vc_size_row;
 
-	err = resize_screen(vc, new_cols, new_rows, user);
-	if (err) {
-		kfree(newscreen);
-		vc_uniscr_free(new_uniscr);
-		return err;
-	}
-
 	vc->vc_rows = new_rows;
 	vc->vc_cols = new_cols;
 	vc->vc_size_row = new_row_size;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread