linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out
@ 2019-12-09 19:35 syzbot
  2019-12-10  1:38 ` syzbot
  2021-04-26 16:14 ` [PATCH] serial: 8250: fix NULL pointer dereference in serial8250_do_startup() Vegard Nossum
  0 siblings, 2 replies; 9+ messages in thread
From: syzbot @ 2019-12-09 19:35 UTC (permalink / raw)
  To: andriy.shevchenko, asierra, ext-kimmo.rautkoski, gregkh, jslaby,
	kai.heng.feng, linux-kernel, linux-serial, mika.westerberg,
	paulburton, sr, syzkaller-bugs, yegorslists

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+92f32d4e21fb246d31a2@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 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out
  2019-12-09 19:35 BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out syzbot
@ 2019-12-10  1:38 ` syzbot
  2019-12-12 10:57   ` Greg KH
  2021-04-26 16:14 ` [PATCH] serial: 8250: fix NULL pointer dereference in serial8250_do_startup() Vegard Nossum
  1 sibling, 1 reply; 9+ messages in thread
From: syzbot @ 2019-12-10  1:38 UTC (permalink / raw)
  To: andriy.shevchenko, asierra, corbet, ext-kimmo.rautkoski, gregkh,
	jslaby, kai.heng.feng, linux-api, linux-doc, linux-kernel,
	linux-serial, mika.westerberg, paulburton, peter, sr,
	syzkaller-bugs, yamada.masahiro, yegorslists

syzbot has bisected this bug to:

commit bd94c4077a0b2ecc35562c294f80f3659ecd8499
Author: Masahiro Yamada <yamada.masahiro@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+92f32d4e21fb246d31a2@syzkaller.appspotmail.com
Fixes: bd94c4077a0b ("serial: support 16-bit register interface for  
console")

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out
  2019-12-10  1:38 ` syzbot
@ 2019-12-12 10:57   ` Greg KH
  2019-12-13  9:05     ` Dmitry Vyukov
  0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2019-12-12 10:57 UTC (permalink / raw)
  To: syzbot
  Cc: andriy.shevchenko, asierra, corbet, ext-kimmo.rautkoski, jslaby,
	kai.heng.feng, linux-api, linux-doc, linux-kernel, linux-serial,
	mika.westerberg, paulburton, peter, sr, syzkaller-bugs,
	yamada.masahiro, yegorslists

On Mon, Dec 09, 2019 at 05:38:01PM -0800, syzbot wrote:
> syzbot has bisected this bug to:
> 
> commit bd94c4077a0b2ecc35562c294f80f3659ecd8499
> Author: Masahiro Yamada <yamada.masahiro@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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out
  2019-12-12 10:57   ` Greg KH
@ 2019-12-13  9:05     ` Dmitry Vyukov
  0 siblings, 0 replies; 9+ messages in thread
From: Dmitry Vyukov @ 2019-12-13  9:05 UTC (permalink / raw)
  To: Greg KH
  Cc: syzbot, Andy Shevchenko, asierra, Jonathan Corbet,
	ext-kimmo.rautkoski, Jiri Slaby, kai heng feng, Linux API,
	open list:DOCUMENTATION, LKML, linux-serial, mika.westerberg,
	paulburton, Peter Hurley, sr, syzkaller-bugs, Masahiro Yamada,
	yegorslists

On Thu, Dec 12, 2019 at 11:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Dec 09, 2019 at 05:38:01PM -0800, syzbot wrote:
> > syzbot has bisected this bug to:
> >
> > commit bd94c4077a0b2ecc35562c294f80f3659ecd8499
> > Author: Masahiro Yamada <yamada.masahiro@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...

This looks like:

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

Let's continue in that thread.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] serial: 8250: fix NULL pointer dereference in serial8250_do_startup()
  2019-12-09 19:35 BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out syzbot
  2019-12-10  1:38 ` syzbot
@ 2021-04-26 16:14 ` Vegard Nossum
  2021-04-26 16:17   ` Greg Kroah-Hartman
  1 sibling, 1 reply; 9+ messages in thread
From: Vegard Nossum @ 2021-04-26 16:14 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-serial, syzbot+4c7f1a69dfe24c6b3aeb,
	syzbot+92f32d4e21fb246d31a2
  Cc: linux-kernel, syzkaller-bugs, Vegard Nossum, Peter Hurley,
	Caleb Connolly

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+4c7f1a69dfe24c6b3aeb@syzkaller.appspotmail.com
Cc: Peter Hurley <peter@hurleysoftware.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-serial@vger.kernel.org
Cc: Caleb Connolly <caleb@connolly.tech>
Signed-off-by: Vegard Nossum <vegard.nossum@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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] serial: 8250: fix NULL pointer dereference in serial8250_do_startup()
  2021-04-26 16:14 ` [PATCH] serial: 8250: fix NULL pointer dereference in serial8250_do_startup() Vegard Nossum
@ 2021-04-26 16:17   ` Greg Kroah-Hartman
  2021-04-26 16:33     ` Vegard Nossum
  0 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2021-04-26 16:17 UTC (permalink / raw)
  To: Vegard Nossum
  Cc: linux-serial, syzbot+4c7f1a69dfe24c6b3aeb,
	syzbot+92f32d4e21fb246d31a2, linux-kernel, syzkaller-bugs,
	Peter Hurley, Caleb Connolly

On Mon, Apr 26, 2021 at 06:14:33PM +0200, Vegard Nossum wrote:
> 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+4c7f1a69dfe24c6b3aeb@syzkaller.appspotmail.com
> Cc: Peter Hurley <peter@hurleysoftware.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-serial@vger.kernel.org
> Cc: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Vegard Nossum <vegard.nossum@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;

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

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] serial: 8250: fix NULL pointer dereference in serial8250_do_startup()
  2021-04-26 16:17   ` Greg Kroah-Hartman
@ 2021-04-26 16:33     ` Vegard Nossum
  2021-04-28  6:36       ` BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out syzbot
  2021-05-13 14:24       ` [PATCH] serial: 8250: fix NULL pointer dereference in serial8250_do_startup() Greg Kroah-Hartman
  0 siblings, 2 replies; 9+ messages in thread
From: Vegard Nossum @ 2021-04-26 16:33 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-serial, syzbot+4c7f1a69dfe24c6b3aeb,
	syzbot+92f32d4e21fb246d31a2, linux-kernel, syzkaller-bugs,
	Peter Hurley, Caleb Connolly

On 2021-04-26 18:17, Greg Kroah-Hartman wrote:
> On Mon, Apr 26, 2021 at 06:14:33PM +0200, Vegard Nossum wrote:
>>   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;
> 
> These WARN_ON() will still trigger syzbot.  Are you sure you tested this
> and had syzbot verify it?

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out
  2021-04-26 16:33     ` Vegard Nossum
@ 2021-04-28  6:36       ` syzbot
  2021-05-13 14:24       ` [PATCH] serial: 8250: fix NULL pointer dereference in serial8250_do_startup() Greg Kroah-Hartman
  1 sibling, 0 replies; 9+ messages in thread
From: syzbot @ 2021-04-28  6:36 UTC (permalink / raw)
  To: caleb, gregkh, linux-kernel, linux-serial, peter, syzkaller-bugs,
	syzkaller-lts-bugs, vegard.nossum

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:       


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] serial: 8250: fix NULL pointer dereference in serial8250_do_startup()
  2021-04-26 16:33     ` Vegard Nossum
  2021-04-28  6:36       ` BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out syzbot
@ 2021-05-13 14:24       ` Greg Kroah-Hartman
  1 sibling, 0 replies; 9+ messages in thread
From: Greg Kroah-Hartman @ 2021-05-13 14:24 UTC (permalink / raw)
  To: Vegard Nossum
  Cc: linux-serial, syzbot+4c7f1a69dfe24c6b3aeb,
	syzbot+92f32d4e21fb246d31a2, linux-kernel, syzkaller-bugs,
	Peter Hurley, Caleb Connolly

On Mon, Apr 26, 2021 at 06:33:01PM +0200, Vegard Nossum wrote:
> On 2021-04-26 18:17, Greg Kroah-Hartman wrote:
> > On Mon, Apr 26, 2021 at 06:14:33PM +0200, Vegard Nossum wrote:
> > >   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;
> > 
> > These WARN_ON() will still trigger syzbot.  Are you sure you tested this
> > and had syzbot verify it?
> 
> 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?)
> 

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

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-05-13 14:24 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-09 19:35 BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out syzbot
2019-12-10  1:38 ` syzbot
2019-12-12 10:57   ` Greg KH
2019-12-13  9:05     ` Dmitry Vyukov
2021-04-26 16:14 ` [PATCH] serial: 8250: fix NULL pointer dereference in serial8250_do_startup() Vegard Nossum
2021-04-26 16:17   ` Greg Kroah-Hartman
2021-04-26 16:33     ` Vegard Nossum
2021-04-28  6:36       ` BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out syzbot
2021-05-13 14:24       ` [PATCH] serial: 8250: fix NULL pointer dereference in serial8250_do_startup() Greg Kroah-Hartman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).