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

Date:   Fri, 30 Mar 2018 09:20:10 +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/28/2018 5:51 PM, Daniel Borkmann wrote:
> On 03/28/2018 08:18 AM, Prashant Bhole wrote:
>> 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?
> 
> Sure, that's fine.
> 
>> How scatterlist related changes will be picked up by other subsystems?
> 
> Once this gets applied into bpf-next, this will be pushed to net-next tree,
> and during the merge window net-next will be pulled into Linus' tree if this
> is what you are asking. Then also other subsystems outside of bpf/networking
> can make use of the sg_init_debug_marker() helper if suitable for their
> situation.

Thanks. I am submitting V2 soon.

-Prashant