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]

Message-ID: <cf705fbb-d2cf-95d1-680a-739fa3652b6b@redhat.com>
Date:   Fri, 18 Jan 2019 11:52:51 +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/18 上午9:56, Toshiaki Makita wrote:
> 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();
> 		...
> 	}
> }


Yes, I think this makes code more easier to be understand.


> ---
>
> But strictly speaking, virtnet_set_queues() should not be necessary if
> (prog != NULL && old_prog != NULL).


Yes, but it was another possible 'issue'.


> If you prefer this, I can modify it accordingly.


I prefer to do this change.

Thanks