[bpf,v3,1/5] bpf: Free dynamically allocated bits in bpf_iter_bits_destroy()

Commit Message

Hou Tao Oct. 25, 2024, 1:32 a.m. UTC

From: Hou Tao <[e-mail]>

bpf_iter_bits_destroy() uses "kit->nr_bits <= 64" to check whether the
bits are dynamically allocated. However, the check is incorrect and may
cause a kmemleak as shown below:

unreferenced object 0xffff88812628c8c0 (size 32):
  comm "swapper/0", pid 1, jiffies 4294727320
  hex dump (first 32 bytes):
	b0 c1 55 f5 81 88 ff ff f0 f0 f0 f0 f0 f0 f0 f0  ..U...........
	f0 f0 f0 f0 f0 f0 f0 f0 00 00 00 00 00 00 00 00  ..............
  backtrace (crc 781e32cc):
	[<00000000c452b4ab>] kmemleak_alloc+0x4b/0x80
	[<0000000004e09f80>] __kmalloc_node_noprof+0x480/0x5c0
	[<00000000597124d6>] __alloc.isra.0+0x89/0xb0
	[<000000004ebfffcd>] alloc_bulk+0x2af/0x720
	[<00000000d9c10145>] prefill_mem_cache+0x7f/0xb0
	[<00000000ff9738ff>] bpf_mem_alloc_init+0x3e2/0x610
	[<000000008b616eac>] bpf_global_ma_init+0x19/0x30
	[<00000000fc473efc>] do_one_initcall+0xd3/0x3c0
	[<00000000ec81498c>] kernel_init_freeable+0x66a/0x940
	[<00000000b119f72f>] kernel_init+0x20/0x160
	[<00000000f11ac9a7>] ret_from_fork+0x3c/0x70
	[<0000000004671da4>] ret_from_fork_asm+0x1a/0x30

That is because nr_bits will be set as zero in bpf_iter_bits_next()
after all bits have been iterated.

Fix the issue by setting kit->bit to kit->nr_bits instead of setting
kit->nr_bits to zero when the iteration completes in
bpf_iter_bits_next(). In addition, use "!nr_bits || bits >= nr_bits" to
check whether the iteration is complete and still use "nr_bits > 64" to
indicate whether bits are dynamically allocated. The "!nr_bits" check is
necessary because bpf_iter_bits_new() may fail before setting
kit->nr_bits, and this condition will stop the iteration early instead
of accessing the zeroed or freed kit->bits.

Considering the initial value of kit->bits is -1 and the type of
kit->nr_bits is unsigned int, change the type of kit->nr_bits to int.
The potential overflow problem will be handled in the following patch.

Fixes: 4665415975b0 ("bpf: Add bits iterator")
Signed-off-by: Hou Tao <[e-mail]>
---
 kernel/bpf/helpers.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Yafang Shao Oct. 25, 2024, 6:17 a.m. UTC | #1

On Fri, Oct 25, 2024 at 9:20 AM Hou Tao <[e-mail]> wrote:
>
> From: Hou Tao <[e-mail]>
>
> bpf_iter_bits_destroy() uses "kit->nr_bits <= 64" to check whether the
> bits are dynamically allocated. However, the check is incorrect and may
> cause a kmemleak as shown below:
>
> unreferenced object 0xffff88812628c8c0 (size 32):
>   comm "swapper/0", pid 1, jiffies 4294727320
>   hex dump (first 32 bytes):
>         b0 c1 55 f5 81 88 ff ff f0 f0 f0 f0 f0 f0 f0 f0  ..U...........
>         f0 f0 f0 f0 f0 f0 f0 f0 00 00 00 00 00 00 00 00  ..............
>   backtrace (crc 781e32cc):
>         [<00000000c452b4ab>] kmemleak_alloc+0x4b/0x80
>         [<0000000004e09f80>] __kmalloc_node_noprof+0x480/0x5c0
>         [<00000000597124d6>] __alloc.isra.0+0x89/0xb0
>         [<000000004ebfffcd>] alloc_bulk+0x2af/0x720
>         [<00000000d9c10145>] prefill_mem_cache+0x7f/0xb0
>         [<00000000ff9738ff>] bpf_mem_alloc_init+0x3e2/0x610
>         [<000000008b616eac>] bpf_global_ma_init+0x19/0x30
>         [<00000000fc473efc>] do_one_initcall+0xd3/0x3c0
>         [<00000000ec81498c>] kernel_init_freeable+0x66a/0x940
>         [<00000000b119f72f>] kernel_init+0x20/0x160
>         [<00000000f11ac9a7>] ret_from_fork+0x3c/0x70
>         [<0000000004671da4>] ret_from_fork_asm+0x1a/0x30
>
> That is because nr_bits will be set as zero in bpf_iter_bits_next()
> after all bits have been iterated.
>
> Fix the issue by setting kit->bit to kit->nr_bits instead of setting
> kit->nr_bits to zero when the iteration completes in
> bpf_iter_bits_next(). In addition, use "!nr_bits || bits >= nr_bits" to
> check whether the iteration is complete and still use "nr_bits > 64" to
> indicate whether bits are dynamically allocated. The "!nr_bits" check is
> necessary because bpf_iter_bits_new() may fail before setting
> kit->nr_bits, and this condition will stop the iteration early instead
> of accessing the zeroed or freed kit->bits.
>
> Considering the initial value of kit->bits is -1 and the type of
> kit->nr_bits is unsigned int, change the type of kit->nr_bits to int.
> The potential overflow problem will be handled in the following patch.
>
> Fixes: 4665415975b0 ("bpf: Add bits iterator")
> Signed-off-by: Hou Tao <[e-mail]>

LGTM
Acked-by: Yafang Shao <[e-mail]>

> ---
>  kernel/bpf/helpers.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 1a43d06eab28..40ef6a56619f 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2856,7 +2856,7 @@ struct bpf_iter_bits_kern {
>                 unsigned long *bits;
>                 unsigned long bits_copy;
>         };
> -       u32 nr_bits;
> +       int nr_bits;
>         int bit;
>  } __aligned(8);
>
> @@ -2930,17 +2930,16 @@ bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w
>  __bpf_kfunc int *bpf_iter_bits_next(struct bpf_iter_bits *it)
>  {
>         struct bpf_iter_bits_kern *kit = (void *)it;
> -       u32 nr_bits = kit->nr_bits;
> +       int bit = kit->bit, nr_bits = kit->nr_bits;
>         const unsigned long *bits;
> -       int bit;
>
> -       if (nr_bits == 0)
> +       if (!nr_bits || bit >= nr_bits)
>                 return NULL;
>
>         bits = nr_bits == 64 ? &kit->bits_copy : kit->bits;
> -       bit = find_next_bit(bits, nr_bits, kit->bit + 1);
> +       bit = find_next_bit(bits, nr_bits, bit + 1);
>         if (bit >= nr_bits) {
> -               kit->nr_bits = 0;
> +               kit->bit = bit;
>                 return NULL;
>         }
>
> --
> 2.29.2
>

Patch

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 1a43d06eab28..40ef6a56619f 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2856,7 +2856,7 @@  struct bpf_iter_bits_kern {
 		unsigned long *bits;
 		unsigned long bits_copy;
 	};
-	u32 nr_bits;
+	int nr_bits;
 	int bit;
 } __aligned(8);
 
@@ -2930,17 +2930,16 @@  bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w
 __bpf_kfunc int *bpf_iter_bits_next(struct bpf_iter_bits *it)
 {
 	struct bpf_iter_bits_kern *kit = (void *)it;
-	u32 nr_bits = kit->nr_bits;
+	int bit = kit->bit, nr_bits = kit->nr_bits;
 	const unsigned long *bits;
-	int bit;
 
-	if (nr_bits == 0)
+	if (!nr_bits || bit >= nr_bits)
 		return NULL;
 
 	bits = nr_bits == 64 ? &kit->bits_copy : kit->bits;
-	bit = find_next_bit(bits, nr_bits, kit->bit + 1);
+	bit = find_next_bit(bits, nr_bits, bit + 1);
 	if (bit >= nr_bits) {
-		kit->nr_bits = 0;
+		kit->bit = bit;
 		return NULL;
 	}