Out of Memory in nbd_add_socket

[Hyeonjun Ahn]

Dear Linux maintainer & Contributor.

Hello, I'm Hyeonjun Ahn concerned with kernel security.

I deleted the mail that I sent before because it was in a dark theme by mistake and you would have trouble reading it.

Sorry for the inconvenience.
I found an integer overflow bug in drivers/block/nbd.c.
The absence of an integer overflow check for 'config->num_connections' in 'nbd_add_socket' prior to its incrementation poses a potential risk, as it can lead to following issues.

1. out-of-bounds when 'num_connections' reaches 0x80000000 (-2147483648) and the physical memory is large enough to be allocated by krealloc(ptr, 0x3fffffff8. GFP_KERNEL). (about 17 GB)
2. out-of-memory
3. null-pointer dereference and subsequent kernel panic when 'num_connections' reaches -1 and 'krealloc' returns a (void *)16, which corresponds to 'ZERO_SIZE_PTR'.

Kernel version : v6.6

I attached PoC.c, PoC.patch for a null-pointer dereference scenario and a patch file that I suggest.

Belows are crash when running PoC with a linux patched by PoC.patch and the root cause.

crash (KASAN disabled):
root@syzkaller:~# ./a.out
[   30.251866] audit: type=1400 audit(1699111379.984:5): avc:  denied  { read write } for  pid=260 comm="a.out" name="nbd0" dev="devtmpfs" ino=116 scontext=system_u:system_r:sysadm_t:s0 tc1
[   30.253974] audit: type=1400 audit(1699111379.984:6): avc:  denied  { open } for  pid=260 comm="a.out" path="/dev/nbd0" dev="devtmpfs" ino=116 scontext=system_u:system_r:sysadm_t:s0 tco1
[   30.256055] audit: type=1400 audit(1699111379.984:7): avc:  denied  { ioctl } for  pid=260 comm="poc" path="/dev/nbd0" dev="devtmpfs" ino=116 ioctlcmd=0xab00 scontext=system_u:system_r:1
[   30.260077] BUG: kernel NULL pointer dereference, address: 0000000000000008
[   30.260735] #PF: supervisor write access in kernel mode
[   30.261235] #PF: error_code(0x0002) - not-present page
[   30.261726] PGD 0 P4D 0
[   30.261979] Oops: 0002 [#1] PREEMPT SMP PTI
[   30.262381] CPU: 1 PID: 260 Comm: poc Not tainted 6.6.0-12401-g8f6f76a6a29f-dirty #11
[   30.263104] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
[   30.263876] RIP: 0010:nbd_add_socket+0x16f/0x280
[   30.264367] Code: 49 89 2c 24 49 c7 44 24 28 00 00 00 00 41 c7 44 24 30 00 00 00 00 41 c7 44 24 3c 00 00 00 00 49 63 45 20 8d 50 01 41 89 55 20 <4d> 89 24 c6 f0 41 ff 45 24 48 8b 83 e0 8
[   30.265962] RSP: 0018:ffffc9000050fe30 EFLAGS: 00010202
[   30.266285] RAX: ffffffffffffffff RBX: ffff888003a0cc00 RCX: 0000000000000000
[   30.266717] RDX: 0000000000000000 RSI: ffffffff826e03fa RDI: ffff8880041698c8
[   30.267154] RBP: ffff888003d86a00 R08: 0000000000000040 R09: 0000000000000000
[   30.267588] R10: ffff8880041698c0 R11: 0000000000000001 R12: ffff8880041698c0
[   30.268020] R13: ffff888005564d80 R14: 0000000000000010 R15: 0000000000000000
[   30.268452] FS:  00007fefe72ed540(0000) GS:ffff88807dc80000(0000) knlGS:0000000000000000
[   30.268941] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   30.269295] CR2: 0000000000000008 CR3: 0000000004798000 CR4: 00000000000006f0
[   30.269735] Call Trace:
[   30.269892]  <TASK>
[   30.270030]  ? __die+0x1e/0x60
[   30.270230]  ? page_fault_oops+0x17c/0x470
[   30.270487]  ? search_module_extables+0x14/0x50
[   30.270771]  ? exc_page_fault+0x68/0x150
[   30.271025]  ? asm_exc_page_fault+0x26/0x30
[   30.271285]  ? nbd_add_socket+0x16f/0x280
[   30.271538]  nbd_ioctl+0x2ec/0x430
[   30.271757]  blkdev_ioctl+0x131/0x270
[   30.271999]  __x64_sys_ioctl+0x92/0xd0
[   30.272235]  do_syscall_64+0x46/0xf0
[   30.272464]  entry_SYSCALL_64_after_hwframe+0x6f/0x77
[   30.272779] RIP: 0033:0x7fefe7209237
[   30.273004] Code: 00 00 00 48 8b 05 59 cc 0d 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 29 8
[   30.274131] RSP: 002b:00007ffca3831e08 EFLAGS: 00000202 ORIG_RAX: 0000000000000010
[   30.274590] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fefe7209237
[   30.275024] RDX: 0000000000000004 RSI: 000000000000ab00 RDI: 0000000000000003
[   30.275461] RBP: 00007ffca3831e10 R08: 0000000000000000 R09: 00007fefe73021b0
[   30.275904] R10: fffffffffffff4b6 R11: 0000000000000202 R12: 000055970d85a0a0
[   30.276341] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[   30.276772]  </TASK>
[   30.276913] Modules linked in:
[   30.277109] CR2: 0000000000000008
[   30.277336] ---[ end trace 0000000000000000 ]---
[   30.277631] RIP: 0010:nbd_add_socket+0x16f/0x280
[   30.278265] Code: 49 89 2c 24 49 c7 44 24 28 00 00 00 00 41 c7 44 24 30 00 00 00 00 41 c7 44 24 3c 00 00 00 00 49 63 45 20 8d 50 01 41 89 55 20 <4d> 89 24 c6 f0 41 ff 45 24 48 8b 83 e0 8
[   30.280435] RSP: 0018:ffffc9000050fe30 EFLAGS: 00010202
[   30.281064] RAX: ffffffffffffffff RBX: ffff888003a0cc00 RCX: 0000000000000000
[   30.281966] RDX: 0000000000000000 RSI: ffffffff826e03fa RDI: ffff8880041698c8
[   30.282830] RBP: ffff888003d86a00 R08: 0000000000000040 R09: 0000000000000000
[   30.283689] R10: ffff8880041698c0 R11: 0000000000000001 R12: ffff8880041698c0
[   30.284552] R13: ffff888005564d80 R14: 0000000000000010 R15: 0000000000000000
[   30.285430] FS:  00007fefe72ed540(0000) GS:ffff88807dc80000(0000) knlGS:0000000000000000
[   30.286519] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   30.287235] CR2: 0000000000000008 CR3: 0000000004798000 CR4: 00000000000006f0

nbd_add_socket (drivers/block/nbd.c):
...
socks = krealloc(config->socks, (config->num_connections + 1) *
sizeof(struct nbd_sock *), GFP_KERNEL); // out-of-memory
if (!socks) {
kfree(nsock);
err = -ENOMEM;
goto put_socket;
}

config->socks = socks;

nsock->fallback_index = -1;
nsock->dead = false;
mutex_init(&nsock->tx_lock);
nsock->sock = sock;
nsock->pending = NULL;
nsock->sent = 0;
nsock->cookie = 0;
socks[config->num_connections++] = nsock; // out-of-bounds or null-ptr-dereference
...

Thanks,

Hyeonjun

[Josef Bacik]

On Sun, Nov 5, 2023 at 9:33 PM Hyeonjun Ahn <guswn...@gmail.com> wrote:
>
>
> Dear Linux maintainer & Contributor.
>
> Hello, I'm Hyeonjun Ahn concerned with kernel security.
> I deleted the mail that I sent before because it was in a dark theme by mistake and you would have trouble reading it.
> Sorry for the inconvenience.
> I found an integer overflow bug in drivers/block/nbd.c.
> The absence of an integer overflow check for 'config->num_connections' in 'nbd_add_socket' prior to its incrementation poses a potential risk, as it can lead to following issues.
>
> 1. out-of-bounds when 'num_connections' reaches 0x80000000 (-2147483648) and the physical memory is large enough to be allocated by krealloc(ptr, 0x3fffffff8. GFP_KERNEL). (about 17 GB)
> 2. out-of-memory
> 3. null-pointer dereference and subsequent kernel panic when 'num_connections' reaches -1 and 'krealloc' returns a (void *)16, which corresponds to 'ZERO_SIZE_PTR'.
>

The path is acceptable, please follow the guide here
https://docs.kernel.org/process/submitting-patches.html for submitting
an actual fix.

I would just clone Jens's tree, make the change against that and
commit it. Then use git format-patch -1 which will create a properly
formatted email, and then you can send that email and I'll review it
and Jens can merge it. Thanks,

Josef

[Hyeonjun Ahn]

From dcd3d6cf9bfab6b664aee341779465977762a2d3 Mon Sep 17 00:00:00 2001

From: Hyeonjun Ahn <guswn...@gmail.com>

Date: Tue, 7 Nov 2023 11:40:52 +0900

Subject: [PATCH] add max_connections

---

 drivers/block/nbd.c | 15 +++++++++++++++

 1 file changed, 15 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c

index 800f131222fc..2cd41db20e31 100644

--- a/drivers/block/nbd.c

+++ b/drivers/block/nbd.c

@@ -162,6 +162,7 @@ static struct dentry *nbd_dbg_dir;

 static unsigned int nbds_max = 16;

 static int max_part = 16;

 static int part_shift;

+static unsigned long max_connections;

 

 static int nbd_dev_dbg_init(struct nbd_device *nbd);

 static void nbd_dev_dbg_close(struct nbd_device *nbd);

@@ -1117,6 +1118,14 @@ static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg,

    /* Arg will be cast to int, check it to avoid overflow */

    if (arg > INT_MAX)

        return -EINVAL;

+

+   if(config->num_connections >= max_connections)

+   {

+       dev_err(disk_to_dev(nbd->disk),

+           "Number of socket connections exceeded limit.\n");

+       return -ENOMEM;

+   }

+

    sock = nbd_get_socket(nbd, arg, &err);

    if (!sock)

        return err;

@@ -2482,6 +2491,11 @@ static void nbd_dead_link_work(struct work_struct *work)

    kfree(args);

 }

 

+static void set_max_connections(void)

+{

+   max_connections = PAGE_SIZE / sizeof(struct nbd_sock *);

+}

+

 static int __init nbd_init(void)

 {

    int i;

@@ -2529,6 +2543,7 @@ static int __init nbd_init(void)

        return -EINVAL;

    }

    nbd_dbg_init();

+   set_max_connections();

 

    for (i = 0; i < nbds_max; i++)

        nbd_dev_add(i, 1);

--

2.34.1

[Josef Bacik]

On Mon, Nov 6, 2023 at 10:07 PM Hyeonjun Ahn <guswn...@gmail.com> wrote:
>
> From dcd3d6cf9bfab6b664aee341779465977762a2d3 Mon Sep 17 00:00:00 2001
> From: Hyeonjun Ahn <guswn...@gmail.com>
> Date: Tue, 7 Nov 2023 11:40:52 +0900
> Subject: [PATCH] add max_connections
>
> ---
> drivers/block/nbd.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 800f131222fc..2cd41db20e31 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -162,6 +162,7 @@ static struct dentry *nbd_dbg_dir;
> static unsigned int nbds_max = 16;
> static int max_part = 16;
> static int part_shift;
> +static unsigned long max_connections;

I would just do

static unsigned long max_connections = PAGE_SIZE / sizeof(struct nbd_sock *);

and then leave the check below, not do the extra stuff.

Additionally you need to have a proper change log describing the
change, and change the title to something like

[PATCH] nbd: limit the number of connections per config

Thanks,

Josef

[Jens Axboe]

On 11/7/23 7:38 AM, Josef Bacik wrote:

Also CC the public lists, not sure why this is a private thing, this
isn't sensitive at all and just a root only thing for very contrived
conditions.

Outside of that, this:

if(config->num_connections >= max_connections)
+ {
+ dev_err(disk_to_dev(nbd->disk),
+ "Number of socket connections exceeded limit.\n");
+ return -ENOMEM;
+ }

is completely the wrong style. Should be:

if (config->num_connections >= max_connections) {
dev_err(disk_to_dev(nbd->disk),

"Number of socket connections exceeded limit.\n");

return -ENOMEM;
}

I kind of lost track on this a bit with a lot of different versions in
broken formats going through, the above is from reading the last one I
found.

--
Jens Axboe

[Hyeonjun Ahn]

Thanks for your reviews.

Below is the modified patch.

From 8a577c9579d24bb702c03dd012626326d3a4be19 Mon Sep 17 00:00:00 2001

From: Hyeonjun Ahn <guswn...@gmail.com>

Date: Wed, 8 Nov 2023 12:55:48 +0900

Subject: [PATCH] nbd: limit the number of connections per config

---

 drivers/block/nbd.c | 8 ++++++++

 1 file changed, 8 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c

index 800f131222fc..69f7fe0d07d6 100644

--- a/drivers/block/nbd.c

+++ b/drivers/block/nbd.c

@@ -162,6 +162,7 @@ static struct dentry *nbd_dbg_dir;

 static unsigned int nbds_max = 16;

 static int max_part = 16;

 static int part_shift;

+static unsigned long max_connections = PAGE_SIZE / sizeof(struct nbd_sock *);

 

 static int nbd_dev_dbg_init(struct nbd_device *nbd);

 static void nbd_dev_dbg_close(struct nbd_device *nbd);

@@ -1117,6 +1118,13 @@ static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg,

        /* Arg will be cast to int, check it to avoid overflow */

        if (arg > INT_MAX)

                return -EINVAL;

+

+       if (config->num_connections >= max_connections) {

+               dev_err(disk_to_dev(nbd->disk),

+                       "Number of socket connections exceeded limit.\n");

+               return -ENOMEM;

+       }

+

        sock = nbd_get_socket(nbd, arg, &err);

        if (!sock)

                return err;

--

2.34.1

[Jens Axboe]

On 11/7/23 9:30 PM, Hyeonjun Ahn wrote:
> Thanks for your reviews.
> Below is the modified patch.

Please send it with a CC to linux-block, as that is the proper list.
You'll need to ensure it's just plain text, looks like it's html right
now.

And also add a proper commit message.

--
Jens Axboe

[Hyeonjun Ahn]

From dcca9cecaa77cf362e694031de080540f55d6daf Mon Sep 17 00:00:00 2001
From: Hyeonjun Ahn <guswn...@gmail.com>
Date: Thu, 9 Nov 2023 11:41:02 +0900

Subject: [PATCH] nbd: limit the number of connections per config

Add max_connections to prevent out-of-memory in nbd_add_socket.

Signed-off-by: Hyeonjun Ahn <guswn...@gmail.com>
Reviewed-by: Josef Bacik <jo...@toxicpanda.com>
Reviewed-by: Jens Axboe <ax...@kernel.dk>

2023년 11월 9일 (목) 오전 12:34, Jens Axboe <ax...@kernel.dk>님이 작성: