[BUG] nbd: backend string memory leaked when device_create_file() fails in nbd_genl_connect()

[Peiyang He]

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