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 21:05:47 +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 下午8:53, Jason Wang wrote:
>
> 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) {


Should be !prog


>
>     rcu_assign_pointer()
>
>     synchronize_net()
>
> }
>
> set queues
>
> if (!prog) {


Should be prog.

Thanks


>
>     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);