| Message ID | 20191113114502.22462-1-jouni.hogander@unikie.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | slip: Fix memory leak in slip_open error path | expand |
From: jouni.hogander@unikie.com Date: Wed, 13 Nov 2019 13:45:02 +0200 > From: Jouni Hogander <jouni.hogander@unikie.com> > > Driver/net/can/slcan.c is derived from slip.c. Memory leak was detected > by Syzkaller in slcan. Same issue exists in slip.c and this patch is > addressing the leak in slip.c. > > Here is the slcan memory leak trace reported by Syzkaller: ... > Signed-off-by: Jouni Hogander <jouni.hogander@unikie.com> Applied and queued up for -stable. Looking at slip_open() while reviewing this patch, it has this test: if (!test_bit(SLF_INUSE, &sl->flags)) { /* Perform the low-level SLIP initialization. */ err = sl_alloc_bufs(sl, SL_MTU); if (err) goto err_free_chan; which seems bogus, because 'sl' here is always a freshly allocated object from sl_alloc(), which always provides an all-zeros value on sl->flags so this test always passes. It can therefore be removed.
jouni.hogander@unikie.com writes: > From: Jouni Hogander <jouni.hogander@unikie.com> > > Driver/net/can/slcan.c is derived from slip.c. Memory leak was detected > by Syzkaller in slcan. Same issue exists in slip.c and this patch is > addressing the leak in slip.c. > > Here is the slcan memory leak trace reported by Syzkaller: > > BUG: memory leak unreferenced object 0xffff888067f65500 (size 4096): > comm "syz-executor043", pid 454, jiffies 4294759719 (age 11.930s) > hex dump (first 32 bytes): > 73 6c 63 61 6e 30 00 00 00 00 00 00 00 00 00 00 slcan0.......... > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<00000000a06eec0d>] __kmalloc+0x18b/0x2c0 > [<0000000083306e66>] kvmalloc_node+0x3a/0xc0 > [<000000006ac27f87>] alloc_netdev_mqs+0x17a/0x1080 > [<0000000061a996c9>] slcan_open+0x3ae/0x9a0 > [<000000001226f0f9>] tty_ldisc_open.isra.1+0x76/0xc0 > [<0000000019289631>] tty_set_ldisc+0x28c/0x5f0 > [<000000004de5a617>] tty_ioctl+0x48d/0x1590 > [<00000000daef496f>] do_vfs_ioctl+0x1c7/0x1510 > [<0000000059068dbc>] ksys_ioctl+0x99/0xb0 > [<000000009a6eb334>] __x64_sys_ioctl+0x78/0xb0 > [<0000000053d0332e>] do_syscall_64+0x16f/0x580 > [<0000000021b83b99>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > [<000000008ea75434>] 0xfffffffffffffff > > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Oliver Hartkopp <socketcan@hartkopp.net> > Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com> > Signed-off-by: Jouni Hogander <jouni.hogander@unikie.com> > --- > drivers/net/slip/slip.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c > index cac64b96d545..4d479e3c817d 100644 > --- a/drivers/net/slip/slip.c > +++ b/drivers/net/slip/slip.c > @@ -855,6 +855,7 @@ static int slip_open(struct tty_struct *tty) > sl->tty = NULL; > tty->disc_data = NULL; > clear_bit(SLF_INUSE, &sl->flags); > + free_netdev(sl->dev); > > err_exit: > rtnl_unlock(); Observed panic in another error path in my overnight Syzkaller run with this patch. Better not to apply it. Sorry for inconvenience. BR, Jouni Högander
jouni.hogander@unikie.com (Jouni Högander) writes: > jouni.hogander@unikie.com writes: > >> From: Jouni Hogander <jouni.hogander@unikie.com> >> >> Driver/net/can/slcan.c is derived from slip.c. Memory leak was detected >> by Syzkaller in slcan. Same issue exists in slip.c and this patch is >> addressing the leak in slip.c. >> >> Here is the slcan memory leak trace reported by Syzkaller: >> >> BUG: memory leak unreferenced object 0xffff888067f65500 (size 4096): >> comm "syz-executor043", pid 454, jiffies 4294759719 (age 11.930s) >> hex dump (first 32 bytes): >> 73 6c 63 61 6e 30 00 00 00 00 00 00 00 00 00 00 slcan0.......... >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >> backtrace: >> [<00000000a06eec0d>] __kmalloc+0x18b/0x2c0 >> [<0000000083306e66>] kvmalloc_node+0x3a/0xc0 >> [<000000006ac27f87>] alloc_netdev_mqs+0x17a/0x1080 >> [<0000000061a996c9>] slcan_open+0x3ae/0x9a0 >> [<000000001226f0f9>] tty_ldisc_open.isra.1+0x76/0xc0 >> [<0000000019289631>] tty_set_ldisc+0x28c/0x5f0 >> [<000000004de5a617>] tty_ioctl+0x48d/0x1590 >> [<00000000daef496f>] do_vfs_ioctl+0x1c7/0x1510 >> [<0000000059068dbc>] ksys_ioctl+0x99/0xb0 >> [<000000009a6eb334>] __x64_sys_ioctl+0x78/0xb0 >> [<0000000053d0332e>] do_syscall_64+0x16f/0x580 >> [<0000000021b83b99>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >> [<000000008ea75434>] 0xfffffffffffffff >> >> Cc: "David S. Miller" <davem@davemloft.net> >> Cc: Oliver Hartkopp <socketcan@hartkopp.net> >> Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com> >> Signed-off-by: Jouni Hogander <jouni.hogander@unikie.com> >> --- >> drivers/net/slip/slip.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c >> index cac64b96d545..4d479e3c817d 100644 >> --- a/drivers/net/slip/slip.c >> +++ b/drivers/net/slip/slip.c >> @@ -855,6 +855,7 @@ static int slip_open(struct tty_struct *tty) >> sl->tty = NULL; >> tty->disc_data = NULL; >> clear_bit(SLF_INUSE, &sl->flags); >> + free_netdev(sl->dev); >> >> err_exit: >> rtnl_unlock(); > > Observed panic in another error path in my overnight Syzkaller run with > this patch. Better not to apply it. Sorry for inconvenience. The panic was caused by another error path fix I had in my Syzkaller setup. I.e. this patch is ok. BR, Jouni Högander
From: jouni.hogander@unikie.com (Jouni Högander) Date: Thu, 14 Nov 2019 09:25:23 +0200 > Observed panic in another error path in my overnight Syzkaller run with > this patch. Better not to apply it. Sorry for inconvenience. I already did, please send a revert or a fix.
David Miller <davem@davemloft.net> writes: > From: jouni.hogander@unikie.com (Jouni Högander) > Date: Thu, 14 Nov 2019 09:25:23 +0200 > >> Observed panic in another error path in my overnight Syzkaller run with >> this patch. Better not to apply it. Sorry for inconvenience. > > I already did, please send a revert or a fix. I found out (and commented on patch) that the panic was caused by another error path fix I had in my Syzkaller setup. This patch is ok, no need to revert. BR, Jouni Högander
diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c index cac64b96d545..4d479e3c817d 100644 --- a/drivers/net/slip/slip.c +++ b/drivers/net/slip/slip.c @@ -855,6 +855,7 @@ static int slip_open(struct tty_struct *tty) sl->tty = NULL; tty->disc_data = NULL; clear_bit(SLF_INUSE, &sl->flags); + free_netdev(sl->dev); err_exit: rtnl_unlock();