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: [email protected]
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 [email protected].
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 has bisected this bug to:
commit bd94c4077a0b2ecc35562c294f80f3659ecd8499
Author: Masahiro Yamada <[email protected]>
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: [email protected]
Fixes: bd94c4077a0b ("serial: support 16-bit register interface for
console")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
On Mon, Dec 09, 2019 at 05:38:01PM -0800, syzbot wrote:
> syzbot has bisected this bug to:
>
> commit bd94c4077a0b2ecc35562c294f80f3659ecd8499
> Author: Masahiro Yamada <[email protected]>
> 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
On Thu, Dec 12, 2019 at 11:57 AM Greg KH <[email protected]> wrote:
>
> On Mon, Dec 09, 2019 at 05:38:01PM -0800, syzbot wrote:
> > syzbot has bisected this bug to:
> >
> > commit bd94c4077a0b2ecc35562c294f80f3659ecd8499
> > Author: Masahiro Yamada <[email protected]>
> > 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.
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: [email protected]
Cc: Peter Hurley <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Cc: Caleb Connolly <[email protected]>
Signed-off-by: Vegard Nossum <[email protected]>
---
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
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: [email protected]
> Cc: Peter Hurley <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> Cc: Caleb Connolly <[email protected]>
> Signed-off-by: Vegard Nossum <[email protected]>
> ---
> 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
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
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:
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