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.
>> /* Fallback to memalloc */
>> kit->bits = bpf_mem_alloc(&bpf_global_ma, nr_bytes);
>> if (!kit->bits)
>> --
>> 2.29.2
>>
>
