diff mbox series

[bpf,v3,3/5] bpf: Check the validity of nr_words in bpf_iter_bits_new()

Message ID 20241025013233.804027-4-houtao@huaweicloud.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Fixes for bits iterator | expand

Checks

Context Check Description
bpf/vmtest-bpf-PR success PR summary
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 5 this patch: 6
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 14 of 14 maintainers
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 58 this patch: 58
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 50 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 10 this patch: 10
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-13 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-15 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-10 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-7 success Logs for s390x-gcc / build-release

Commit Message

Hou Tao Oct. 25, 2024, 1:32 a.m. UTC 
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(-)

Comments

Yafang Shao Oct. 25, 2024, 6:04 a.m. UTC | #1 
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
>
Hou Tao Oct. 25, 2024, 7:52 a.m. UTC | #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
>>
>
Yafang Shao Oct. 25, 2024, 1:28 p.m. UTC | #3 
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.
Alexei Starovoitov Oct. 25, 2024, 3:45 p.m. UTC | #4 
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.
Alexei Starovoitov Oct. 29, 2024, 8:53 p.m. UTC | #5 
On Thu, Oct 24, 2024 at 6:20 PM 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);

The check for nr_words is good, but moving computation after 'if'
feels like code churn and nothing else.
Even if nr_words is large, it's fine to do the math.

>
>         /* Optimization for u64 mask */
> -       if (nr_bits == 64) {
> +       if (nr_words == 1) {

This is also unnecessary churn.

Also it seems it's causing a warn on 32-bit:
https://netdev.bots.linux.dev/static/nipa/902902/13849894/build_32bit/

pw-bot: cr

>                 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)
> --
> 2.29.2
>
Hou Tao Oct. 30, 2024, 1:45 a.m. UTC | #6 
Hi,

On 10/30/2024 4:53 AM, Alexei Starovoitov wrote:
> On Thu, Oct 24, 2024 at 6:20 PM 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.
>>

SNIP
>>  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);
> The check for nr_words is good, but moving computation after 'if'
> feels like code churn and nothing else.
> Even if nr_words is large, it's fine to do the math.

No intention for code churn. I thought the overflow during
multiplication may lead to UBSAN warning, but it seems the overflow
warning is only for signed integer. Andrii also suggested me to move the
assignment after the check [1].

[1]:
https://lore.kernel.org/bpf/CAEf4BzahtDCZDEdejm6cNMzDNTc0gXPzhc5xcWg9c8S_i6yWNA@mail.gmail.com/
>
>>         /* Optimization for u64 mask */
>> -       if (nr_bits == 64) {
>> +       if (nr_words == 1) {
> This is also unnecessary churn.
>
> Also it seems it's causing a warn on 32-bit:
> https://netdev.bots.linux.dev/static/nipa/902902/13849894/build_32bit/

It is weird that using "nr_bits = 64" doesn't reproduce the warning.
Because the warning is due to the size of bits_copy is 4 bytes under
32-bit host and the error path of bpf_probe_read_kernel_common() invokes
memset(&bits_copy, 0, 8). The warning will be fixed by the following
patch "bpf: Use __u64 to save the bits in bits iterator". Anyway, will
change it back to "nr_bits = 64".

>
> pw-bot: cr
>
>>                 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)
>> --
>> 2.29.2
>>
Alexei Starovoitov Oct. 30, 2024, 1:56 a.m. UTC | #7 
On Tue, Oct 29, 2024 at 6:45 PM Hou Tao <houtao@huaweicloud.com> wrote:
>
> Hi,
>
> On 10/30/2024 4:53 AM, Alexei Starovoitov wrote:
> > On Thu, Oct 24, 2024 at 6:20 PM 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.
> >>
>
> SNIP
> >>  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);
> > The check for nr_words is good, but moving computation after 'if'
> > feels like code churn and nothing else.
> > Even if nr_words is large, it's fine to do the math.
>
> No intention for code churn. I thought the overflow during
> multiplication may lead to UBSAN warning, but it seems the overflow
> warning is only for signed integer. Andrii also suggested me to move the
> assignment after the check [1].
>
> [1]:
> https://lore.kernel.org/bpf/CAEf4BzahtDCZDEdejm6cNMzDNTc0gXPzhc5xcWg9c8S_i6yWNA@mail.gmail.com/

u32 overflow is defined.
nr_words * sizeof(u64) and BYTES_TO_BITS(nr_bytes)
are ok to do regardless of the value of nr_words,
since it's in the unsigned domain.
Let's not make a precedent out of this.
Otherwise people will change the code this way all over the place.

Saying it differently... if UBSAN had an issue with existing code
it would have complained long ago.

> >>         /* Optimization for u64 mask */
> >> -       if (nr_bits == 64) {
> >> +       if (nr_words == 1) {
> > This is also unnecessary churn.
> >
> > Also it seems it's causing a warn on 32-bit:
> > https://netdev.bots.linux.dev/static/nipa/902902/13849894/build_32bit/
>
> It is weird that using "nr_bits = 64" doesn't reproduce the warning.
> Because the warning is due to the size of bits_copy is 4 bytes under
> 32-bit host and the error path of bpf_probe_read_kernel_common() invokes
> memset(&bits_copy, 0, 8). The warning will be fixed by the following
> patch "bpf: Use __u64 to save the bits in bits iterator". Anyway, will
> change it back to "nr_bits = 64".

It was puzzling for me as well. Thanks for these details.
diff mbox series

Patch

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)