BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit: e42617b8 Linux 5.5-rc1
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1157cd41e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=3754e2c78c1adb82
dashboard link: https://syzkaller.appspot.com/bug?extid=92f32d4e21fb246d31a2
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=136f7e41e00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=112b7c82e00000

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

BUG: kernel NULL pointer dereference, address: 0000000000000003
#PF: supervisor write access in kernel mode
#PF: error_code(0x0002) - not-present page
PGD a9a61067 P4D a9a61067 PUD 8fa24067 PMD 0
Oops: 0002 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 9054 Comm: syz-executor150 Not tainted 5.5.0-rc1-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
RIP: 0010:writew arch/x86/include/asm/io.h:66 [inline]
RIP: 0010:mem16_serial_out+0x6c/0x90 drivers/tty/serial/8250/8250_port.c:414
Code: b6 8d e9 00 00 00 49 8d 7d 40 48 b8 00 00 00 00 00 fc ff df 48 89 fa
48 c1 ea 03 d3 e3 80 3c 02 00 75 19 48 63 db 49 03 5d 40 <66> 44 89 23 5b
41 5c 41 5d 5d c3 e8 d4 44 cf fd eb c2 e8 2d 45 cf
RSP: 0018:ffffc90001cf7908 EFLAGS: 00010206
RAX: dffffc0000000000 RBX: 0000000000000003 RCX: 0000000000000000
RDX: 1ffffffff182080e RSI: ffffffff83e38106 RDI: ffffffff8c104070
RBP: ffffc90001cf7920 R08: ffff88808ffac040 R09: ffffed10431421c6
R10: ffffed10431421c5 R11: ffff888218a10e2b R12: 00000000000000bf
R13: ffffffff8c104030 R14: ffffc90001cf7a40 R15: ffffffff8c104188
FS: 0000000000866880(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000003 CR3: 00000000a64a2000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
serial_port_out include/linux/serial_core.h:265 [inline]
serial8250_do_startup+0x12b9/0x1cf0
drivers/tty/serial/8250/8250_port.c:2077
serial8250_startup+0x62/0x80 drivers/tty/serial/8250/8250_port.c:2329
uart_port_startup drivers/tty/serial/serial_core.c:219 [inline]
uart_startup drivers/tty/serial/serial_core.c:258 [inline]
uart_startup+0x452/0x980 drivers/tty/serial/serial_core.c:249
uart_set_info drivers/tty/serial/serial_core.c:998 [inline]
uart_set_info_user+0x13b4/0x1cf0 drivers/tty/serial/serial_core.c:1023
tty_tiocsserial drivers/tty/tty_io.c:2506 [inline]
tty_ioctl+0xf60/0x14f0 drivers/tty/tty_io.c:2648
vfs_ioctl fs/ioctl.c:47 [inline]
file_ioctl fs/ioctl.c:545 [inline]
do_vfs_ioctl+0x977/0x14e0 fs/ioctl.c:732
ksys_ioctl+0xab/0xd0 fs/ioctl.c:749
__do_sys_ioctl fs/ioctl.c:756 [inline]
__se_sys_ioctl fs/ioctl.c:754 [inline]
__x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754
do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x440219
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffc99622388 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440219
RDX: 0000000020000240 RSI: 000000000000541f RDI: 0000000000000003
RBP: 00000000006ca018 R08: 0000000000000000 R09: 00000000004002c8
R10: 0000000000401b30 R11: 0000000000000246 R12: 0000000000401aa0
R13: 0000000000401b30 R14: 0000000000000000 R15: 0000000000000000
Modules linked in:
CR2: 0000000000000003
---[ end trace 2e0575eb0019173e ]---
RIP: 0010:writew arch/x86/include/asm/io.h:66 [inline]
RIP: 0010:mem16_serial_out+0x6c/0x90 drivers/tty/serial/8250/8250_port.c:414
Code: b6 8d e9 00 00 00 49 8d 7d 40 48 b8 00 00 00 00 00 fc ff df 48 89 fa
48 c1 ea 03 d3 e3 80 3c 02 00 75 19 48 63 db 49 03 5d 40 <66> 44 89 23 5b
41 5c 41 5d 5d c3 e8 d4 44 cf fd eb c2 e8 2d 45 cf
RSP: 0018:ffffc90001cf7908 EFLAGS: 00010206
RAX: dffffc0000000000 RBX: 0000000000000003 RCX: 0000000000000000
RDX: 1ffffffff182080e RSI: ffffffff83e38106 RDI: ffffffff8c104070
RBP: ffffc90001cf7920 R08: ffff88808ffac040 R09: ffffed10431421c6
R10: ffffed10431421c5 R11: ffff888218a10e2b R12: 00000000000000bf
R13: ffffffff8c104030 R14: ffffc90001cf7a40 R15: ffffffff8c104188
FS: 0000000000866880(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000003 CR3: 00000000a64a2000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


---
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 syzk...@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

[syzbot]

syzbot has bisected this bug to:

commit bd94c4077a0b2ecc35562c294f80f3659ecd8499
Author: Masahiro Yamada <yamada....@socionext.com>
Date: Wed Oct 28 03:46:05 2015 +0000

serial: support 16-bit register interface for console

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=13723196e00000
start commit: e42617b8 Linux 5.5-rc1
git tree: upstream
final crash: https://syzkaller.appspot.com/x/report.txt?x=10f23196e00000
console output: https://syzkaller.appspot.com/x/log.txt?x=17723196e00000

kernel config: https://syzkaller.appspot.com/x/.config?x=3754e2c78c1adb82
dashboard link: https://syzkaller.appspot.com/bug?extid=92f32d4e21fb246d31a2

syz repro: https://syzkaller.appspot.com/x/repro.syz?x=136f7e41e00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=112b7c82e00000

Reported-by: syzbot+92f32d...@syzkaller.appspotmail.com
Fixes: bd94c4077a0b ("serial: support 16-bit register interface for
console")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

[Greg KH]

On Mon, Dec 09, 2019 at 05:38:01PM -0800, syzbot wrote:
> syzbot has bisected this bug to:
>
> commit bd94c4077a0b2ecc35562c294f80f3659ecd8499
> Author: Masahiro Yamada <yamada....@socionext.com>
> Date: Wed Oct 28 03:46:05 2015 +0000
>
> serial: support 16-bit register interface for console

That would be because that is when this function was added to the kernel
:)

Again, you are asking the kernel to write to a bad place in memory, and
then crash when that happens. That sounds like the correct
functionality to me...

thanks,

greg k-h

[Dmitry Vyukov]

This looks like:

#syz dup:
BUG: unable to handle kernel NULL pointer dereference in mem_serial_out

Let's continue in that thread.

[Vegard Nossum]

Syzbot reported a crash, here reproduced on a recent mainline kernel:

BUG: kernel NULL pointer dereference, address: 0000000000000005
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 120cf067 P4D 120cf067 PUD 135d4067 PMD 0
Oops: 0000 [#1] PREEMPT SMP KASAN
CPU: 2 PID: 4830 Comm: a.out Not tainted 5.12.0-rc7+ #209
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
RIP: 0010:mem16_serial_in+0x83/0xa0
[...]
Call Trace:
serial8250_do_startup+0x475/0x1e40
serial8250_startup+0x5c/0x80
uart_startup+0x360/0x870
uart_set_info_user+0x13a3/0x1c30
tty_ioctl+0x711/0x14f0
__x64_sys_ioctl+0x193/0x200
do_syscall_64+0x2d/0x70
entry_SYSCALL_64_after_hwframe+0x44/0xae

A more readable reproducer is:

#include <sys/ioctl.h>
#include <fcntl.h>

#include <linux/serial.h>

#ifndef SERIAL_IO_MEM16
#define SERIAL_IO_MEM16 7
#endif

int main(int argc, char *argv[])
{
int fd = open("/dev/ttyS3", O_RDONLY);

struct serial_struct ss = {};
ss.type = 0x10;
ss.baud_base = 0x7fffffff;
ss.io_type = SERIAL_IO_MEM16;
ioctl(fd, TIOCSSERIAL, &ss);

return 0;
}

ioctl(TIOCSSERIAL) attempts to configure the serial port, but when
requesting io_type SERIAL_IO_MEM*/UPIO_MEM* it goes on to dereference
->membase in serial8250_do_startup().

I propose this fix, which will fail validation of the TIOCSSERIAL request
if you request a memory-based or io-based io_type when the underlying port
has no valid ->membase or ->iobase, respectively.

As far as I can tell, this driver was written to support being able to
switch between the two IO types for a given port (assuming the underlying
driver supports it); see serial8250_do_startup()/set_io_from_upio().

I'm also adding a couple of WARN_ON_ONCE()s which are technically
redundant, but which could help somebody else if they come across a
similar issue in the future.

Reported-by: syzbot+4c7f1a...@syzkaller.appspotmail.com
Cc: Peter Hurley <pe...@hurleysoftware.com>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: linux-...@vger.kernel.org
Cc: Caleb Connolly <ca...@connolly.tech>
Signed-off-by: Vegard Nossum <vegard...@oracle.com>
---
drivers/tty/serial/8250/8250_port.c | 43 +++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index b0af13074cd36..aec3abff8e48e 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -455,6 +455,33 @@ static void io_serial_out(struct uart_port *p, int offset, int value)

static int serial8250_default_handle_irq(struct uart_port *port);

+static int needs_membase(int iotype)
+{
+ switch (iotype) {
+ case UPIO_MEM:
+ case UPIO_MEM16:
+ case UPIO_MEM32:
+ case UPIO_MEM32BE:
+#ifdef CONFIG_SERIAL_8250_RT288X
+ case UPIO_AU:
+#endif
+ return 1;
+ }
+
+ return 0;
+}
+
+static int needs_iobase(int iotype)
+{
+ switch (iotype) {
+ case UPIO_HUB6:
+ case UPIO_PORT:
+ return 1;
+ }
+
+ return 0;
+}
+
static void set_io_from_upio(struct uart_port *p)
{
struct uart_8250_port *up = up_to_u8250p(p);
@@ -2151,6 +2178,11 @@ int serial8250_do_startup(struct uart_port *port)
unsigned char lsr, iir;
int retval;

+ if (WARN_ON_ONCE(needs_membase(port->iotype) && !port->membase))
+ return -ENODEV;
+ if (WARN_ON_ONCE(needs_iobase(port->iotype) && !port->iobase))
+ return -ENODEV;
+
if (!port->fifosize)
port->fifosize = uart_config[port->type].fifo_size;
if (!up->tx_loadsz)
@@ -3157,6 +3189,17 @@ serial8250_verify_port(struct uart_port *port, struct serial_struct *ser)
ser->type >= ARRAY_SIZE(uart_config) || ser->type == PORT_CIRRUS ||
ser->type == PORT_STARTECH)
return -EINVAL;
+
+ /*
+ * This driver clearly was intended to support switching between
+ * io types (see serial8250_do_startup()), so we need to ensure that
+ * the underlying port type will support the request.
+ */
+ if (needs_membase(ser->io_type) && !port->membase)
+ return -EINVAL;
+ if (needs_iobase(ser->io_type) && !port->iobase)
+ return -EINVAL;
+
return 0;
}

--
2.16.1.72.g5be1f00a9.dirty

[Greg Kroah-Hartman]

These WARN_ON() will still trigger syzbot. Are you sure you tested this
and had syzbot verify it?

thanks,

greg k-h

[Vegard Nossum]

I tested it locally and the WARN_ON()s don't trigger -- presumably
because serial8250_verify_port() is called from uart_set_info() before
we get to serial8250_do_startup():

/*
* Ask the low level driver to verify the settings.
*/
if (uport->ops->verify_port)
retval = uport->ops->verify_port(uport, new_info);

[...]

retval = uart_startup(tty, state, 1);

At least, this was my intention. Although now that I look at it again,
it looks like this check may be skipped in some cases; is that what
you're referring to?

I didn't have syzbot verify it -- I thought it would do that when
submitting my patch. Looks like I need to push somewhere and ask syzbot
to test it using this?

#syz test: git://repo/address.git commit-hash

(I assume I can send this privately as long as I use the right
syzbot+...@ To-address?)

Thanks,


Vegard

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

failed to checkout kernel repo git://repo/address.git/commit-hash: failed to run ["git" "fetch" "--force" "b7cf8f2fbfc36c709a08e0b9c77990e491473738" "commit-hash"]: exit status 128
fatal: unable to look up repo (port 9418) (Name or service not known)



Tested on:

commit: [unknown
git tree: git://repo/address.git commit-hash
dashboard link: https://syzkaller.appspot.com/bug?extid=4c7f1a69dfe24c6b3aeb
compiler:

[syzbot]

> #syz test

This bug is already marked as invalid. No point in testing.

> http://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

[Dmitry Vyukov]

See discussion on the bug:
https://groups.google.com/g/syzkaller-bugs/c/1rVENJf9P4U/m/qWKQUQuiAAAJ

[Vegard Nossum]

Hm. It's not like the user can set an arbitrary memory address for the
kernel to use here, it's a problem that the driver simply doesn't check
whether the configuration passed by userspace is supported by the
underlying device. Which I think we can check easily -- that's what my
patch does.

I'm okay dropping it, but I really don't see why this is any different
from other logic bugs, and fixing it would allow fuzzing to continue
with fewer false positives.


Vegard

[Dmitry Vyukov]

I completely agree, logical bugs are worth fixing. It's just that
syzbot rejects to test it because it's already marked as invalid:
https://github.com/google/syzkaller/blob/77e2b66864e69c17416614228723a1ebd3581ddc/dashboard/app/jobs.go#L178-L179
It was done to prevent potential abuse, and all use cases weren't
clear at the time.
There probably different "shades" of invalid, but syzbot doesn't
distinguish today.

[Greg Kroah-Hartman]

Dropping this now until you get this tested properly...

thanks,

greg k-h