On Thu, Oct 24, 2024 at 6:20 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> From: Hou Tao <houtao1@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
>
