On Wed, Oct 23, 2024 at 5:25 PM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
>
> On Wed, Oct 23, 2024 at 4:29 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
> >
> >
> >
> > On 10/23/2024 11:17 AM, Yafang Shao wrote:
> > > On Mon, Oct 21, 2024 at 9:28 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 max value of nr_words to 512.
> > >>
> > >> Fixes: 4665415975b0 ("bpf: Add bits iterator")
> > >> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
> > >> ---
> > >> kernel/bpf/helpers.c | 4 ++++
> > >> 1 file changed, 4 insertions(+)
> > >>
> > >> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > >> index 62349e206a29..c147f75e1b48 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 512
> > >> +
> > >> struct bpf_iter_bits_kern {
> > >> union {
> > >> unsigned long *bits;
> > >> @@ -2892,6 +2894,8 @@ 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;
> > > It is documented that nr_words cannot exceed 512, not due to overflow
> > > concerns, but because of memory allocation limits. It might be better
> > > to use 512 directly instead of BITS_ITER_NR_WORDS_MAX. Alternatively,
> > > if we decide to keep using the macro, the documentation should be
> > > updated accordingly.
> >
> > Thanks for the explanation. Actually according to the limitation of bpf
> > memory allocator, the limitation should be (4096 - 8) / 8 = 511 due to
> > the overhead of llist_head in the returned pointer.
>
> If that's the case, we should make the following code change, right ?
>
> diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c
> index 1a1b4458114c..64e73579c7d6 100644
> --- a/kernel/bpf/memalloc.c
> +++ b/kernel/bpf/memalloc.c
> @@ -65,7 +65,7 @@ static u8 size_index[24] __ro_after_init = {
>
> static int bpf_mem_cache_idx(size_t size)
> {
> - if (!size || size > 4096)
> + if (!size || size > (4096 - 8))
> return -1;
>
> if (size <= 192)
>
>
>
> --
> Regards
> Yafang
Please disregard my previous comment—I missed the !ma->percpu case.
You're right, the value cannot exceed 511.
--
Regards
Yafang
