KMSAN: kernel-infoleak in tty_compat_ioctl

KMSAN: kernel-infoleak in tty_compat_ioctl

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:    8bbbc5cf kmsan: don't compile memmove
git tree:       https://github.com/google/kmsan.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=1280ea7ee00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=cd0e9a6b0e555cc3
dashboard link: https://syzkaller.appspot.com/bug?extid=8da9175e28eadcb203ce
compiler:       clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
userspace arch: i386
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1722a3d9e00000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1071e265e00000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+8da9175e28eadcb203ce@syzkaller.appspotmail.com

=====================================================
BUG: KMSAN: kernel-infoleak in kmsan_copy_to_user+0x81/0x90 mm/kmsan/kmsan_hooks.c:253
CPU: 1 PID: 11476 Comm: syz-executor814 Not tainted 5.6.0-rc2-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x1c9/0x220 lib/dump_stack.c:118
 kmsan_report+0xf7/0x1e0 mm/kmsan/kmsan_report.c:118
 kmsan_internal_check_memory+0x238/0x3d0 mm/kmsan/kmsan.c:423
 kmsan_copy_to_user+0x81/0x90 mm/kmsan/kmsan_hooks.c:253
 _copy_to_user+0x15a/0x1f0 lib/usercopy.c:33
 copy_to_user include/linux/uaccess.h:174 [inline]
 compat_tty_tiocgserial drivers/tty/tty_io.c:2748 [inline]
 tty_compat_ioctl+0x1482/0x1850 drivers/tty/tty_io.c:2846
 __do_compat_sys_ioctl fs/ioctl.c:857 [inline]
 __se_compat_sys_ioctl+0x57c/0xed0 fs/ioctl.c:808
 __ia32_compat_sys_ioctl+0xd9/0x110 fs/ioctl.c:808
 do_syscall_32_irqs_on arch/x86/entry/common.c:339 [inline]
 do_fast_syscall_32+0x3c7/0x6e0 arch/x86/entry/common.c:410
 entry_SYSENTER_compat+0x68/0x77 arch/x86/entry/entry_64_compat.S:139
RIP: 0023:0xf7ff7d99
Code: 90 e8 0b 00 00 00 f3 90 0f ae e8 eb f9 8d 74 26 00 89 3c 24 c3 90 90 90 90 90 90 90 90 90 90 90 90 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 eb 0d 90 90 90 90 90 90 90 90 90 90 90 90
RSP: 002b:00000000ff97b20c EFLAGS: 00000213 ORIG_RAX: 0000000000000036
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 000000000000541e
RDX: 0000000020000300 RSI: 00000000080ea078 RDI: 00000000ff97b260
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000

Local variable ----v32.i105@tty_compat_ioctl created at:
 compat_tty_tiocgserial drivers/tty/tty_io.c:2735 [inline]
 tty_compat_ioctl+0xf12/0x1850 drivers/tty/tty_io.c:2846
 compat_tty_tiocgserial drivers/tty/tty_io.c:2735 [inline]
 tty_compat_ioctl+0xf12/0x1850 drivers/tty/tty_io.c:2846

Bytes 50-51 of 60 are uninitialized
Memory access of size 60 starts at ffffb50b0158fce0
Data copied to user address 0000000020000300
=====================================================


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

[PATCH] tty: fix compat TIOCGSERIAL leaking uninitialized memory

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Commit 77654350306a ("take compat TIOC[SG]SERIAL treatment into
tty_compat_ioctl()") changed the compat version of TIOCGSERIAL to start
copying a whole struct to userspace rather than individual fields, but
failed to initialize all padding and fields -- namely the hole after the
'iomem_reg_shift' field, and the 'reserved' field.

Fix this by initializing the struct to zero.

Reported-by: syzbot+8da9175e28eadcb203ce@syzkaller.appspotmail.com
Fixes: 77654350306a ("take compat TIOC[SG]SERIAL treatment into tty_compat_ioctl()")
Cc: <stable@vger.kernel.org> # v4.20+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 drivers/tty/tty_io.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 1fcf7ad83dfa0..d24c250312edf 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2731,6 +2731,7 @@ static int compat_tty_tiocgserial(struct tty_struct *tty,
 	struct serial_struct v;
 	int err;
 	memset(&v, 0, sizeof(struct serial_struct));
+	memset(&v32, 0, sizeof(struct serial_struct32));
 
 	if (!tty->ops->set_serial)
 		return -ENOTTY;
-- 
2.25.1

Re: [PATCH] tty: fix compat TIOCGSERIAL leaking uninitialized memory

[Jiri Slaby]

On 24. 02. 20, 9:38, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Commit 77654350306a ("take compat TIOC[SG]SERIAL treatment into
> tty_compat_ioctl()") changed the compat version of TIOCGSERIAL to start
> copying a whole struct to userspace rather than individual fields, but
> failed to initialize all padding and fields -- namely the hole after the
> 'iomem_reg_shift' field, and the 'reserved' field.
> 
> Fix this by initializing the struct to zero.
> 
> Reported-by: syzbot+8da9175e28eadcb203ce@syzkaller.appspotmail.com
> Fixes: 77654350306a ("take compat TIOC[SG]SERIAL treatment into tty_compat_ioctl()")
> Cc: <stable@vger.kernel.org> # v4.20+
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  drivers/tty/tty_io.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 1fcf7ad83dfa0..d24c250312edf 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2731,6 +2731,7 @@ static int compat_tty_tiocgserial(struct tty_struct *tty,
>  	struct serial_struct v;
>  	int err;
>  	memset(&v, 0, sizeof(struct serial_struct));
> +	memset(&v32, 0, sizeof(struct serial_struct32));

sizeof(v32) please.

thanks,
-- 
js
suse labs

Re: [PATCH] tty: fix compat TIOCGSERIAL leaking uninitialized memory

[Eric Biggers]

On Mon, Feb 24, 2020 at 09:47:26AM +0100, Jiri Slaby wrote:
> On 24. 02. 20, 9:38, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Commit 77654350306a ("take compat TIOC[SG]SERIAL treatment into
> > tty_compat_ioctl()") changed the compat version of TIOCGSERIAL to start
> > copying a whole struct to userspace rather than individual fields, but
> > failed to initialize all padding and fields -- namely the hole after the
> > 'iomem_reg_shift' field, and the 'reserved' field.
> > 
> > Fix this by initializing the struct to zero.
> > 
> > Reported-by: syzbot+8da9175e28eadcb203ce@syzkaller.appspotmail.com
> > Fixes: 77654350306a ("take compat TIOC[SG]SERIAL treatment into tty_compat_ioctl()")
> > Cc: <stable@vger.kernel.org> # v4.20+
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  drivers/tty/tty_io.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index 1fcf7ad83dfa0..d24c250312edf 100644
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -2731,6 +2731,7 @@ static int compat_tty_tiocgserial(struct tty_struct *tty,
> >  	struct serial_struct v;
> >  	int err;
> >  	memset(&v, 0, sizeof(struct serial_struct));
> > +	memset(&v32, 0, sizeof(struct serial_struct32));
> 
> sizeof(v32) please.
> 

Sure, I was trying to be consistent with the other line, but I'll just change
them both to sizeof.

- Eric

[PATCH v2 0/2] tty: fix bugs in compat TIOCGSERIAL

[Eric Biggers]

Fix two bugs in the compat implementation of TIOCGSERIAL, both of which
were introduced by the compat ioctl refactoring in Linux v4.20.

Eric Biggers (2):
  tty: fix compat TIOCGSERIAL leaking uninitialized memory
  tty: fix compat TIOCGSERIAL checking wrong function ptr

 drivers/tty/tty_io.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.25.0.265.gbab2e86ba0-goog

[PATCH v2 1/2] tty: fix compat TIOCGSERIAL leaking uninitialized memory

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Commit 77654350306a ("take compat TIOC[SG]SERIAL treatment into
tty_compat_ioctl()") changed the compat version of TIOCGSERIAL to start
copying a whole 'serial_struct32' to userspace rather than individual
fields, but failed to initialize all padding and fields -- namely the
hole after the 'iomem_reg_shift' field, and the 'reserved' field.

Fix this by initializing the struct to zero.

[v2: use sizeof, and convert the adjacent line for consistency.]

Reported-by: syzbot+8da9175e28eadcb203ce@syzkaller.appspotmail.com
Fixes: 77654350306a ("take compat TIOC[SG]SERIAL treatment into tty_compat_ioctl()")
Cc: <stable@vger.kernel.org> # v4.20+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 drivers/tty/tty_io.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 1fcf7ad83dfa0a..db4a13bc855ed6 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2730,7 +2730,9 @@ static int compat_tty_tiocgserial(struct tty_struct *tty,
 	struct serial_struct32 v32;
 	struct serial_struct v;
 	int err;
-	memset(&v, 0, sizeof(struct serial_struct));
+
+	memset(&v, 0, sizeof(v));
+	memset(&v32, 0, sizeof(v32));
 
 	if (!tty->ops->set_serial)
 		return -ENOTTY;
-- 
2.25.0.265.gbab2e86ba0-goog

[PATCH v2 2/2] tty: fix compat TIOCGSERIAL checking wrong function ptr

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Commit 77654350306a ("take compat TIOC[SG]SERIAL treatment into
tty_compat_ioctl()") changed the compat version of TIOCGSERIAL to start
checking for the presence of the ->set_serial function pointer rather
than ->get_serial.  This appears to be a copy-and-paste error, since
->get_serial is the function pointer that is called as well as the
pointer that is checked by the non-compat version of TIOCGSERIAL.

Fix this by checking the correct function pointer.

Fixes: 77654350306a ("take compat TIOC[SG]SERIAL treatment into tty_compat_ioctl()")
Cc: <stable@vger.kernel.org> # v4.20+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 drivers/tty/tty_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index db4a13bc855ed6..5a6f36b391d95d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2734,7 +2734,7 @@ static int compat_tty_tiocgserial(struct tty_struct *tty,
 	memset(&v, 0, sizeof(v));
 	memset(&v32, 0, sizeof(v32));
 
-	if (!tty->ops->set_serial)
+	if (!tty->ops->get_serial)
 		return -ENOTTY;
 	err = tty->ops->get_serial(tty, &v);
 	if (!err) {
-- 
2.25.0.265.gbab2e86ba0-goog

Re: [PATCH v2 2/2] tty: fix compat TIOCGSERIAL checking wrong function ptr

[Jiri Slaby]

On 24. 02. 20, 19:20, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Commit 77654350306a ("take compat TIOC[SG]SERIAL treatment into
> tty_compat_ioctl()") changed the compat version of TIOCGSERIAL to start
> checking for the presence of the ->set_serial function pointer rather
> than ->get_serial.  This appears to be a copy-and-paste error, since
> ->get_serial is the function pointer that is called as well as the
> pointer that is checked by the non-compat version of TIOCGSERIAL.
> 
> Fix this by checking the correct function pointer.
> 
> Fixes: 77654350306a ("take compat TIOC[SG]SERIAL treatment into tty_compat_ioctl()")
> Cc: <stable@vger.kernel.org> # v4.20+
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Jiri Slaby <jslaby@suse.cz>

> ---
>  drivers/tty/tty_io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index db4a13bc855ed6..5a6f36b391d95d 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2734,7 +2734,7 @@ static int compat_tty_tiocgserial(struct tty_struct *tty,
>  	memset(&v, 0, sizeof(v));
>  	memset(&v32, 0, sizeof(v32));
>  
> -	if (!tty->ops->set_serial)
> +	if (!tty->ops->get_serial)
>  		return -ENOTTY;
>  	err = tty->ops->get_serial(tty, &v);
>  	if (!err) {
> 


-- 
js
suse labs

Re: [PATCH v2 1/2] tty: fix compat TIOCGSERIAL leaking uninitialized memory

[Jiri Slaby]

On 24. 02. 20, 19:20, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Commit 77654350306a ("take compat TIOC[SG]SERIAL treatment into
> tty_compat_ioctl()") changed the compat version of TIOCGSERIAL to start
> copying a whole 'serial_struct32' to userspace rather than individual
> fields, but failed to initialize all padding and fields -- namely the
> hole after the 'iomem_reg_shift' field, and the 'reserved' field.
> 
> Fix this by initializing the struct to zero.
> 
> [v2: use sizeof, and convert the adjacent line for consistency.]
> 
> Reported-by: syzbot+8da9175e28eadcb203ce@syzkaller.appspotmail.com
> Fixes: 77654350306a ("take compat TIOC[SG]SERIAL treatment into tty_compat_ioctl()")
> Cc: <stable@vger.kernel.org> # v4.20+
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Jiri Slaby <jslaby@suse.cz>

> ---
>  drivers/tty/tty_io.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 1fcf7ad83dfa0a..db4a13bc855ed6 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2730,7 +2730,9 @@ static int compat_tty_tiocgserial(struct tty_struct *tty,
>  	struct serial_struct32 v32;
>  	struct serial_struct v;
>  	int err;
> -	memset(&v, 0, sizeof(struct serial_struct));
> +
> +	memset(&v, 0, sizeof(v));
> +	memset(&v32, 0, sizeof(v32));
>  
>  	if (!tty->ops->set_serial)
>  		return -ENOTTY;
> 


-- 
js
suse labs

Re: [PATCH v2 1/2] tty: fix compat TIOCGSERIAL leaking uninitialized memory

[Eric Biggers]

On Tue, Feb 25, 2020 at 08:30:35AM +0100, Jiri Slaby wrote:
> On 24. 02. 20, 19:20, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Commit 77654350306a ("take compat TIOC[SG]SERIAL treatment into
> > tty_compat_ioctl()") changed the compat version of TIOCGSERIAL to start
> > copying a whole 'serial_struct32' to userspace rather than individual
> > fields, but failed to initialize all padding and fields -- namely the
> > hole after the 'iomem_reg_shift' field, and the 'reserved' field.
> > 
> > Fix this by initializing the struct to zero.
> > 
> > [v2: use sizeof, and convert the adjacent line for consistency.]
> > 
> > Reported-by: syzbot+8da9175e28eadcb203ce@syzkaller.appspotmail.com
> > Fixes: 77654350306a ("take compat TIOC[SG]SERIAL treatment into tty_compat_ioctl()")
> > Cc: <stable@vger.kernel.org> # v4.20+
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> 
> Acked-by: Jiri Slaby <jslaby@suse.cz>
> 

Thanks.  Greg, are you planning to take these patches?

- Eric

Re: [PATCH v2 1/2] tty: fix compat TIOCGSERIAL leaking uninitialized memory

[Greg Kroah-Hartman]

On Mon, Mar 02, 2020 at 01:24:25PM -0800, Eric Biggers wrote:
> On Tue, Feb 25, 2020 at 08:30:35AM +0100, Jiri Slaby wrote:
> > On 24. 02. 20, 19:20, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > Commit 77654350306a ("take compat TIOC[SG]SERIAL treatment into
> > > tty_compat_ioctl()") changed the compat version of TIOCGSERIAL to start
> > > copying a whole 'serial_struct32' to userspace rather than individual
> > > fields, but failed to initialize all padding and fields -- namely the
> > > hole after the 'iomem_reg_shift' field, and the 'reserved' field.
> > > 
> > > Fix this by initializing the struct to zero.
> > > 
> > > [v2: use sizeof, and convert the adjacent line for consistency.]
> > > 
> > > Reported-by: syzbot+8da9175e28eadcb203ce@syzkaller.appspotmail.com
> > > Fixes: 77654350306a ("take compat TIOC[SG]SERIAL treatment into tty_compat_ioctl()")
> > > Cc: <stable@vger.kernel.org> # v4.20+
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > 
> > Acked-by: Jiri Slaby <jslaby@suse.cz>
> > 
> 
> Thanks.  Greg, are you planning to take these patches?

Yes, please give me a chance to catch up on things :)

Re: [PATCH v2 1/2] tty: fix compat TIOCGSERIAL leaking uninitialized memory

[Greg Kroah-Hartman]

On Mon, Mar 02, 2020 at 01:24:25PM -0800, Eric Biggers wrote:
> On Tue, Feb 25, 2020 at 08:30:35AM +0100, Jiri Slaby wrote:
> > On 24. 02. 20, 19:20, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > Commit 77654350306a ("take compat TIOC[SG]SERIAL treatment into
> > > tty_compat_ioctl()") changed the compat version of TIOCGSERIAL to start
> > > copying a whole 'serial_struct32' to userspace rather than individual
> > > fields, but failed to initialize all padding and fields -- namely the
> > > hole after the 'iomem_reg_shift' field, and the 'reserved' field.
> > > 
> > > Fix this by initializing the struct to zero.
> > > 
> > > [v2: use sizeof, and convert the adjacent line for consistency.]
> > > 
> > > Reported-by: syzbot+8da9175e28eadcb203ce@syzkaller.appspotmail.com
> > > Fixes: 77654350306a ("take compat TIOC[SG]SERIAL treatment into tty_compat_ioctl()")
> > > Cc: <stable@vger.kernel.org> # v4.20+
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > 
> > Acked-by: Jiri Slaby <jslaby@suse.cz>
> > 
> 
> Thanks.  Greg, are you planning to take these patches?

Yes, sorry, they were not cc: linux-serial and fell through my initial
filters, to go into my generic "to-review" bucket.  Will take them
now...

greg k-h

Re: [PATCH v2 1/2] tty: fix compat TIOCGSERIAL leaking uninitialized memory

[Eric Biggers]

On Wed, Mar 18, 2020 at 01:00:00PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Mar 02, 2020 at 01:24:25PM -0800, Eric Biggers wrote:
> > On Tue, Feb 25, 2020 at 08:30:35AM +0100, Jiri Slaby wrote:
> > > On 24. 02. 20, 19:20, Eric Biggers wrote:
> > > > From: Eric Biggers <ebiggers@google.com>
> > > > 
> > > > Commit 77654350306a ("take compat TIOC[SG]SERIAL treatment into
> > > > tty_compat_ioctl()") changed the compat version of TIOCGSERIAL to start
> > > > copying a whole 'serial_struct32' to userspace rather than individual
> > > > fields, but failed to initialize all padding and fields -- namely the
> > > > hole after the 'iomem_reg_shift' field, and the 'reserved' field.
> > > > 
> > > > Fix this by initializing the struct to zero.
> > > > 
> > > > [v2: use sizeof, and convert the adjacent line for consistency.]
> > > > 
> > > > Reported-by: syzbot+8da9175e28eadcb203ce@syzkaller.appspotmail.com
> > > > Fixes: 77654350306a ("take compat TIOC[SG]SERIAL treatment into tty_compat_ioctl()")
> > > > Cc: <stable@vger.kernel.org> # v4.20+
> > > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > 
> > > Acked-by: Jiri Slaby <jslaby@suse.cz>
> > > 
> > 
> > Thanks.  Greg, are you planning to take these patches?
> 
> Yes, sorry, they were not cc: linux-serial and fell through my initial
> filters, to go into my generic "to-review" bucket.  Will take them
> now...
> 

If people are supposed to send tty patches to linux-serial, then you need to add
it to MAINTAINERS:

$ ./scripts/get_maintainer.pl drivers/tty/
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:TTY LAYER)
Jiri Slaby <jslaby@suse.com> (supporter:TTY LAYER)
linux-kernel@vger.kernel.org (open list)