The attribute header handling is terrible in vcs_read_buf. Separate it
to a new function and simply do memmove (of up to 4 bytes) to the start
of the con_buf -- if user seeked.
Signed-off-by: Jiri Slaby <[email protected]>
---
drivers/tty/vt/vc_screen.c | 31 ++++++++++++++++++-------------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index c178a1611223..9d68c6b36ddf 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -297,6 +297,22 @@ static void vcs_read_buf_noattr(const struct vc_data *vc, char *con_buf,
}
}
+static unsigned int vcs_read_buf_header(const struct vc_data *vc, char *con_buf,
+ unsigned int pos, unsigned int count)
+{
+ count = min(HEADER_SIZE - pos, count);
+
+ /* clamp header values if they don't fit */
+ con_buf[0] = min(vc->vc_rows, 0xFFu);
+ con_buf[1] = min(vc->vc_cols, 0xFFu);
+ getconsxy(vc, con_buf + 2);
+
+ if (pos)
+ memmove(con_buf, con_buf + pos, count);
+
+ return count;
+}
+
static unsigned int vcs_read_buf(const struct vc_data *vc, char *con_buf,
unsigned int pos, unsigned int count, bool viewed,
unsigned int *skip)
@@ -306,22 +322,11 @@ static unsigned int vcs_read_buf(const struct vc_data *vc, char *con_buf,
unsigned int filled = count;
if (pos < HEADER_SIZE) {
- /* clamp header values if they don't fit */
- con_buf[0] = min(vc->vc_rows, 0xFFu);
- con_buf[1] = min(vc->vc_cols, 0xFFu);
- getconsxy(vc, con_buf + 2);
-
- *skip += pos;
- count += pos;
- if (count > CON_BUF_SIZE) {
- count = CON_BUF_SIZE;
- filled = count - pos;
- }
+ count -= vcs_read_buf_header(vc, con_buf, pos, count);
- /* Advance state pointers and move on. */
- count -= min(HEADER_SIZE, count);
pos = HEADER_SIZE;
con_buf += HEADER_SIZE;
+
/* If count >= 0, then pos is even... */
} else if (pos & 1) {
/*
--
2.28.0
Hi all,
Link: https://syzkaller.appspot.com/bug?id=f332576321998d36cd07d09c9c1268cfed1895c9
As reported by syzbot, vcs_read_buf() is overflowing `con_buf16`, since
this patch removed the following check:
- if (count > CON_BUF_SIZE) {
- count = CON_BUF_SIZE;
- filled = count - pos;
- }
Decreasing `count` by `min(HEADER_SIZE - pos, count)` bypasses this check.
Additionally, this patch also removed updates to `skip` and `filled`.
What should we do in order to fix it?
Thank you,
Peilin Ye
On Tue, Aug 25, 2020 at 12:48:04PM -0400, Peilin Ye wrote:
> Hi all,
>
> Link: https://syzkaller.appspot.com/bug?id=f332576321998d36cd07d09c9c1268cfed1895c9
>
> As reported by syzbot, vcs_read_buf() is overflowing `con_buf16`, since
> this patch removed the following check:
>
> - if (count > CON_BUF_SIZE) {
> - count = CON_BUF_SIZE;
> - filled = count - pos;
> - }
>
> Decreasing `count` by `min(HEADER_SIZE - pos, count)` bypasses this check.
> Additionally, this patch also removed updates to `skip` and `filled`.
>
> What should we do in order to fix it?
This patch is already reverted, and it has been discussed a bit as to
how to do this properly if you look at the email where this was reported
to us.
thanks,
greg k-h
On Tue, Aug 25, 2020 at 06:59:35PM +0200, Greg KH wrote:
> On Tue, Aug 25, 2020 at 12:48:04PM -0400, Peilin Ye wrote:
> > Hi all,
> >
> > Link: https://syzkaller.appspot.com/bug?id=f332576321998d36cd07d09c9c1268cfed1895c9
> >
> > As reported by syzbot, vcs_read_buf() is overflowing `con_buf16`, since
> > this patch removed the following check:
> >
> > - if (count > CON_BUF_SIZE) {
> > - count = CON_BUF_SIZE;
> > - filled = count - pos;
> > - }
> >
> > Decreasing `count` by `min(HEADER_SIZE - pos, count)` bypasses this check.
> > Additionally, this patch also removed updates to `skip` and `filled`.
> >
> > What should we do in order to fix it?
>
> This patch is already reverted, and it has been discussed a bit as to
> how to do this properly if you look at the email where this was reported
> to us.
Ah, syzbot has reported this issue as two different bugs...I started
looking into it without noticing your discussion under another thread.
Sorry,
Peilin Ye