netdev - Re: [PATCH bpf-next] bpf: select proper size for bpf_prog

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 8 Mar 2022 17:19:49 +0000
From:   Song Liu <songliubraving@...com>
To:     Daniel Borkmann <daniel@...earbox.net>
CC:     Song Liu <song@...nel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>,
        "andrii@...nel.org" <andrii@...nel.org>,
        Kernel Team <Kernel-team@...com>,
        "edumazet@...gle.com" <edumazet@...gle.com>
Subject: Re: [PATCH bpf-next] bpf: select proper size for bpf_prog_pack



> On Mar 8, 2022, at 8:58 AM, Daniel Borkmann <daniel@...earbox.net> wrote:
> 
> On 3/4/22 7:43 PM, Song Liu wrote:
>> Using HPAGE_PMD_SIZE as the size for bpf_prog_pack is not ideal in some
>> cases. Specifically, for NUMA systems, __vmalloc_node_range requires
>> PMD_SIZE * num_online_nodes() to allocate huge pages. Also, if the system
>> does not support huge pages (i.e., with cmdline option nohugevmalloc), it
>> is better to use PAGE_SIZE packs.
>> Add logic to select proper size for bpf_prog_pack. This solution is not
>> ideal, as it makes assumption about the behavior of module_alloc and
>> __vmalloc_node_range. However, it appears to be the easiest solution as
>> it doesn't require changes in module_alloc and vmalloc code.
> 
> nit: Fixes tag?
> 
>> Signed-off-by: Song Liu <song@...nel.org>
> [...]
>>  +static size_t bpf_prog_pack_size = -1;
>> +
>> +static inline int bpf_prog_chunk_count(void)
>> +{
>> +	WARN_ON_ONCE(bpf_prog_pack_size == -1);
>> +	return bpf_prog_pack_size / BPF_PROG_CHUNK_SIZE;
>> +}
>> +
>>  static DEFINE_MUTEX(pack_mutex);
>>  static LIST_HEAD(pack_list);
>>    static struct bpf_prog_pack *alloc_new_pack(void)
>>  {
>>  	struct bpf_prog_pack *pack;
>> +	size_t size;
>> +	void *ptr;
>>  -	pack = kzalloc(sizeof(*pack) + BITS_TO_BYTES(BPF_PROG_CHUNK_COUNT), GFP_KERNEL);
>> -	if (!pack)
>> +	if (bpf_prog_pack_size == -1) {
>> +		/* Test whether we can get huge pages. If not just use
>> +		 * PAGE_SIZE packs.
>> +		 */
>> +		size = PMD_SIZE * num_online_nodes();
>> +		ptr = module_alloc(size);
>> +		if (ptr && is_vm_area_hugepages(ptr)) {
>> +			bpf_prog_pack_size = size;
>> +			goto got_ptr;
>> +		} else {
>> +			bpf_prog_pack_size = PAGE_SIZE;
>> +			vfree(ptr);
>> +		}
>> +	}
>> +
>> +	ptr = module_alloc(bpf_prog_pack_size);
>> +	if (!ptr)
>>  		return NULL;
>> -	pack->ptr = module_alloc(BPF_PROG_PACK_SIZE);
>> -	if (!pack->ptr) {
>> -		kfree(pack);
>> +got_ptr:
>> +	pack = kzalloc(struct_size(pack, bitmap, BITS_TO_LONGS(bpf_prog_chunk_count())),
>> +		       GFP_KERNEL);
>> +	if (!pack) {
>> +		vfree(ptr);
>>  		return NULL;
>>  	}
>> -	bitmap_zero(pack->bitmap, BPF_PROG_PACK_SIZE / BPF_PROG_CHUNK_SIZE);
>> +	pack->ptr = ptr;
>> +	bitmap_zero(pack->bitmap, bpf_prog_pack_size / BPF_PROG_CHUNK_SIZE);
>>  	list_add_tail(&pack->list, &pack_list);
>>    	set_vm_flush_reset_perms(pack->ptr);
>> -	set_memory_ro((unsigned long)pack->ptr, BPF_PROG_PACK_SIZE / PAGE_SIZE);
>> -	set_memory_x((unsigned long)pack->ptr, BPF_PROG_PACK_SIZE / PAGE_SIZE);
>> +	set_memory_ro((unsigned long)pack->ptr, bpf_prog_pack_size / PAGE_SIZE);
>> +	set_memory_x((unsigned long)pack->ptr, bpf_prog_pack_size / PAGE_SIZE);
>>  	return pack;
>>  }
>>  @@ -864,7 +886,7 @@ static void *bpf_prog_pack_alloc(u32 size)
>>  	unsigned long pos;
>>  	void *ptr = NULL;
>>  -	if (size > BPF_PROG_MAX_PACK_PROG_SIZE) {
>> +	if (size > bpf_prog_pack_size) {
>>  		size = round_up(size, PAGE_SIZE);
>>  		ptr = module_alloc(size);
>>  		if (ptr) {
> 
> What happens if the /very first/ program requests an allocation size of >PAGE_SIZE? Wouldn't
> this result in OOB write?
> 
> The 'size > bpf_prog_pack_size' is initially skipped due to -1 but then the module_alloc()
> won't return a huge page, so we redo the allocation with bpf_prog_pack_size as PAGE_SIZE and
> return a pointer into this pack?

Good catch! Let me see how to fix this.

Thanks,
Song