netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] slip: Fix memory leak in slip_open error path
@ 2019-11-13 11:45 jouni.hogander
  2019-11-13 20:08 ` David Miller
  2019-11-14  7:25 ` Jouni Högander
  0 siblings, 2 replies; 6+ messages in thread
From: jouni.hogander @ 2019-11-13 11:45 UTC (permalink / raw)
  To: netdev; +Cc: Jouni Hogander, David S. Miller, Oliver Hartkopp, Lukas Bulwahn

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();
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] slip: Fix memory leak in slip_open error path
  2019-11-13 11:45 [PATCH] slip: Fix memory leak in slip_open error path jouni.hogander
@ 2019-11-13 20:08 ` David Miller
  2019-11-14  7:25 ` Jouni Högander
  1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2019-11-13 20:08 UTC (permalink / raw)
  To: jouni.hogander; +Cc: netdev, socketcan, lukas.bulwahn

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] slip: Fix memory leak in slip_open error path
  2019-11-13 11:45 [PATCH] slip: Fix memory leak in slip_open error path jouni.hogander
  2019-11-13 20:08 ` David Miller
@ 2019-11-14  7:25 ` Jouni Högander
  2019-11-14  8:51   ` Jouni Högander
  2019-11-14  9:09   ` David Miller
  1 sibling, 2 replies; 6+ messages in thread
From: Jouni Högander @ 2019-11-14  7:25 UTC (permalink / raw)
  To: netdev; +Cc: David S. Miller, Oliver Hartkopp, Lukas Bulwahn

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] slip: Fix memory leak in slip_open error path
  2019-11-14  7:25 ` Jouni Högander
@ 2019-11-14  8:51   ` Jouni Högander
  2019-11-14  9:09   ` David Miller
  1 sibling, 0 replies; 6+ messages in thread
From: Jouni Högander @ 2019-11-14  8:51 UTC (permalink / raw)
  To: netdev; +Cc: David S. Miller, Oliver Hartkopp, Lukas Bulwahn

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] slip: Fix memory leak in slip_open error path
  2019-11-14  7:25 ` Jouni Högander
  2019-11-14  8:51   ` Jouni Högander
@ 2019-11-14  9:09   ` David Miller
  2019-11-14  9:30     ` Jouni Högander
  1 sibling, 1 reply; 6+ messages in thread
From: David Miller @ 2019-11-14  9:09 UTC (permalink / raw)
  To: jouni.hogander; +Cc: netdev, socketcan, lukas.bulwahn

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] slip: Fix memory leak in slip_open error path
  2019-11-14  9:09   ` David Miller
@ 2019-11-14  9:30     ` Jouni Högander
  0 siblings, 0 replies; 6+ messages in thread
From: Jouni Högander @ 2019-11-14  9:30 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, socketcan, lukas.bulwahn

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-14  9:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-13 11:45 [PATCH] slip: Fix memory leak in slip_open error path jouni.hogander
2019-11-13 20:08 ` David Miller
2019-11-14  7:25 ` Jouni Högander
2019-11-14  8:51   ` Jouni Högander
2019-11-14  9:09   ` David Miller
2019-11-14  9:30     ` Jouni Högander

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).