netdev - Re: [PATCH 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]

Date:   Thu, 17 Jan 2019 20:53:32 +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 net 5/7] virtio_net: Don't process redirected XDP frames
 when XDP is disabled


On 2019/1/17 下午7:20, 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.
>
> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@....ntt.co.jp>


I wonder whether or not we could simply do:


if (prog) {

     rcu_assign_pointer()

     synchronize_net()

}

set queues

if (!prog) {

     rcu_assign_pointer()

}

Thanks


> ---
>   drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++-------
>   1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 204eedf..ae93f0e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2424,14 +2424,16 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   		}
>   	}
>   
> -	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> -	if (err)
> -		goto err;
> -	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> -	vi->xdp_queue_pairs = xdp_qp;
> +	old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
> +	if (!old_prog && prog) {
> +		err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> +		if (err)
> +			goto err_new_prog;
> +		netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> +		vi->xdp_queue_pairs = xdp_qp;
> +	}
>   
>   	for (i = 0; i < vi->max_queue_pairs; i++) {
> -		old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
>   		rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
>   		if (i == 0) {
>   			if (!old_prog)
> @@ -2439,6 +2441,18 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   			if (!prog)
>   				virtnet_restore_guest_offloads(vi);
>   		}
> +	}
> +
> +	if (old_prog && !prog) {
> +		synchronize_net();
> +		err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> +		if (err)
> +			goto err_old_prog;
> +		netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> +		vi->xdp_queue_pairs = xdp_qp;
> +	}
> +
> +	for (i = 0; i < vi->max_queue_pairs; i++) {
>   		if (old_prog)
>   			bpf_prog_put(old_prog);
>   		if (netif_running(dev)) {
> @@ -2450,7 +2464,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   
>   	return 0;
>   
> -err:
> +err_old_prog:
> +	virtnet_clear_guest_offloads(vi);
> +	for (i = 0; i < vi->max_queue_pairs; i++)
> +		rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog);
> +err_new_prog:
>   	if (netif_running(dev)) {
>   		for (i = 0; i < vi->max_queue_pairs; i++) {
>   			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);