Message ID | 20241021014004.1647816-6-houtao@huaweicloud.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | BPF |
Headers | show |
Series | Misc fixes for bpf | expand |
On Mon, Oct 21, 2024 at 09:40:02AM +0800, Hou Tao 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 max value of nr_words to 512. lgtm, nice catch .. it's actually stated in the comment, but we did not force it Acked-by: Jiri Olsa <jolsa@kernel.org> jirka > > Fixes: 4665415975b0 ("bpf: Add bits iterator") > Signed-off-by: Hou Tao <houtao1@huawei.com> > --- > 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; > > /* Optimization for u64 mask */ > if (nr_bits == 64) { > -- > 2.29.2 >
On Sun, Oct 20, 2024 at 6:28 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 max value of nr_words to 512. > > Fixes: 4665415975b0 ("bpf: Add bits iterator") > Signed-off-by: Hou Tao <houtao1@huawei.com> > --- > 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; ah, didn't see this before replying on the previous patch. But still, maybe, let's move nr_bytes and nr_bits assignment to after this check? > > /* Optimization for u64 mask */ > if (nr_bits == 64) { > -- > 2.29.2 >
On Mon, Oct 21, 2024 at 9:28 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 max value of nr_words to 512. > > Fixes: 4665415975b0 ("bpf: Add bits iterator") > Signed-off-by: Hou Tao <houtao1@huawei.com> > --- > 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. > > /* Optimization for u64 mask */ > if (nr_bits == 64) { > -- > 2.29.2 > -- Regards Yafang
On 10/23/2024 11:17 AM, Yafang Shao wrote: > On Mon, Oct 21, 2024 at 9:28 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 max value of nr_words to 512. >> >> Fixes: 4665415975b0 ("bpf: Add bits iterator") >> Signed-off-by: Hou Tao <houtao1@huawei.com> >> --- >> 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. I prefer to keep the macro. How about updating the comment as follows: * Due to the limitation of memalloc, it can't be greater than 511. Therefore, * 511 is used as the maximum value for @nr_words. > >> /* Optimization for u64 mask */ >> if (nr_bits == 64) { >> -- >> 2.29.2 >> > > -- > Regards > Yafang
On Wed, Oct 23, 2024 at 4:29 PM Hou Tao <houtao@huaweicloud.com> wrote: > > > > On 10/23/2024 11:17 AM, Yafang Shao wrote: > > On Mon, Oct 21, 2024 at 9:28 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 max value of nr_words to 512. > >> > >> Fixes: 4665415975b0 ("bpf: Add bits iterator") > >> Signed-off-by: Hou Tao <houtao1@huawei.com> > >> --- > >> 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
On Wed, Oct 23, 2024 at 5:25 PM Yafang Shao <laoar.shao@gmail.com> wrote: > > On Wed, Oct 23, 2024 at 4:29 PM Hou Tao <houtao@huaweicloud.com> wrote: > > > > > > > > On 10/23/2024 11:17 AM, Yafang Shao wrote: > > > On Mon, Oct 21, 2024 at 9:28 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 max value of nr_words to 512. > > >> > > >> Fixes: 4665415975b0 ("bpf: Add bits iterator") > > >> Signed-off-by: Hou Tao <houtao1@huawei.com> > > >> --- > > >> 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.
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; /* Optimization for u64 mask */ if (nr_bits == 64) {