[bpf,v2,6/7] bpf: Use __u64 to save the bits in bits iterator

Commit Message

Hou Tao Oct. 21, 2024, 1:40 a.m. UTC

From: Hou Tao <[e-mail]>

On 32-bit hosts (e.g., arm32), when a bpf program passes a u64 to
bpf_iter_bits_new(), bpf_iter_bits_new() will use bits_copy to store the
content of the u64. However, bits_copy is only 4 bytes, leading to stack
corruption.

The straightforward solution would be to replace u64 with unsigned long
in bpf_iter_bits_new(). However, this introduces confusion and problems
for 32-bit hosts because the size of ulong in bpf program is 8 bytes,
but it is treated as 4-bytes after passed to bpf_iter_bits_new().

Fix it by changing the type of both bits and bit_count from unsigned
long to u64. However, the change is not enough. The main reason is that
bpf_iter_bits_next() uses find_next_bit() to find the next bit and the
pointer passed to find_next_bit() is an unsigned long pointer instead
of a u64 pointer. For 32-bit little-endian host, it is fine but it is
not the case for 32-bit big-endian host. Because under 32-bit big-endian
host, the first iterated unsigned long will be the bits 32-63 of the u64
instead of the expected bits 0-31. Therefore, in addition to changing
the type, swap the two unsigned longs within the u64 for 32-bit
big-endian host.

Signed-off-by: Hou Tao <[e-mail]>
---
 kernel/bpf/helpers.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

Comments

Yafang Shao Oct. 23, 2024, 3:10 a.m. UTC | #1

On Mon, Oct 21, 2024 at 9:28 AM Hou Tao <[e-mail]> wrote:
>
> From: Hou Tao <[e-mail]>
>
> On 32-bit hosts (e.g., arm32), when a bpf program passes a u64 to
> bpf_iter_bits_new(), bpf_iter_bits_new() will use bits_copy to store the
> content of the u64. However, bits_copy is only 4 bytes, leading to stack
> corruption.
>
> The straightforward solution would be to replace u64 with unsigned long
> in bpf_iter_bits_new(). However, this introduces confusion and problems
> for 32-bit hosts because the size of ulong in bpf program is 8 bytes,
> but it is treated as 4-bytes after passed to bpf_iter_bits_new().
>
> Fix it by changing the type of both bits and bit_count from unsigned
> long to u64.

Thank you for the fix. This change is necessary.

>  However, the change is not enough. The main reason is that
> bpf_iter_bits_next() uses find_next_bit() to find the next bit and the
> pointer passed to find_next_bit() is an unsigned long pointer instead
> of a u64 pointer. For 32-bit little-endian host, it is fine but it is
> not the case for 32-bit big-endian host. Because under 32-bit big-endian
> host, the first iterated unsigned long will be the bits 32-63 of the u64
> instead of the expected bits 0-31. Therefore, in addition to changing
> the type, swap the two unsigned longs within the u64 for 32-bit
> big-endian host.

The API uses a u64 data type, and the nr_words parameter represents
the number of 8-byte units. On a 32-bit system, if you want to call
this API, you would define an array like `u32 data[2]` and invoke the
function as `bpf_for_each(bits, bit, &data[0], 1)`. However, since the
API expects a u64, you'll need to merge the two u32 values into a
single u64 value.

Given this, it might be more appropriate to ask users to handle the
u32 to u64 merge on their side when preparing the data, rather than
performing the swap within the kernel itself.

--
Regards

Yafang

Hou Tao Oct. 23, 2024, 8:09 a.m. UTC | #2

Hi,

On 10/23/2024 11:10 AM, Yafang Shao wrote:
> On Mon, Oct 21, 2024 at 9:28 AM Hou Tao <[e-mail]> wrote:
>> From: Hou Tao <[e-mail]>
>>
>> On 32-bit hosts (e.g., arm32), when a bpf program passes a u64 to
>> bpf_iter_bits_new(), bpf_iter_bits_new() will use bits_copy to store the
>> content of the u64. However, bits_copy is only 4 bytes, leading to stack
>> corruption.
>>
>> The straightforward solution would be to replace u64 with unsigned long
>> in bpf_iter_bits_new(). However, this introduces confusion and problems
>> for 32-bit hosts because the size of ulong in bpf program is 8 bytes,
>> but it is treated as 4-bytes after passed to bpf_iter_bits_new().
>>
>> Fix it by changing the type of both bits and bit_count from unsigned
>> long to u64.
> Thank you for the fix. This change is necessary.
>
>>  However, the change is not enough. The main reason is that
>> bpf_iter_bits_next() uses find_next_bit() to find the next bit and the
>> pointer passed to find_next_bit() is an unsigned long pointer instead
>> of a u64 pointer. For 32-bit little-endian host, it is fine but it is
>> not the case for 32-bit big-endian host. Because under 32-bit big-endian
>> host, the first iterated unsigned long will be the bits 32-63 of the u64
>> instead of the expected bits 0-31. Therefore, in addition to changing
>> the type, swap the two unsigned longs within the u64 for 32-bit
>> big-endian host.
> The API uses a u64 data type, and the nr_words parameter represents
> the number of 8-byte units. On a 32-bit system, if you want to call
> this API, you would define an array like `u32 data[2]` and invoke the
> function as `bpf_for_each(bits, bit, &data[0], 1)`. However, since the
> API expects a u64, you'll need to merge the two u32 values into a
> single u64 value.

It is a bit weird to pass a u32 pointer to bpf_for_each, because it
expects a u64 pointer. I think the bpf program should pass a u64 pointer
instead.
>
> Given this, it might be more appropriate to ask users to handle the
> u32 to u64 merge on their side when preparing the data, rather than
> performing the swap within the kernel itself.

However, the swap implemented in the patch has nothing to do whether the
user passes pointer to u32 array or not. It is necessary due to the
mismatched between the pointer type used by find_next_bit and the
pointer used by bpf_iter_bits_new(). The latter uses u64 pointer, but
find_next_bit() uses unsigned long pointer and iterates a long each
time. So just like the comment in the patch said:

    under 32-bit big-endian host, the first iterated unsigned long will
be the bits 32-63 of the u64 instead of the expected bits 0-31.

The swap in the patch will swap the two long values in the u64 and make
the first iterated unsigned long will be the bits 0-31 of the u64 value.
>
> --
> Regards
>
> Yafang

Patch

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index c147f75e1b48..0ad85201fb84 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2855,13 +2855,36 @@  struct bpf_iter_bits {
 
 struct bpf_iter_bits_kern {
 	union {
-		unsigned long *bits;
-		unsigned long bits_copy;
+		__u64 *bits;
+		__u64 bits_copy;
 	};
 	u32 nr_bits;
 	int bit;
 } __aligned(8);
 
+/* On 64-bit hosts, unsigned long and u64 have the same size, so passing
+ * a u64 pointer and an unsigned long pointer to find_next_bit() will
+ * return the same result, as both point to the same 8-byte area.
+ *
+ * For 32-bit little-endian hosts, using a u64 pointer or unsigned long
+ * pointer also makes no difference. This is because the first iterated
+ * unsigned long is composed of bits 0-31 of the u64 and the second unsigned
+ * long is composed of bits 32-63 of the u64.
+ *
+ * However, for 32-bit big-endian hosts, this is not the case. The first
+ * iterated unsigned long will be bits 32-63 of the u64, so swap these two
+ * ulong values within the u64.
+ */
+static void swap_ulong_in_u64(u64 *bits, unsigned int nr)
+{
+#if !defined(CONFIG_64BIT) && defined(__BIG_ENDIAN)
+	unsigned int i;
+
+	for (i = 0; i < nr; i++)
+		bits[i] = (bits[i] >> 32) | ((u64)(u32)bits[i] << 32);
+#endif
+}
+
 /**
  * bpf_iter_bits_new() - Initialize a new bits iterator for a given memory area
  * @it: The new bpf_iter_bits to be created
@@ -2903,6 +2926,8 @@  bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w
 		if (err)
 			return -EFAULT;
 
+		swap_ulong_in_u64(&kit->bits_copy, nr_words);
+
 		kit->nr_bits = nr_bits;
 		return 0;
 	}
@@ -2918,6 +2943,8 @@  bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w
 		return err;
 	}
 
+	swap_ulong_in_u64(kit->bits, nr_words);
+
 	kit->nr_bits = nr_bits;
 	return 0;
 }
@@ -2935,7 +2962,7 @@  __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;
-	const unsigned long *bits;
+	const void *bits;
 	int bit;
 
 	if (kit->bit >= nr_bits)