Message ID | 20241025013233.804027-4-houtao@huaweicloud.com (mailing list archive) |
---|---|
State | New |
Delegated to: | BPF |
Headers | show |
Series | Fixes for bits iterator | expand |
On Fri, Oct 25, 2024 at 9:20 AM Hou Tao <houtao@huaweicloud.com> wrote: > > From: Hou Tao <houtao1@huawei.com> > > Check the validity of nr_words in bpf_iter_bits_new(). Without this > check, when multiplication overflow occurs for nr_bits (e.g., when > nr_words = 0x0400-0001, nr_bits becomes 64), stack corruption may occur > due to bpf_probe_read_kernel_common(..., nr_bytes = 0x2000-0008). > > Fix it by limiting the maximum value of nr_words to 511. The value is > derived from the current implementation of BPF memory allocator. To > ensure compatibility if the BPF memory allocator's size limitation > changes in the future, use the helper bpf_mem_alloc_check_size() to > check whether nr_bytes is too larger. And return -E2BIG instead of > -ENOMEM for oversized nr_bytes. > > Fixes: 4665415975b0 ("bpf: Add bits iterator") > Signed-off-by: Hou Tao <houtao1@huawei.com> > --- > kernel/bpf/helpers.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index 40ef6a56619f..daec74820dbe 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -2851,6 +2851,8 @@ struct bpf_iter_bits { > __u64 __opaque[2]; > } __aligned(8); > > +#define BITS_ITER_NR_WORDS_MAX 511 > + > struct bpf_iter_bits_kern { > union { > unsigned long *bits; > @@ -2865,7 +2867,8 @@ struct bpf_iter_bits_kern { > * @it: The new bpf_iter_bits to be created > * @unsafe_ptr__ign: A pointer pointing to a memory area to be iterated over > * @nr_words: The size of the specified memory area, measured in 8-byte units. > - * Due to the limitation of memalloc, it can't be greater than 512. > + * The maximum value of @nr_words is @BITS_ITER_NR_WORDS_MAX. This limit may be > + * further reduced by the BPF memory allocator implementation. > * > * This function initializes a new bpf_iter_bits structure for iterating over > * a memory area which is specified by the @unsafe_ptr__ign and @nr_words. It > @@ -2878,8 +2881,7 @@ __bpf_kfunc int > bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_words) > { > struct bpf_iter_bits_kern *kit = (void *)it; > - u32 nr_bytes = nr_words * sizeof(u64); > - u32 nr_bits = BYTES_TO_BITS(nr_bytes); > + u32 nr_bytes, nr_bits; > int err; > > BUILD_BUG_ON(sizeof(struct bpf_iter_bits_kern) != sizeof(struct bpf_iter_bits)); > @@ -2892,9 +2894,14 @@ bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w > > if (!unsafe_ptr__ign || !nr_words) > return -EINVAL; > + if (nr_words > BITS_ITER_NR_WORDS_MAX) > + return -E2BIG; > + > + nr_bytes = nr_words * sizeof(u64); > + nr_bits = BYTES_TO_BITS(nr_bytes); > > /* Optimization for u64 mask */ > - if (nr_bits == 64) { > + if (nr_words == 1) { > err = bpf_probe_read_kernel_common(&kit->bits_copy, nr_bytes, unsafe_ptr__ign); > if (err) > return -EFAULT; > @@ -2903,6 +2910,9 @@ bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w > return 0; > } > > + if (bpf_mem_alloc_check_size(false, nr_bytes)) > + return -E2BIG; > + Is this check necessary here? If `E2BIG` is a concern, perhaps it would be more appropriate to return it using ERR_PTR() in bpf_mem_alloc()? > /* Fallback to memalloc */ > kit->bits = bpf_mem_alloc(&bpf_global_ma, nr_bytes); > if (!kit->bits) > -- > 2.29.2 >
Hi Yafang, On 10/25/2024 2:04 PM, Yafang Shao wrote: > On Fri, Oct 25, 2024 at 9:20 AM Hou Tao <houtao@huaweicloud.com> wrote: >> From: Hou Tao <houtao1@huawei.com> >> >> Check the validity of nr_words in bpf_iter_bits_new(). Without this >> check, when multiplication overflow occurs for nr_bits (e.g., when >> nr_words = 0x0400-0001, nr_bits becomes 64), stack corruption may occur >> due to bpf_probe_read_kernel_common(..., nr_bytes = 0x2000-0008). >> >> Fix it by limiting the maximum value of nr_words to 511. The value is >> derived from the current implementation of BPF memory allocator. To >> ensure compatibility if the BPF memory allocator's size limitation >> changes in the future, use the helper bpf_mem_alloc_check_size() to >> check whether nr_bytes is too larger. And return -E2BIG instead of >> -ENOMEM for oversized nr_bytes. >> >> Fixes: 4665415975b0 ("bpf: Add bits iterator") >> Signed-off-by: Hou Tao <houtao1@huawei.com> >> --- >> kernel/bpf/helpers.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c >> index 40ef6a56619f..daec74820dbe 100644 >> --- a/kernel/bpf/helpers.c >> +++ b/kernel/bpf/helpers.c >> @@ -2851,6 +2851,8 @@ struct bpf_iter_bits { >> __u64 __opaque[2]; >> } __aligned(8); >> >> +#define BITS_ITER_NR_WORDS_MAX 511 >> + >> struct bpf_iter_bits_kern { >> union { >> unsigned long *bits; >> @@ -2865,7 +2867,8 @@ struct bpf_iter_bits_kern { >> * @it: The new bpf_iter_bits to be created >> * @unsafe_ptr__ign: A pointer pointing to a memory area to be iterated over >> * @nr_words: The size of the specified memory area, measured in 8-byte units. >> - * Due to the limitation of memalloc, it can't be greater than 512. >> + * The maximum value of @nr_words is @BITS_ITER_NR_WORDS_MAX. This limit may be >> + * further reduced by the BPF memory allocator implementation. >> * >> * This function initializes a new bpf_iter_bits structure for iterating over >> * a memory area which is specified by the @unsafe_ptr__ign and @nr_words. It >> @@ -2878,8 +2881,7 @@ __bpf_kfunc int >> bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_words) >> { >> struct bpf_iter_bits_kern *kit = (void *)it; >> - u32 nr_bytes = nr_words * sizeof(u64); >> - u32 nr_bits = BYTES_TO_BITS(nr_bytes); >> + u32 nr_bytes, nr_bits; >> int err; >> >> BUILD_BUG_ON(sizeof(struct bpf_iter_bits_kern) != sizeof(struct bpf_iter_bits)); >> @@ -2892,9 +2894,14 @@ bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w >> >> if (!unsafe_ptr__ign || !nr_words) >> return -EINVAL; >> + if (nr_words > BITS_ITER_NR_WORDS_MAX) >> + return -E2BIG; >> + >> + nr_bytes = nr_words * sizeof(u64); >> + nr_bits = BYTES_TO_BITS(nr_bytes); >> >> /* Optimization for u64 mask */ >> - if (nr_bits == 64) { >> + if (nr_words == 1) { >> err = bpf_probe_read_kernel_common(&kit->bits_copy, nr_bytes, unsafe_ptr__ign); >> if (err) >> return -EFAULT; >> @@ -2903,6 +2910,9 @@ bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w >> return 0; >> } >> >> + if (bpf_mem_alloc_check_size(false, nr_bytes)) >> + return -E2BIG; >> + > Is this check necessary here? If `E2BIG` is a concern, perhaps it > would be more appropriate to return it using ERR_PTR() in > bpf_mem_alloc()? The check is necessary to ensure a correct error code is returned. Returning ERR_PTR() in bpf_mem_alloc() is also feasible, but the return value of bpf_mem_alloc() and bpf_mem_cache_alloc() will be different, so I prefer to introduce an extra helper for the size checking. >> /* Fallback to memalloc */ >> kit->bits = bpf_mem_alloc(&bpf_global_ma, nr_bytes); >> if (!kit->bits) >> -- >> 2.29.2 >> >
On Fri, Oct 25, 2024 at 3:52 PM Hou Tao <houtao@huaweicloud.com> wrote: > > Hi Yafang, > > On 10/25/2024 2:04 PM, Yafang Shao wrote: > > On Fri, Oct 25, 2024 at 9:20 AM Hou Tao <houtao@huaweicloud.com> wrote: > >> From: Hou Tao <houtao1@huawei.com> > >> > >> Check the validity of nr_words in bpf_iter_bits_new(). Without this > >> check, when multiplication overflow occurs for nr_bits (e.g., when > >> nr_words = 0x0400-0001, nr_bits becomes 64), stack corruption may occur > >> due to bpf_probe_read_kernel_common(..., nr_bytes = 0x2000-0008). > >> > >> Fix it by limiting the maximum value of nr_words to 511. The value is > >> derived from the current implementation of BPF memory allocator. To > >> ensure compatibility if the BPF memory allocator's size limitation > >> changes in the future, use the helper bpf_mem_alloc_check_size() to > >> check whether nr_bytes is too larger. And return -E2BIG instead of > >> -ENOMEM for oversized nr_bytes. > >> > >> Fixes: 4665415975b0 ("bpf: Add bits iterator") > >> Signed-off-by: Hou Tao <houtao1@huawei.com> > >> --- > >> kernel/bpf/helpers.c | 18 ++++++++++++++---- > >> 1 file changed, 14 insertions(+), 4 deletions(-) > >> > >> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > >> index 40ef6a56619f..daec74820dbe 100644 > >> --- a/kernel/bpf/helpers.c > >> +++ b/kernel/bpf/helpers.c > >> @@ -2851,6 +2851,8 @@ struct bpf_iter_bits { > >> __u64 __opaque[2]; > >> } __aligned(8); > >> > >> +#define BITS_ITER_NR_WORDS_MAX 511 > >> + > >> struct bpf_iter_bits_kern { > >> union { > >> unsigned long *bits; > >> @@ -2865,7 +2867,8 @@ struct bpf_iter_bits_kern { > >> * @it: The new bpf_iter_bits to be created > >> * @unsafe_ptr__ign: A pointer pointing to a memory area to be iterated over > >> * @nr_words: The size of the specified memory area, measured in 8-byte units. > >> - * Due to the limitation of memalloc, it can't be greater than 512. > >> + * The maximum value of @nr_words is @BITS_ITER_NR_WORDS_MAX. This limit may be > >> + * further reduced by the BPF memory allocator implementation. > >> * > >> * This function initializes a new bpf_iter_bits structure for iterating over > >> * a memory area which is specified by the @unsafe_ptr__ign and @nr_words. It > >> @@ -2878,8 +2881,7 @@ __bpf_kfunc int > >> bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_words) > >> { > >> struct bpf_iter_bits_kern *kit = (void *)it; > >> - u32 nr_bytes = nr_words * sizeof(u64); > >> - u32 nr_bits = BYTES_TO_BITS(nr_bytes); > >> + u32 nr_bytes, nr_bits; > >> int err; > >> > >> BUILD_BUG_ON(sizeof(struct bpf_iter_bits_kern) != sizeof(struct bpf_iter_bits)); > >> @@ -2892,9 +2894,14 @@ bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w > >> > >> if (!unsafe_ptr__ign || !nr_words) > >> return -EINVAL; > >> + if (nr_words > BITS_ITER_NR_WORDS_MAX) > >> + return -E2BIG; > >> + > >> + nr_bytes = nr_words * sizeof(u64); > >> + nr_bits = BYTES_TO_BITS(nr_bytes); > >> > >> /* Optimization for u64 mask */ > >> - if (nr_bits == 64) { > >> + if (nr_words == 1) { > >> err = bpf_probe_read_kernel_common(&kit->bits_copy, nr_bytes, unsafe_ptr__ign); > >> if (err) > >> return -EFAULT; > >> @@ -2903,6 +2910,9 @@ bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w > >> return 0; > >> } > >> > >> + if (bpf_mem_alloc_check_size(false, nr_bytes)) > >> + return -E2BIG; > >> + > > Is this check necessary here? If `E2BIG` is a concern, perhaps it > > would be more appropriate to return it using ERR_PTR() in > > bpf_mem_alloc()? > > The check is necessary to ensure a correct error code is returned. > Returning ERR_PTR() in bpf_mem_alloc() is also feasible, but the return > value of bpf_mem_alloc() and bpf_mem_cache_alloc() will be different, so > I prefer to introduce an extra helper for the size checking. Perhaps we should refactor the return values of both bpf_mem_alloc() and bpf_mem_cache_alloc() to return more appropriate error codes, such as -E2BIG, -ENOMEM, and -EINVAL. However, this change would be better addressed in a separate patchset.
On Fri, Oct 25, 2024 at 6:29 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > On Fri, Oct 25, 2024 at 3:52 PM Hou Tao <houtao@huaweicloud.com> wrote: > > > > Hi Yafang, > > > > On 10/25/2024 2:04 PM, Yafang Shao wrote: > > > On Fri, Oct 25, 2024 at 9:20 AM Hou Tao <houtao@huaweicloud.com> wrote: > > >> From: Hou Tao <houtao1@huawei.com> > > >> > > >> Check the validity of nr_words in bpf_iter_bits_new(). Without this > > >> check, when multiplication overflow occurs for nr_bits (e.g., when > > >> nr_words = 0x0400-0001, nr_bits becomes 64), stack corruption may occur > > >> due to bpf_probe_read_kernel_common(..., nr_bytes = 0x2000-0008). > > >> > > >> Fix it by limiting the maximum value of nr_words to 511. The value is > > >> derived from the current implementation of BPF memory allocator. To > > >> ensure compatibility if the BPF memory allocator's size limitation > > >> changes in the future, use the helper bpf_mem_alloc_check_size() to > > >> check whether nr_bytes is too larger. And return -E2BIG instead of > > >> -ENOMEM for oversized nr_bytes. > > >> > > >> Fixes: 4665415975b0 ("bpf: Add bits iterator") > > >> Signed-off-by: Hou Tao <houtao1@huawei.com> > > >> --- > > >> kernel/bpf/helpers.c | 18 ++++++++++++++---- > > >> 1 file changed, 14 insertions(+), 4 deletions(-) > > >> > > >> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > > >> index 40ef6a56619f..daec74820dbe 100644 > > >> --- a/kernel/bpf/helpers.c > > >> +++ b/kernel/bpf/helpers.c > > >> @@ -2851,6 +2851,8 @@ struct bpf_iter_bits { > > >> __u64 __opaque[2]; > > >> } __aligned(8); > > >> > > >> +#define BITS_ITER_NR_WORDS_MAX 511 > > >> + > > >> struct bpf_iter_bits_kern { > > >> union { > > >> unsigned long *bits; > > >> @@ -2865,7 +2867,8 @@ struct bpf_iter_bits_kern { > > >> * @it: The new bpf_iter_bits to be created > > >> * @unsafe_ptr__ign: A pointer pointing to a memory area to be iterated over > > >> * @nr_words: The size of the specified memory area, measured in 8-byte units. > > >> - * Due to the limitation of memalloc, it can't be greater than 512. > > >> + * The maximum value of @nr_words is @BITS_ITER_NR_WORDS_MAX. This limit may be > > >> + * further reduced by the BPF memory allocator implementation. > > >> * > > >> * This function initializes a new bpf_iter_bits structure for iterating over > > >> * a memory area which is specified by the @unsafe_ptr__ign and @nr_words. It > > >> @@ -2878,8 +2881,7 @@ __bpf_kfunc int > > >> bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_words) > > >> { > > >> struct bpf_iter_bits_kern *kit = (void *)it; > > >> - u32 nr_bytes = nr_words * sizeof(u64); > > >> - u32 nr_bits = BYTES_TO_BITS(nr_bytes); > > >> + u32 nr_bytes, nr_bits; > > >> int err; > > >> > > >> BUILD_BUG_ON(sizeof(struct bpf_iter_bits_kern) != sizeof(struct bpf_iter_bits)); > > >> @@ -2892,9 +2894,14 @@ bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w > > >> > > >> if (!unsafe_ptr__ign || !nr_words) > > >> return -EINVAL; > > >> + if (nr_words > BITS_ITER_NR_WORDS_MAX) > > >> + return -E2BIG; > > >> + > > >> + nr_bytes = nr_words * sizeof(u64); > > >> + nr_bits = BYTES_TO_BITS(nr_bytes); > > >> > > >> /* Optimization for u64 mask */ > > >> - if (nr_bits == 64) { > > >> + if (nr_words == 1) { > > >> err = bpf_probe_read_kernel_common(&kit->bits_copy, nr_bytes, unsafe_ptr__ign); > > >> if (err) > > >> return -EFAULT; > > >> @@ -2903,6 +2910,9 @@ bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w > > >> return 0; > > >> } > > >> > > >> + if (bpf_mem_alloc_check_size(false, nr_bytes)) > > >> + return -E2BIG; > > >> + > > > Is this check necessary here? If `E2BIG` is a concern, perhaps it > > > would be more appropriate to return it using ERR_PTR() in > > > bpf_mem_alloc()? > > > > The check is necessary to ensure a correct error code is returned. > > Returning ERR_PTR() in bpf_mem_alloc() is also feasible, but the return > > value of bpf_mem_alloc() and bpf_mem_cache_alloc() will be different, so > > I prefer to introduce an extra helper for the size checking. > > Perhaps we should refactor the return values of both bpf_mem_alloc() > and bpf_mem_cache_alloc() to return more appropriate error codes, such > as -E2BIG, -ENOMEM, and -EINVAL. However, this change would be better > addressed in a separate patchset. No. bpf_mem_alloc() returns NULL or valid and will stay this way.
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 40ef6a56619f..daec74820dbe 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -2851,6 +2851,8 @@ struct bpf_iter_bits { __u64 __opaque[2]; } __aligned(8); +#define BITS_ITER_NR_WORDS_MAX 511 + struct bpf_iter_bits_kern { union { unsigned long *bits; @@ -2865,7 +2867,8 @@ struct bpf_iter_bits_kern { * @it: The new bpf_iter_bits to be created * @unsafe_ptr__ign: A pointer pointing to a memory area to be iterated over * @nr_words: The size of the specified memory area, measured in 8-byte units. - * Due to the limitation of memalloc, it can't be greater than 512. + * The maximum value of @nr_words is @BITS_ITER_NR_WORDS_MAX. This limit may be + * further reduced by the BPF memory allocator implementation. * * This function initializes a new bpf_iter_bits structure for iterating over * a memory area which is specified by the @unsafe_ptr__ign and @nr_words. It @@ -2878,8 +2881,7 @@ __bpf_kfunc int bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_words) { struct bpf_iter_bits_kern *kit = (void *)it; - u32 nr_bytes = nr_words * sizeof(u64); - u32 nr_bits = BYTES_TO_BITS(nr_bytes); + u32 nr_bytes, nr_bits; int err; BUILD_BUG_ON(sizeof(struct bpf_iter_bits_kern) != sizeof(struct bpf_iter_bits)); @@ -2892,9 +2894,14 @@ bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w if (!unsafe_ptr__ign || !nr_words) return -EINVAL; + if (nr_words > BITS_ITER_NR_WORDS_MAX) + return -E2BIG; + + nr_bytes = nr_words * sizeof(u64); + nr_bits = BYTES_TO_BITS(nr_bytes); /* Optimization for u64 mask */ - if (nr_bits == 64) { + if (nr_words == 1) { err = bpf_probe_read_kernel_common(&kit->bits_copy, nr_bytes, unsafe_ptr__ign); if (err) return -EFAULT; @@ -2903,6 +2910,9 @@ bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w return 0; } + if (bpf_mem_alloc_check_size(false, nr_bytes)) + return -E2BIG; + /* Fallback to memalloc */ kit->bits = bpf_mem_alloc(&bpf_global_ma, nr_bytes); if (!kit->bits)