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]

Message-ID: <92d66b9e-d93a-9f87-a6db-84aee0c14284@lab.ntt.co.jp>
Date:   Wed, 28 Mar 2018 15:18:59 +0900
From:   Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     John Fastabend <john.fastabend@...il.com>,
        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 6:05 PM, Daniel Borkmann wrote:
> On 03/27/2018 10:41 AM, Prashant Bhole wrote:
>> 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.
> 
> But then lets fix is properly and add a static inline helper to the
> include/linux/scatterlist.h header like ...
> 
> static inline void sg_init_debug_marker(struct scatterlist *sgl,
> 					unsigned int nents)
> {
> #ifdef CONFIG_DEBUG_SG
> 	unsigned int i;
> 
> 	for (i = 0; i < nents; i++)
> 		sgl[i].sg_magic = SG_MAGIC;
> #endif
> }
> 
> ... and reuse it in all the places that would otherwise open-code this,
> as well as sg_init_table():
> 
> void sg_init_table(struct scatterlist *sgl, unsigned int nents)
> {
>          memset(sgl, 0, sizeof(*sgl) * nents);
> 	sg_init_debug_marker(sgl, nents);
>          sg_mark_end(&sgl[nents - 1]);
> }
> 
> This would be a lot cleaner than having this duplicated in various places.

Daniel, This is a good suggestion. Is it ok if I submit both changes in
a patch series? How scatterlist related changes will be picked up by 
other subsystems?

-Prashant