netdev - Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 27 Mar 2018 17:41:16 +0900
From:   Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries
 properly



On 3/27/2018 12:15 PM, John Fastabend wrote:
> On 03/25/2018 11:54 PM, Prashant Bhole wrote:
>> When CONFIG_DEBUG_SG is set, sg->sg_magic is initialized to SG_MAGIC,
>> when sg table is initialized using sg_init_table(). Magic is checked
>> while navigating the scatterlist. We hit BUG_ON when magic check is
>> failed.
>>
>> Fixed following things:
>> - Initialization of sg table in bpf_tcp_sendpage() was missing,
>>    initialized it using sg_init_table()
>>
>> - bpf_tcp_sendmsg() initializes sg table using sg_init_table() before
>>    entering the loop, but further consumed sg entries are initialized
>>    using memset. Fixed it by replacing memset with sg_init_table() in
>>    function bpf_tcp_push()
>>
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
>> ---
>>   kernel/bpf/sockmap.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
>> index 69c5bccabd22..8a848a99d768 100644
>> --- a/kernel/bpf/sockmap.c
>> +++ b/kernel/bpf/sockmap.c
>> @@ -312,7 +312,7 @@ static int bpf_tcp_push(struct sock *sk, int apply_bytes,
>>   			md->sg_start++;
>>   			if (md->sg_start == MAX_SKB_FRAGS)
>>   				md->sg_start = 0;
>> -			memset(sg, 0, sizeof(*sg));
>> +			sg_init_table(sg, 1);
> 
> Looks OK here.
> 
>>   
>>   			if (md->sg_start == md->sg_end)
>>   				break;
>> @@ -763,10 +763,14 @@ static int bpf_tcp_sendpage(struct sock *sk, struct page *page,
>>   
>>   	lock_sock(sk);
>>   
>> -	if (psock->cork_bytes)
>> +	if (psock->cork_bytes) {
>>   		m = psock->cork;
>> -	else
>> +		sg = &m->sg_data[m->sg_end];
>> +	} else {
>>   		m = &md;
>> +		sg = m->sg_data;
>> +		sg_init_table(sg, MAX_SKB_FRAGS);
> 
> sg_init_table() does an unnecessary memset() though. We
> probably either want a new scatterlist API or just open
> code this,
> 
> #ifdef CONFIG_DEBUG_SG
> {
> 	unsigned int i;
> 	for (i = 0; i < nents; i++)
> 		sgl[i].sg_magic = SG_MAGIC;
> }

Similar sg_init_table() is present in bpf_tcp_sendmsg().
I agree that it causes unnecessary memset, but I don't agree with open 
coded fix.
I am still with V1. Thanks.

-Prashant