On Fri, Oct 25, 2024 at 3:52 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> 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@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);
> >>
> >> /* 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.
--
Regards
Yafang
