netdev - Re: [PATCH v2 net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

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: <c76c0e36-aef2-5bbc-92f7-5040749208f8@redhat.com>
Date:   Tue, 29 Jan 2019 10:20:11 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Toshiaki Makita <makita.toshiaki@....ntt.co.jp>,
        "David S. Miller" <davem@...emloft.net>,
        "Michael S. Tsirkin" <mst@...hat.com>
Cc:     netdev@...r.kernel.org, virtualization@...ts.linux-foundation.org,
        Jesper Dangaard Brouer <brouer@...hat.com>
Subject: Re: [PATCH v2 net 5/7] virtio_net: Don't process redirected XDP
 frames when XDP is disabled


On 2019/1/29 上午8:45, Toshiaki Makita wrote:
> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
> ready for XDP") tried to avoid access to unexpected sq while XDP is
> disabled, but was not complete.
>
> There was a small window which causes out of bounds sq access in
> virtnet_xdp_xmit() while disabling XDP.
>
> An example case of
>   - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
>   - online_cpu_num = xdp_queue_paris = 4
> when XDP is enabled:
>
> CPU 0                         CPU 1
> (Disabling XDP)               (Processing redirected XDP frames)
>
>                                virtnet_xdp_xmit()
> virtnet_xdp_set()
>   _virtnet_set_queues()
>    set curr_queue_pairs (2)
>                                 check if rq->xdp_prog is not NULL
>                                 virtnet_xdp_sq(vi)
>                                  qp = curr_queue_pairs -
>                                       xdp_queue_pairs +
>                                       smp_processor_id()
>                                     = 2 - 4 + 1 = -1
>                                  sq = &vi->sq[qp] // out of bounds access
>    set xdp_queue_pairs (0)
>    rq->xdp_prog = NULL
>
> Basically we should not change curr_queue_pairs and xdp_queue_pairs
> while someone can read the values. Thus, when disabling XDP, assign NULL
> to rq->xdp_prog first, and wait for RCU grace period, then change
> xxx_queue_pairs.
> Note that we need to keep the current order when enabling XDP though.
>
> - v2: Make rcu_assign_pointer/synchronize_net conditional instead of
>        _virtnet_set_queues.
>
> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 33 ++++++++++++++++++++++++++-------
>   1 file changed, 26 insertions(+), 7 deletions(-)


Acked-by: Jason Wang <jasowang@...hat.com>

Thanks.