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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJ+HfNiq=PNRz_Y8NJYuz=f33dmbu4nLM432ktBzPXTOSSTsMQ@mail.gmail.com>
Date:   Mon, 23 Jul 2018 11:41:02 +0200
From:   Björn Töpel <bjorn.topel@...il.com>
To:     ap420073@...il.com
Cc:     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()

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?


Björn

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