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

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

Message-ID: <efe2af1b-4af1-da87-577c-e34bd5536572@lab.ntt.co.jp>
Date:   Fri, 18 Jan 2019 10:56:27 +0900
From:   Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
To:     Jason Wang <jasowang@...hat.com>,
        "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/01/17 22:05, Jason Wang wrote:
> 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.

Either would work.

With your suggestion the code will look like:

---
if (!prog) {
	for (...) {
		rcu_assign_pointer();
		...
	}
	synchronize_net();
}

virtnet_set_queues();
netif_set_real_num_rx_queues();
vi->xdp_queue_pairs = xdp_qp;

if (prog) {
	for (...) {
		rcu_assign_pointer();
		...
	}
}
---

But strictly speaking, virtnet_set_queues() should not be necessary if
(prog != NULL && old_prog != NULL).
If you prefer this, I can modify it accordingly.

-- 
Toshiaki Makita