[BUG] nbd: backend string memory leaked when device_create_file() fails in nbd_genl_connect()
Hello Linux kernel developers and maintainers,
I found a memory leak in drivers/block/nbd.c when fuzzing with Syzkaller.
Kernel version: commit 8cd9520d35a6c38db6567e97dd93b1f11f185dc6 (tag v7.1).
And the leak is also possible in the current mainline.
Relevant kernel config:
CONFIG_BLK_DEV_NBD=y
CONFIG_DEBUG_KMEMLEAK=y
CONFIG_FAULT_INJECTION=y
CONFIG_FAILSLAB=y
CONFIG_FAULT_INJECTION_DEBUG_FS=y
=============================
The original Syzkaller report
=============================
BUG: memory leak
unreferenced object 0xffff888011d17420 (size 16):
comm "syz.3.80", pid 9959, jiffies 4294977483
hex dump (first 16 bytes):
2f 64 65 76 2f 6e 62 64 23 00 00 00 00 00 00 00 /dev/nbd#.......
backtrace (crc 889be63d):
kmemleak_alloc_recursive include/linux/kmemleak.h:44 [inline]
slab_post_alloc_hook mm/slub.c:4575 [inline]
slab_alloc_node mm/slub.c:4899 [inline]
__do_kmalloc_node mm/slub.c:5295 [inline]
__kmalloc_noprof+0x552/0x850 mm/slub.c:5308
kmalloc_noprof include/linux/slab.h:954 [inline]
nla_strdup+0xc6/0x150 lib/nlattr.c:816
nbd_genl_connect+0x1231/0x1c10 drivers/block/nbd.c:2224
genl_family_rcv_msg_doit+0x209/0x2f0 net/netlink/genetlink.c:1114
genl_family_rcv_msg net/netlink/genetlink.c:1194 [inline]
genl_rcv_msg+0x55c/0x800 net/netlink/genetlink.c:1209
netlink_rcv_skb+0x158/0x420 net/netlink/af_netlink.c:2555
genl_rcv+0x28/0x40 net/netlink/genetlink.c:1218
netlink_unicast_kernel net/netlink/af_netlink.c:1318 [inline]
netlink_unicast+0x584/0x850 net/netlink/af_netlink.c:1344
netlink_sendmsg+0x8ba/0xd60 net/netlink/af_netlink.c:1899
sock_sendmsg_nosec net/socket.c:787 [inline]
__sock_sendmsg net/socket.c:802 [inline]
____sys_sendmsg+0x9eb/0xb80 net/socket.c:2699
___sys_sendmsg+0x134/0x1d0 net/socket.c:2753
__sys_sendmsg+0x16d/0x220 net/socket.c:2785
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x116/0x800 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
<<<<<<<<<<<<<<< tail report >>>>>>>>>>>>>>>
==========
Root cause
==========
nbd_genl_connect() allocates the backend string unconditionally BEFORE
creating the sysfs file:
/* drivers/block/nbd.c (v7.1, around line 2223) */
if (info->attrs[NBD_ATTR_BACKEND_IDENTIFIER]) {
nbd->backend = nla_strdup(...); /* (1) heap alloc */
if (!nbd->backend) {
ret = -ENOMEM;
goto out; /* (2) backend is NULL here, no leak */
}
}
ret = device_create_file(disk_to_dev(nbd->disk), &backend_attr);
if (ret) {
dev_err(...);
goto out; /* (3) LEAK: flag not yet set */
}
set_bit(NBD_RT_HAS_BACKEND_FILE, &config->runtime_flags); /* (4) skipped */
ret = nbd_start_device(nbd);
out:
...
mutex_unlock(&nbd->config_lock);
nbd_config_put(nbd);
...
The cleanup path in nbd_config_put() frees nbd->backend ONLY when
NBD_RT_HAS_BACKEND_FILE is set:
/* drivers/block/nbd.c (v7.1, around line 1445) */
if (test_and_clear_bit(NBD_RT_HAS_BACKEND_FILE,
&config->runtime_flags)) {
device_remove_file(disk_to_dev(nbd->disk), &backend_attr);
kfree(nbd->backend); /* never reached here if flag is not set */
nbd->backend = NULL;
}
When device_create_file() at step (3) fails (e.g. due to OOM or fault injection),
execution jumps to out before step (4) sets the flag.
nbd_config_put() is then called with config_refs dropping to zero,
but since NBD_RT_HAS_BACKEND_FILE is not set, it skips the kfree().
The string allocated at step (1) is therefore leaked.
The size of a single leak equals the length of the NBD_ATTR_BACKEND_IDENTIFIER
netlink string rounded up to the next slab boundary.
The nla_policy entry for this attribute has no .len constraint, so in theory a single connect call
could leak up to ~65507 bytes (NLA_STRING maximum).
During Syzkaller fuzzing, the fuzzer produced a 16-byte string.
============
Proposed Fix
============
Decouple device_remove_file() (which correctly requires the flag to avoid
removing a file that was never created) from kfree(nbd->backend) (which only
needs the pointer to be non-NULL):
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1445,9 +1445,9 @@ static void nbd_config_put(struct nbd_device *nbd)
if (test_and_clear_bit(NBD_RT_HAS_BACKEND_FILE,
&config->runtime_flags)) {
device_remove_file(disk_to_dev(nbd->disk), &backend_attr);
- kfree(nbd->backend);
- nbd->backend = NULL;
}
+ kfree(nbd->backend);
+ nbd->backend = NULL;
The flag still guards device_remove_file() so we never try to remove a sysfs
file that was never created. nbd->backend is always freed if non-NULL,
regardless of whether the file creation succeeded.
Note: the line number in the above diff is valid in the v7.1 kernel.
I will adjust it according to the mainline kernel if this patch is considered resonable.
Best,
Peiyang
Reply to: