On Fri, Oct 25, 2024 at 6:29 AM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
>
> 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.
No. bpf_mem_alloc() returns NULL or valid and will stay this way.
