netdev - Re: [PATCH bpf] xdp: add NULL pointer check in __xdp

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: <CAMArcTV6dwFR5Nsi2u8zGqQnM7BYeKmW1M+V0vCjGfqwpd30Pw@mail.gmail.com>
Date:   Mon, 23 Jul 2018 22:21:28 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     Björn Töpel <bjorn.topel@...il.com>
Cc:     Martin KaFai Lau <kafai@...com>,
        Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org,
        Björn Töpel <bjorn.topel@...el.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

2018-07-23 18:41 GMT+09:00 Björn Töpel <bjorn.topel@...il.com>:
> Den lör 21 juli 2018 kl 14:58 skrev Taehee Yoo <ap420073@...il.com>:
>>
>> 2018-07-21 2:18 GMT+09:00 Martin KaFai Lau <kafai@...com>:
>> > On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote:
>> >> rhashtable_lookup() can return NULL. so that NULL pointer
>> >> check routine should be added.
>> >>
>> >> Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
>> >> Signed-off-by: Taehee Yoo <ap420073@...il.com>
>> >> ---
>> >>  net/core/xdp.c | 3 ++-
>> >>  1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> >> index 9d1f220..1c12bc7 100644
>> >> --- a/net/core/xdp.c
>> >> +++ b/net/core/xdp.c
>> >> @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
>> >>               rcu_read_lock();
>> >>               /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
>> >>               xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
>> >> -             xa->zc_alloc->free(xa->zc_alloc, handle);
>> >> +             if (xa)
>> >> +                     xa->zc_alloc->free(xa->zc_alloc, handle);
>> > hmm...It is not clear to me the "!xa" case don't have to be handled?
>>
>> Thank you for reviewing!
>>
>> Returning NULL pointer is bug case such as calling after use
>> xdp_rxq_info_unreg().
>> so that, I think it can't handle at that moment.
>> we can make __xdp_return to add WARN_ON_ONCE() or
>> add return error code to driver.
>> But I'm not sure if these is useful information.
>>
>> I might have misunderstood scenario of MEM_TYPE_ZERO_COPY
>> because there is no use case of MEM_TYPE_ZERO_COPY yet.
>>
>
> Taehee, again, sorry for the slow response and thanks for patch!
>
> If xa is NULL, the driver has a buggy/broken implementation. What
> would be a proper way of dealing with this? BUG?
>

Thank you for reviewing!

I would like to add WARN_ON_ONCE. because code writers can
get opportunity for debugging this in runtime. also I think this bug
doesn't make critical side effect.

Thanks!

>
> Björn
>
>> Thanks!
>>
>> >
>> >>               rcu_read_unlock();
>> >>       default:
>> >>               /* Not possible, checked in xdp_rxq_info_reg_mem_model() */
>> >> --
>> >> 2.9.3
>> >>