On Tue, Oct 29, 2024 at 6:45 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 10/30/2024 4:53 AM, Alexei Starovoitov wrote:
> > 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.
> >>
>
> SNIP
> >> 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.
>
> No intention for code churn. I thought the overflow during
> multiplication may lead to UBSAN warning, but it seems the overflow
> warning is only for signed integer. Andrii also suggested me to move the
> assignment after the check [1].
>
> [1]:
> https://lore.kernel.org/bpf/CAEf4BzahtDCZDEdejm6cNMzDNTc0gXPzhc5xcWg9c8S_i6yWNA@xxxxxxxxxxxxxx/
u32 overflow is defined.
nr_words * sizeof(u64) and BYTES_TO_BITS(nr_bytes)
are ok to do regardless of the value of nr_words,
since it's in the unsigned domain.
Let's not make a precedent out of this.
Otherwise people will change the code this way all over the place.
Saying it differently... if UBSAN had an issue with existing code
it would have complained long ago.
> >> /* 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/
>
> It is weird that using "nr_bits = 64" doesn't reproduce the warning.
> Because the warning is due to the size of bits_copy is 4 bytes under
> 32-bit host and the error path of bpf_probe_read_kernel_common() invokes
> memset(&bits_copy, 0, 8). The warning will be fixed by the following
> patch "bpf: Use __u64 to save the bits in bits iterator". Anyway, will
> change it back to "nr_bits = 64".
It was puzzling for me as well. Thanks for these details.
