On 05. 11. 19, 10:03, Or Cohen wrote:
> @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 )
Because unicode uses 4 bytes. The issue is that there is no handling for
unicode in vcs_write at all. (Compare with vcs_read.)
thanks,
--
js
suse labs
On Tue, 5 Nov 2019, Jiri Slaby wrote:
> Because unicode uses 4 bytes. The issue is that there is no handling for
> unicode in vcs_write at all. (Compare with vcs_read.)
Exact.
----- >8
Subject: [PATCH] vcs: prevent write access to vcsu devices
Commit d21b0be246bf ("vt: introduce unicode mode for /dev/vcs") guarded
against using devices containing attributes as this is not yet
implemented. It however failed to guard against writes to any devices
as this is also unimplemented.
Signed-off-by: Nicolas Pitre <[email protected]>
Cc: <[email protected]> # v4.19+
diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index fa07d79027..ef19b95b73 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -456,6 +456,9 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
size_t ret;
char *con_buf;
+ if (use_unicode(inode))
+ return -EOPNOTSUPP;
+
con_buf = (char *) __get_free_page(GFP_KERNEL);
if (!con_buf)
return -ENOMEM;
On 05. 11. 19, 10:33, Nicolas Pitre wrote:
> Subject: [PATCH] vcs: prevent write access to vcsu devices
>
> Commit d21b0be246bf ("vt: introduce unicode mode for /dev/vcs") guarded
> against using devices containing attributes as this is not yet
> implemented. It however failed to guard against writes to any devices
> as this is also unimplemented.
>
> Signed-off-by: Nicolas Pitre <[email protected]>
> Cc: <[email protected]> # v4.19+
>
> diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
> index fa07d79027..ef19b95b73 100644
> --- a/drivers/tty/vt/vc_screen.c
> +++ b/drivers/tty/vt/vc_screen.c
> @@ -456,6 +456,9 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> size_t ret;
> char *con_buf;
>
> + if (use_unicode(inode))
> + return -EOPNOTSUPP;
Looks good to me. I am also thinking about a ban directly in open:
if (use_unicode(inode) && (filp->f_flags & O_ACCMODE) != O_RDONLY)
return -EOPNOTSUPP;
Would that break the unicode users?
thanks,
--
js
suse labs
On Tue, Nov 5, 2019 at 11:29 AM Jiri Slaby <[email protected]> wrote:
>
> On 05. 11. 19, 10:33, Nicolas Pitre wrote:
> > Subject: [PATCH] vcs: prevent write access to vcsu devices
> >
> > Commit d21b0be246bf ("vt: introduce unicode mode for /dev/vcs") guarded
> > against using devices containing attributes as this is not yet
> > implemented. It however failed to guard against writes to any devices
> > as this is also unimplemented.
> >
> > Signed-off-by: Nicolas Pitre <[email protected]>
> > Cc: <[email protected]> # v4.19+
> >
> > diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
> > index fa07d79027..ef19b95b73 100644
> > --- a/drivers/tty/vt/vc_screen.c
> > +++ b/drivers/tty/vt/vc_screen.c
> > @@ -456,6 +456,9 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> > size_t ret;
> > char *con_buf;
> >
> > + if (use_unicode(inode))
> > + return -EOPNOTSUPP;
>
> Looks good to me. I am also thinking about a ban directly in open:
>
> if (use_unicode(inode) && (filp->f_flags & O_ACCMODE) != O_RDONLY)
> return -EOPNOTSUPP;
>
> Would that break the unicode users?
On a related note, syzbot seems to get very similar bug reports on
some downstream kernels (4.15):
KASAN: use-after-free Read in vcs_scr_readw
KASAN: use-after-free Write in vcs_scr_writew
but not on upstream. I wonder why. And if we are missing some good
config in upstream kernel or something. This all fuzzing is somewhat
random, so it might have just happened without particular reasons
(maybe it will discover it later). But wanted to check if there are
some low hanging fruits. Anything obviously missing in:
https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-kasan.config
?
On Tue, 5 Nov 2019, Jiri Slaby wrote:
> On 05. 11. 19, 10:33, Nicolas Pitre wrote:
> > Subject: [PATCH] vcs: prevent write access to vcsu devices
> >
> > Commit d21b0be246bf ("vt: introduce unicode mode for /dev/vcs") guarded
> > against using devices containing attributes as this is not yet
> > implemented. It however failed to guard against writes to any devices
> > as this is also unimplemented.
> >
> > Signed-off-by: Nicolas Pitre <[email protected]>
> > Cc: <[email protected]> # v4.19+
> >
> > diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
> > index fa07d79027..ef19b95b73 100644
> > --- a/drivers/tty/vt/vc_screen.c
> > +++ b/drivers/tty/vt/vc_screen.c
> > @@ -456,6 +456,9 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> > size_t ret;
> > char *con_buf;
> >
> > + if (use_unicode(inode))
> > + return -EOPNOTSUPP;
>
> Looks good to me. I am also thinking about a ban directly in open:
>
> if (use_unicode(inode) && (filp->f_flags & O_ACCMODE) != O_RDONLY)
> return -EOPNOTSUPP;
>
> Would that break the unicode users?
The user I know about uses a common helper that uses O_RDWR.
So yes, in that case that would break it.
Nicolas
@[email protected] - Regarding your question, the .config file I used
when fuzzing is attached.
I compared it and I don't think you are missing some good config in upstream.
I created my .config file just with "make defconfig make kvmconfig"
and added some features useful for debugging..
On Tue, Nov 5, 2019 at 3:44 PM Nicolas Pitre <[email protected]> wrote:
>
> On Tue, 5 Nov 2019, Jiri Slaby wrote:
>
> > On 05. 11. 19, 10:33, Nicolas Pitre wrote:
> > > Subject: [PATCH] vcs: prevent write access to vcsu devices
> > >
> > > Commit d21b0be246bf ("vt: introduce unicode mode for /dev/vcs") guarded
> > > against using devices containing attributes as this is not yet
> > > implemented. It however failed to guard against writes to any devices
> > > as this is also unimplemented.
> > >
> > > Signed-off-by: Nicolas Pitre <[email protected]>
> > > Cc: <[email protected]> # v4.19+
> > >
> > > diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
> > > index fa07d79027..ef19b95b73 100644
> > > --- a/drivers/tty/vt/vc_screen.c
> > > +++ b/drivers/tty/vt/vc_screen.c
> > > @@ -456,6 +456,9 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> > > size_t ret;
> > > char *con_buf;
> > >
> > > + if (use_unicode(inode))
> > > + return -EOPNOTSUPP;
> >
> > Looks good to me. I am also thinking about a ban directly in open:
> >
> > if (use_unicode(inode) && (filp->f_flags & O_ACCMODE) != O_RDONLY)
> > return -EOPNOTSUPP;
> >
> > Would that break the unicode users?
>
> The user I know about uses a common helper that uses O_RDWR.
> So yes, in that case that would break it.
>
>
> Nicolas
Greg, could you apply this please?
On Tue, 5 Nov 2019, Nicolas Pitre wrote:
> Subject: [PATCH] vcs: prevent write access to vcsu devices
>
> Commit d21b0be246bf ("vt: introduce unicode mode for /dev/vcs") guarded
> against using devices containing attributes as this is not yet
> implemented. It however failed to guard against writes to any devices
> as this is also unimplemented.
>
> Signed-off-by: Nicolas Pitre <[email protected]>
> Cc: <[email protected]> # v4.19+
>
> diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
> index fa07d79027..ef19b95b73 100644
> --- a/drivers/tty/vt/vc_screen.c
> +++ b/drivers/tty/vt/vc_screen.c
> @@ -456,6 +456,9 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> size_t ret;
> char *con_buf;
>
> + if (use_unicode(inode))
> + return -EOPNOTSUPP;
> +
> con_buf = (char *) __get_free_page(GFP_KERNEL);
> if (!con_buf)
> return -ENOMEM;
>
>
On Tue, Nov 26, 2019 at 04:55:32PM -0500, Nicolas Pitre wrote:
> Greg, could you apply this please?
Yes, I will, but I don't seem to have it in my email archives anywhere,
was it burried in that long thread?
thanks,
greg k-h
On Wed, 27 Nov 2019, Greg KH wrote:
> On Tue, Nov 26, 2019 at 04:55:32PM -0500, Nicolas Pitre wrote:
> > Greg, could you apply this please?
>
> Yes, I will, but I don't seem to have it in my email archives anywhere,
> was it burried in that long thread?
I've seen much longer threads than this one, but yes it was at its tail.
Hence the direct ping.
Nicolas
On Wed, Nov 27, 2019 at 11:24:52AM -0500, Nicolas Pitre wrote:
> On Wed, 27 Nov 2019, Greg KH wrote:
>
> > On Tue, Nov 26, 2019 at 04:55:32PM -0500, Nicolas Pitre wrote:
> > > Greg, could you apply this please?
> >
> > Yes, I will, but I don't seem to have it in my email archives anywhere,
> > was it burried in that long thread?
>
> I've seen much longer threads than this one, but yes it was at its tail.
> Hence the direct ping.
Yeah, I dug it out, sorry about that, my fault. All now queued up.
greg k-h