On Mon, Nov 04, 2019 at 04:39:55AM -0800, Or Cohen wrote:
> Hi,
> I discovered a OOB access bug using Syzkaller and decided to report it,
> as I could not find a similar report in syzkaller mailing list,
> syzkaller-bugs mailing list
> or syzbot dashboard. ( as described in:
> https://github.com/google/syzkaller/blob/master/docs/linux/reporting_kernel_bugs.md
> )
>
> I've tested it and the bug reproduces on the following versions:
>
> commit 4d856f72c10ecb060868ed10ff1b1453943fc6c8 (HEAD, tag: v5.3)
> Author: Linus Torvalds <[email protected]>
> Date: Sun Sep 15 14:19:32 2019 -0700
> Linux 5.3
>
> commit 84df9525b0c27f3ebc2ebb1864fa62a97fdedb7d (grafted, HEAD, tag: v4.19)
> Author: Greg Kroah-Hartman <[email protected]>
> Date: Mon Oct 22 07:37:37 2018 +0100
> Linux 4.19
>
> The call stack at the time of the crash is as follows: (kernel 5.3)
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x88/0xce lib/dump_stack.c:113
> print_address_description+0x60/0x31f mm/kasan/report.c:351
> __kasan_report.cold.6+0x1a/0x3c mm/kasan/report.c:482
> kasan_report+0xe/0x12 mm/kasan/common.c:618
> vcs_scr_readw+0xb1/0xc0 drivers/tty/vt/vt.c:4665
> vcs_write+0x5c2/0xbb0 drivers/tty/vt/vc_screen.c:545
> do_loop_readv_writev fs/read_write.c:717 [inline]
> do_loop_readv_writev fs/read_write.c:701 [inline]
> do_iter_write fs/read_write.c:972 [inline]
> do_iter_write+0x47b/0x5f0 fs/read_write.c:951
> vfs_writev+0x1b3/0x2f0 fs/read_write.c:1015
> do_writev+0x11f/0x2e0 fs/read_write.c:1058
> do_syscall_64+0xb7/0x3a0 arch/x86/entry/common.c:296
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
>
> The overwritten buffer is of size 4000 bytes and is allocated in:
> vc_allocate+0x3b0/0x6d0 drivers/tty/vt/vt.c:1098
>
> >From my brief analysis of the "vcs_write" function ( vc_screen.c ), it
> seems like the
> "screen_pos" function (line 548) is used to return a pointer into the
> overwritten buffer:
>
> 540: while (this_round > 0) {
> 541: unsigned char c = *con_buf0++;
> 542:
> 543: this_round--;
> 544: vcs_scr_writew(vc,
> 545: vcs_scr_readw(vc, org) & 0xff00) | c, org);
> 546: org++;
> 547: if (++col == maxcol) {
> 548: org = screen_pos(vc, p, viewed);
> 549: col = 0;
> 550: p += maxcol;
> 551: }
> 552: }
>
> The "count" argument ( controlled from user space ) affects the
> initialization
> of "this_round".
> After several iterations of the while loop, "screen_pos" returns a pointer
> to offset
> 4000 into the buffer which leads KASAN to detect it as an OOB read of 2
> bytes in "vcs_scr_readw".
> We can see however, that immediately after that "vcs_scr_writew" is called
> which
> leads to a write of 2 bytes past the end of the buffer.
>
> The following files are attached:
> repro.c - A C reproducer for the bug.
Your repo.c file is "interesting" saying you have a giant buffer, yet it
really is just 1 byte long. Does that have something to do with the
problem here?
I am at another conference at the moment and can't look at this much
now, will try to later this week...
thanks,
greg k-h
On Mon, 4 Nov 2019, Greg KH wrote:
> On Mon, Nov 04, 2019 at 04:39:55AM -0800, Or Cohen wrote:
> > Hi,
> > I discovered a OOB access bug using Syzkaller and decided to report it,
> > as I could not find a similar report in syzkaller mailing list,
> > syzkaller-bugs mailing list
[...]
>
> I am at another conference at the moment and can't look at this much
> now, will try to later this week...
I'll looking into it now.
Nicolas
@[email protected] @[email protected] - Thanks for the quick response.
@[email protected] - Regarding your question, I don't think
the 1 byte buffer is related to the problem. ( it's just was there in
the initial reproducer the fuzzer created, and I forgot to remove it
while reducing code from the reproducer ).
I think the problem is related to the huge size argument , which
influences the initialization of "this_round".
On Mon, Nov 4, 2019 at 7:50 AM Nicolas Pitre <[email protected]> wrote:
>
> On Mon, 4 Nov 2019, Greg KH wrote:
>
> > On Mon, Nov 04, 2019 at 04:39:55AM -0800, Or Cohen wrote:
> > > Hi,
> > > I discovered a OOB access bug using Syzkaller and decided to report it,
> > > as I could not find a similar report in syzkaller mailing list,
> > > syzkaller-bugs mailing list
> [...]
> >
> > I am at another conference at the moment and can't look at this much
> > now, will try to later this week...
>
> I'll looking into it now.
>
>
> Nicolas
On Mon, 4 Nov 2019, Or Cohen wrote:
> @[email protected] @[email protected] - Thanks for the quick response.
> @[email protected] - Regarding your question, I don't think
> the 1 byte buffer is related to the problem. ( it's just was there in
> the initial reproducer the fuzzer created, and I forgot to remove it
> while reducing code from the reproducer ).
I think I know what the problem is. I have no time to test it though.
Please try this (untested) patch. Also please try running the same test
code but with vcsa6 in addition to vcsu6 to be sure.
---------- >8
Subject: [PATCH] vcs: add missing validation on vcs_size() returned value
One usage instance didn't account for the fact that vcs_size() may
return a negative error code.
Signed-off-by: Nicolas Pitre <[email protected]>
diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index 1f042346e7..fa07d79027 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -474,6 +474,10 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
goto unlock_out;
size = vcs_size(inode);
+ if (size < 0) {
+ ret = size;
+ goto unlock_out;
+ }
ret = -EINVAL;
if (pos < 0 || pos > size)
goto unlock_out;
On 04. 11. 19, 19:33, Nicolas Pitre wrote:
> On Mon, 4 Nov 2019, Or Cohen wrote:
>
>> @[email protected] @[email protected] - Thanks for the quick response.
>> @[email protected] - Regarding your question, I don't think
>> the 1 byte buffer is related to the problem. ( it's just was there in
>> the initial reproducer the fuzzer created, and I forgot to remove it
>> while reducing code from the reproducer ).
>
> I think I know what the problem is. I have no time to test it though.
>
> Please try this (untested) patch. Also please try running the same test
> code but with vcsa6 in addition to vcsu6 to be sure.
>
> ---------- >8
> Subject: [PATCH] vcs: add missing validation on vcs_size() returned value
>
> One usage instance didn't account for the fact that vcs_size() may
> return a negative error code.
>
> Signed-off-by: Nicolas Pitre <[email protected]>
>
> diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
> index 1f042346e7..fa07d79027 100644
> --- a/drivers/tty/vt/vc_screen.c
> +++ b/drivers/tty/vt/vc_screen.c
> @@ -474,6 +474,10 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> goto unlock_out;
>
> size = vcs_size(inode);
> + if (size < 0) {
> + ret = size;
> + goto unlock_out;
> + }
> ret = -EINVAL;
> if (pos < 0 || pos > size)
> goto unlock_out;
pos must be >= 0, so "pos > size" would catch this case as a side
effect, or am I missing something? That being said, the patch is
correct, but won't fix the issue IMO.
thanks,
--
js
suse labs
@Nicolas Pitre - I agree with you, "vcs_size" may return a negative
error code, so the patch is correct but as @[email protected] said it
won't fix the issue.
In my debugging session, "vcs_size" returns a positive integer ( 8000
decimal ) and the bug still triggers.
Maybe it's related to the following logic in "vcs_size"? ( not sure
about that.. )
221 if (use_attributes(inode)) {
222 if (use_unicode(inode))
223 return -EOPNOTSUPP;
224 size = 2*size + HEADER_SIZE;
225 } else if (use_unicode(inode))
226 size *= 4;
227 return size;
Why in the case of "use_unicode(inode)" size is multiplied by 4 and
not 2? ( as we can see in line 224 )
On Mon, Nov 4, 2019 at 10:54 PM Jiri Slaby <[email protected]> wrote:
>
> On 04. 11. 19, 19:33, Nicolas Pitre wrote:
> > On Mon, 4 Nov 2019, Or Cohen wrote:
> >
> >> @[email protected] @[email protected] - Thanks for the quick response.
> >> @[email protected] - Regarding your question, I don't think
> >> the 1 byte buffer is related to the problem. ( it's just was there in
> >> the initial reproducer the fuzzer created, and I forgot to remove it
> >> while reducing code from the reproducer ).
> >
> > I think I know what the problem is. I have no time to test it though.
> >
> > Please try this (untested) patch. Also please try running the same test
> > code but with vcsa6 in addition to vcsu6 to be sure.
> >
> > ---------- >8
> > Subject: [PATCH] vcs: add missing validation on vcs_size() returned value
> >
> > One usage instance didn't account for the fact that vcs_size() may
> > return a negative error code.
> >
> > Signed-off-by: Nicolas Pitre <[email protected]>
> >
> > diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
> > index 1f042346e7..fa07d79027 100644
> > --- a/drivers/tty/vt/vc_screen.c
> > +++ b/drivers/tty/vt/vc_screen.c
> > @@ -474,6 +474,10 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> > goto unlock_out;
> >
> > size = vcs_size(inode);
> > + if (size < 0) {
> > + ret = size;
> > + goto unlock_out;
> > + }
> > ret = -EINVAL;
> > if (pos < 0 || pos > size)
> > goto unlock_out;
>
> pos must be >= 0, so "pos > size" would catch this case as a side
> effect, or am I missing something? That being said, the patch is
> correct, but won't fix the issue IMO.
>
> thanks,
> --
> js
> suse labs