netdev - Re: [PATCH bpf-next 2/2] bpf, x86: set header->size properly before freeing it

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]

Message-ID: <3CC742A6-36B9-4D8D-B801-9700660C9E6B@fb.com>
Date:   Wed, 2 Mar 2022 17:24:44 +0000
From:   Song Liu <songliubraving@...com>
To:     Yonghong Song <yhs@...com>
CC:     Song Liu <song@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Kernel Team <Kernel-team@...com>, Kui-Feng Lee <kuifeng@...com>
Subject: Re: [PATCH bpf-next 2/2] bpf, x86: set header->size properly before
 freeing it



> On Mar 1, 2022, at 11:12 PM, Yonghong Song <yhs@...com> wrote:
> 
> 
> 
> On 3/1/22 4:43 PM, Song Liu wrote:
>> On do_jit failure path, the header is freed by bpf_jit_binary_pack_free.
>> While bpf_jit_binary_pack_free doesn't require proper ro_header->size,
>> bpf_prog_pack_free still uses it. Set header->size in bpf_int_jit_compile
>> before calling bpf_jit_binary_pack_free.
>> Fixes: 1022a5498f6f ("bpf, x86_64: Use bpf_jit_binary_pack_alloc")
>> Fixes: 33c9805860e5 ("bpf: Introduce bpf_jit_binary_pack_[alloc|finalize|free]")
>> Reported-by: Kui-Feng Lee <kuifeng@...com>
>> Signed-off-by: Song Liu <song@...nel.org>
> 
> LGTM with a nit below related to comments.
> 
> Acked-by: Yonghong Song <yhs@...com>
> 
>> ---
>>  arch/x86/net/bpf_jit_comp.c | 6 +++++-
>>  kernel/bpf/core.c           | 7 ++++---
>>  2 files changed, 9 insertions(+), 4 deletions(-)
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index c7db0fe4de2f..b923d81ff6f9 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -2330,8 +2330,12 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>>  		if (proglen <= 0) {
>>  out_image:
>>  			image = NULL;
>> -			if (header)
>> +			if (header) {
>> +				/* set header->size for bpf_arch_text_copy */
> 
> This comment is confusing. Setting header->size is not for bpf_arch_text_copy. Probably you mean 'by bpf_arch_text_copy?
> I think this comment is not necessary.

I meant to say set header->size for bpf_jit_binary_pack_free(). I guess it is
not really necessary. Let me remove it. 

> 
>> +				bpf_arch_text_copy(&header->size, &rw_header->size,
>> +						   sizeof(rw_header->size));
>>  				bpf_jit_binary_pack_free(header, rw_header);
>> +			}
>>  			prog = orig_prog;
>>  			goto out_addrs;
>>  		}
>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>> index ebb0193d07f0..da587e4619e0 100644
>> --- a/kernel/bpf/core.c
>> +++ b/kernel/bpf/core.c
>> @@ -1112,13 +1112,14 @@ int bpf_jit_binary_pack_finalize(struct bpf_prog *prog,
>>   *   1) when the program is freed after;
>>   *   2) when the JIT engine fails (before bpf_jit_binary_pack_finalize).
>>   * For case 2), we need to free both the RO memory and the RW buffer.
>> - * Also, ro_header->size in 2) is not properly set yet, so rw_header->size
>> - * is used for uncharge.
>> + *
>> + * If bpf_jit_binary_pack_free is called before _finalize (jit error),
> 
> Do you mean bpf_jit_binary_pack_free() is called before calling
> bpf_jit_binary_pack_finalize()? This seems not the case.

Now I see this is a little confusing. Let me rephrase it in v2. 

Thanks,
Song

> 
>> + * it is necessary to set ro_header->size properly before freeing it.
>>   */
>>  void bpf_jit_binary_pack_free(struct bpf_binary_header *ro_header,
>>  			      struct bpf_binary_header *rw_header)
>>  {
>> -	u32 size = rw_header ? rw_header->size : ro_header->size;
>> +	u32 size = ro_header->size;
>>    	bpf_prog_pack_free(ro_header);
>>  	kvfree(rw_header);