[PATCH v1 net] tun: Fix memory leak for detached NAPI queue.

[Kuniyuki Iwashima]

syzkaller reported [0] memory leaks of sk and skb related to the TUN
device with no repro, but we can reproduce it easily with:

struct ifreq ifr = {}
int fd_tun, fd_tmp;
char buf[4] = {};

fd_tun = openat(AT_FDCWD, "/dev/net/tun", O_WRONLY, 0);
ifr.ifr_flags = IFF_TUN | IFF_NAPI | IFF_MULTI_QUEUE;
ioctl(fd_tun, TUNSETIFF, &ifr);

ifr.ifr_flags = IFF_DETACH_QUEUE;
ioctl(fd_tun, TUNSETQUEUE, &ifr);

fd_tmp = socket(AF_PACKET, SOCK_PACKET, 0);
ifr.ifr_flags = IFF_UP;
ioctl(fd_tmp, SIOCSIFFLAGS, &ifr);

write(fd_tun, buf, sizeof(buf));
close(fd_tun);

If we enable NAPI and multi-queue on a TUN device, we can put skb into
tfile->sk.sk_write_queue after the queue is detached. We should prevent
it by checking tfile->detached before queuing skb.

Note this must be done under tfile->sk.sk_write_queue.lock because write()
and ioctl(IFF_DETACH_QUEUE) can run concurrently. Otherwise, there would
be a small race window:

write() ioctl(IFF_DETACH_QUEUE)
`- tun_get_user `- __tun_detach
|- if (tfile->detached) |- tun_disable_queue
| `-> false | `- tfile->detached = tun
| `- tun_queue_purge
|- spin_lock_bh(&queue->lock)
`- __skb_queue_tail(queue, skb)

Another solution is to call tun_queue_purge() when closing and
reattaching the detached queue, but it could paper over another
problems. Also, we do the same kind of test for IFF_NAPI_FRAGS.

[0]:
unreferenced object 0xffff88801edbc800 (size 2048):
comm "syz-executor.1", pid 33269, jiffies 4295743834 (age 18.756s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 07 40 00 00 00 00 00 00 00 00 00 00 00 00 ...@............
backtrace:
[<000000008c16ea3d>] __do_kmalloc_node mm/slab_common.c:965 [inline]
[<000000008c16ea3d>] __kmalloc+0x4a/0x130 mm/slab_common.c:979
[<000000003addde56>] kmalloc include/linux/slab.h:563 [inline]
[<000000003addde56>] sk_prot_alloc+0xef/0x1b0 net/core/sock.c:2035
[<000000003e20621f>] sk_alloc+0x36/0x2f0 net/core/sock.c:2088
[<0000000028e43843>] tun_chr_open+0x3d/0x190 drivers/net/tun.c:3438
[<000000001b0f1f28>] misc_open+0x1a6/0x1f0 drivers/char/misc.c:165
[<000000004376f706>] chrdev_open+0x111/0x300 fs/char_dev.c:414
[<00000000614d379f>] do_dentry_open+0x2f9/0x750 fs/open.c:920
[<000000008eb24774>] do_open fs/namei.c:3636 [inline]
[<000000008eb24774>] path_openat+0x143f/0x1a30 fs/namei.c:3791
[<00000000955077b5>] do_filp_open+0xce/0x1c0 fs/namei.c:3818
[<00000000b78973b0>] do_sys_openat2+0xf0/0x260 fs/open.c:1356
[<00000000057be699>] do_sys_open fs/open.c:1372 [inline]
[<00000000057be699>] __do_sys_openat fs/open.c:1388 [inline]
[<00000000057be699>] __se_sys_openat fs/open.c:1383 [inline]
[<00000000057be699>] __x64_sys_openat+0x83/0xf0 fs/open.c:1383
[<00000000a7d2182d>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
[<00000000a7d2182d>] do_syscall_64+0x3c/0x90 arch/x86/entry/common.c:80
[<000000004cc4e8c4>] entry_SYSCALL_64_after_hwframe+0x72/0xdc

unreferenced object 0xffff88802f671700 (size 240):
comm "syz-executor.1", pid 33269, jiffies 4295743854 (age 18.736s)
hex dump (first 32 bytes):
68 c9 db 1e 80 88 ff ff 68 c9 db 1e 80 88 ff ff h.......h.......
00 c0 7b 2f 80 88 ff ff 00 c8 db 1e 80 88 ff ff ..{/............
backtrace:
[<00000000e9d9fdb6>] __alloc_skb+0x223/0x250 net/core/skbuff.c:644
[<000000002c3e4e0b>] alloc_skb include/linux/skbuff.h:1288 [inline]
[<000000002c3e4e0b>] alloc_skb_with_frags+0x6f/0x350 net/core/skbuff.c:6378
[<00000000825f98d7>] sock_alloc_send_pskb+0x3ac/0x3e0 net/core/sock.c:2729
[<00000000e9eb3df3>] tun_alloc_skb drivers/net/tun.c:1529 [inline]
[<00000000e9eb3df3>] tun_get_user+0x5e1/0x1f90 drivers/net/tun.c:1841
[<0000000053096912>] tun_chr_write_iter+0xac/0x120 drivers/net/tun.c:2035
[<00000000b9282ae0>] call_write_iter include/linux/fs.h:1868 [inline]
[<00000000b9282ae0>] new_sync_write fs/read_write.c:491 [inline]
[<00000000b9282ae0>] vfs_write+0x40f/0x530 fs/read_write.c:584
[<00000000524566e4>] ksys_write+0xa1/0x170 fs/read_write.c:637
[<00000000a7d2182d>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
[<00000000a7d2182d>] do_syscall_64+0x3c/0x90 arch/x86/entry/common.c:80
[<000000004cc4e8c4>] entry_SYSCALL_64_after_hwframe+0x72/0xdc

Fixes: cde8b15f1aab ("tuntap: add ioctl to attach or detach a file form tuntap device")
Reported-by: syzkaller <syzk...@googlegroups.com>
Signed-off-by: Kuniyuki Iwashima <kun...@amazon.com>
---
drivers/net/tun.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index d4d0a41a905a..d75456adc62a 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1977,6 +1977,14 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
int queue_len;

spin_lock_bh(&queue->lock);
+
+ if (unlikely(tfile->detached)) {
+ spin_unlock_bh(&queue->lock);
+ rcu_read_unlock();
+ err = -EBUSY;
+ goto free_skb;
+ }
+
__skb_queue_tail(queue, skb);
queue_len = skb_queue_len(queue);
spin_unlock(&queue->lock);
@@ -2512,6 +2520,13 @@ static int tun_xdp_one(struct tun_struct *tun,
if (tfile->napi_enabled) {
queue = &tfile->sk.sk_write_queue;
spin_lock(&queue->lock);
+
+ if (unlikely(tfile->detached)) {
+ spin_unlock(&queue->lock);
+ kfree_skb(skb);
+ return -EBUSY;
+ }
+
__skb_queue_tail(queue, skb);
spin_unlock(&queue->lock);
ret = 1;
--
2.30.2

[patchwork-b...@kernel.org]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <da...@davemloft.net>:

On Mon, 15 May 2023 11:42:04 -0700 you wrote:
> syzkaller reported [0] memory leaks of sk and skb related to the TUN
> device with no repro, but we can reproduce it easily with:
>
> struct ifreq ifr = {}
> int fd_tun, fd_tmp;
> char buf[4] = {};
>

> [...]

Here is the summary with links:
- [v1,net] tun: Fix memory leak for detached NAPI queue.
https://git.kernel.org/netdev/net/c/82b2bc279467

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[Greg Kroah-Hartman]

From: Kuniyuki Iwashima <kun...@amazon.com>

[ Upstream commit 82b2bc279467c875ec36f8ef820f00997c2a4e8e ]

syzkaller reported [0] memory leaks of sk and skb related to the TUN
device with no repro, but we can reproduce it easily with:

struct ifreq ifr = {}
int fd_tun, fd_tmp;
char buf[4] = {};

Signed-off-by: David S. Miller <da...@davemloft.net>
Signed-off-by: Sasha Levin <sas...@kernel.org>

---
drivers/net/tun.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c

index 65706824eb828..7c8db8f6f661e 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1971,6 +1971,14 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,

int queue_len;

spin_lock_bh(&queue->lock);
+
+ if (unlikely(tfile->detached)) {
+ spin_unlock_bh(&queue->lock);
+ rcu_read_unlock();
+ err = -EBUSY;
+ goto free_skb;
+ }
+
__skb_queue_tail(queue, skb);
queue_len = skb_queue_len(queue);
spin_unlock(&queue->lock);

@@ -2506,6 +2514,13 @@ static int tun_xdp_one(struct tun_struct *tun,

if (tfile->napi_enabled) {
queue = &tfile->sk.sk_write_queue;
spin_lock(&queue->lock);
+
+ if (unlikely(tfile->detached)) {
+ spin_unlock(&queue->lock);
+ kfree_skb(skb);
+ return -EBUSY;
+ }
+
__skb_queue_tail(queue, skb);
spin_unlock(&queue->lock);
ret = 1;
--

2.39.2

[Greg Kroah-Hartman]

index ad653b32b2f00..44087db2a0595 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1976,6 +1976,14 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,

int queue_len;

spin_lock_bh(&queue->lock);
+
+ if (unlikely(tfile->detached)) {
+ spin_unlock_bh(&queue->lock);
+ rcu_read_unlock();
+ err = -EBUSY;
+ goto free_skb;
+ }
+
__skb_queue_tail(queue, skb);
queue_len = skb_queue_len(queue);
spin_unlock(&queue->lock);

@@ -2511,6 +2519,13 @@ static int tun_xdp_one(struct tun_struct *tun,