[v3,1/2] vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc
Checks
Commit Message
in virtio_dev_split we cannot currently call desc_to_mbuf with
nr_vec == 0, or we end up trying to rte_memcpy from a source address
buf_vec[0] that is an uninitialized stack variable.
Improve this in general by having desc_to_mbuf and mbuf_to_desc
return -1 when called with an invalid nr_vec == 0, which should
fix any other instance of this problem.
This should fix errors that have been reported in multiple occasions
from telcos to the DPDK, OVS and QEMU projects, as this affects in
particular the openvswitch/DPDK, QEMU vhost-user setup when the
guest DPDK application abruptly goes away via SIGKILL and then
reconnects.
The back trace looks roughly like this, depending on the specific
rte_memcpy selected, etc, in any case the "src" parameter is garbage
(in this example containing 0 + dev->host_hlen(12 = 0xc)).
Thread 153 "pmd-c88/id:150" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7f64e5e6b700 (LWP 141373)]
rte_mov128blocks (n=2048, src=0xc <error: Cannot access memory at 0xc>,
dst=0x150da4480) at ../lib/eal/x86/include/rte_memcpy.h:384
(gdb) bt
0 rte_mov128blocks (n=2048, src=0xc, dst=0x150da4480)
1 rte_memcpy_generic (n=2048, src=0xc, dst=0x150da4480)
2 rte_memcpy (n=2048, src=0xc, dst=<optimized out>)
3 sync_fill_seg
4 desc_to_mbuf
5 virtio_dev_tx_split
6 virtio_dev_tx_split_legacy
7 0x00007f676fea0fef in rte_vhost_dequeue_burst
8 0x00007f6772005a62 in netdev_dpdk_vhost_rxq_recv
9 0x00007f6771f38116 in netdev_rxq_recv
10 0x00007f6771f03d96 in dp_netdev_process_rxq_port
11 0x00007f6771f04239 in pmd_thread_main
12 0x00007f6771f92aff in ovsthread_wrapper
13 0x00007f6771c1b6ea in start_thread
14 0x00007f6771933a8f in clone
Tested-by: Claudio Fontana <cfontana@suse.de>
Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
lib/vhost/virtio_net.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
Comments
On Tue, 2 Aug 2022 02:49:37 +0200
Claudio Fontana <cfontana@suse.de> wrote:
> Tested-by: Claudio Fontana <cfontana@suse.de>
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
Minor nit having both tags by same author is redundant and unnecessary.
Hi Claudio,
The title should be reworded, maybe something like below?
"vhost: fix possible out of bound access in buffer vectors"
On 8/2/22 02:49, Claudio Fontana wrote:
> in virtio_dev_split we cannot currently call desc_to_mbuf with
> nr_vec == 0, or we end up trying to rte_memcpy from a source address
> buf_vec[0] that is an uninitialized stack variable.
>
> Improve this in general by having desc_to_mbuf and mbuf_to_desc
> return -1 when called with an invalid nr_vec == 0, which should
> fix any other instance of this problem.
>
> This should fix errors that have been reported in multiple occasions
> from telcos to the DPDK, OVS and QEMU projects, as this affects in
> particular the openvswitch/DPDK, QEMU vhost-user setup when the
> guest DPDK application abruptly goes away via SIGKILL and then
> reconnects.
Looking at fill_vec_buf_split() and map_one_desc(), I suspect that on
guest DPDK application kill/crash, the shared virtqueue memory gets
unmmaped on guest side, and the backend reads zeros in descriptors, and
so zero-length.
If that's the case, then your fix might not be enough, if for example
the backends reads the .next field of the desc as 0, it may cause
undefined behaviour. I'm not sure how we could address this though.
Maybe we miss something on Vhost side to detect the guest application
has crashed, because we should not try to access the vrings as long as
the new guest app instance is done with its initialization.
I'm going to try to reproduce the issue, but I'm not sure I will
succeed. Could you please share the Vhost logs when the issue is
reproduced and you face the crash?
>
> The back trace looks roughly like this, depending on the specific
> rte_memcpy selected, etc, in any case the "src" parameter is garbage
> (in this example containing 0 + dev->host_hlen(12 = 0xc)).
>
> Thread 153 "pmd-c88/id:150" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7f64e5e6b700 (LWP 141373)]
> rte_mov128blocks (n=2048, src=0xc <error: Cannot access memory at 0xc>,
> dst=0x150da4480) at ../lib/eal/x86/include/rte_memcpy.h:384
> (gdb) bt
> 0 rte_mov128blocks (n=2048, src=0xc, dst=0x150da4480)
> 1 rte_memcpy_generic (n=2048, src=0xc, dst=0x150da4480)
> 2 rte_memcpy (n=2048, src=0xc, dst=<optimized out>)
> 3 sync_fill_seg
> 4 desc_to_mbuf
> 5 virtio_dev_tx_split
> 6 virtio_dev_tx_split_legacy
> 7 0x00007f676fea0fef in rte_vhost_dequeue_burst
> 8 0x00007f6772005a62 in netdev_dpdk_vhost_rxq_recv
> 9 0x00007f6771f38116 in netdev_rxq_recv
> 10 0x00007f6771f03d96 in dp_netdev_process_rxq_port
> 11 0x00007f6771f04239 in pmd_thread_main
> 12 0x00007f6771f92aff in ovsthread_wrapper
> 13 0x00007f6771c1b6ea in start_thread
> 14 0x00007f6771933a8f in clone
It is a fix, so it should contains the Fixes tag, and also cc
stable@dpdk.org.
>
> Tested-by: Claudio Fontana <cfontana@suse.de>
Tested-by can be removed as you're already the author.
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
> lib/vhost/virtio_net.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 35fa4670fd..eb19e54c2b 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -1153,7 +1153,7 @@ mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
> struct virtio_net_hdr_mrg_rxbuf tmp_hdr, *hdr = NULL;
> struct vhost_async *async = vq->async;
>
> - if (unlikely(m == NULL))
> + if (unlikely(m == NULL) || nr_vec == 0)
I think nr_vec == 0 is also unlikely.
> return -1;
>
> buf_addr = buf_vec[vec_idx].buf_addr;
> @@ -2673,6 +2673,9 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
> struct vhost_async *async = vq->async;
> struct async_inflight_info *pkts_info;
>
> + if (unlikely(nr_vec == 0))
> + return -1;
> +
> buf_addr = buf_vec[vec_idx].buf_addr;
> buf_iova = buf_vec[vec_idx].buf_iova;
> buf_len = buf_vec[vec_idx].buf_len;
> @@ -2917,9 +2920,11 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
> vq->last_avail_idx + i,
> &nr_vec, buf_vec,
> &head_idx, &buf_len,
> - VHOST_ACCESS_RO) < 0))
> + VHOST_ACCESS_RO) < 0)) {
> + dropped += 1;
> + i++;
> break;
> -
> + }
> update_shadow_used_ring_split(vq, head_idx, 0);
>
> err = virtio_dev_pktmbuf_prep(dev, pkts[i], buf_len);
On 9/28/22 16:37, Maxime Coquelin wrote:
> Hi Claudio,
>
> The title should be reworded, maybe something like below?
> "vhost: fix possible out of bound access in buffer vectors"
possible, I leave it to you and other maintainers here now to figure out.
>
> On 8/2/22 02:49, Claudio Fontana wrote:
>> in virtio_dev_split we cannot currently call desc_to_mbuf with
>> nr_vec == 0, or we end up trying to rte_memcpy from a source address
>> buf_vec[0] that is an uninitialized stack variable.
>>
>> Improve this in general by having desc_to_mbuf and mbuf_to_desc
>> return -1 when called with an invalid nr_vec == 0, which should
>> fix any other instance of this problem.
>>
>> This should fix errors that have been reported in multiple occasions
>> from telcos to the DPDK, OVS and QEMU projects, as this affects in
>> particular the openvswitch/DPDK, QEMU vhost-user setup when the
>> guest DPDK application abruptly goes away via SIGKILL and then
>> reconnects.
>
> Looking at fill_vec_buf_split() and map_one_desc(), I suspect that on
> guest DPDK application kill/crash, the shared virtqueue memory gets
> unmmaped on guest side, and the backend reads zeros in descriptors, and
> so zero-length.
>
> If that's the case, then your fix might not be enough, if for example
> the backends reads the .next field of the desc as 0, it may cause
> undefined behaviour. I'm not sure how we could address this though.
>
> Maybe we miss something on Vhost side to detect the guest application
> has crashed, because we should not try to access the vrings as long as
> the new guest app instance is done with its initialization.
>
> I'm going to try to reproduce the issue, but I'm not sure I will
> succeed. Could you please share the Vhost logs when the issue is
> reproduced and you face the crash?
With vacations and lab work it's unlikely anything can be done from my side for the next 2-3 weeks.
This issue has been reported multiple times from multiple telco customers for about a year, it's all over the mailing lists
between ovs, dpdk and qemu, with all the details.
Something in the governance of these Open Source projects is clearly not working right, probably too much inward-focus between a small number of companies, but I digress.
I think Chenbo Xia already knows the context, and I suspect this is now considered a "security issue". The problem is, the information about all of this has been public already for a year.
I will again repost how to reproduce here:
In general, the easiest thing to reproduce this is to use an older DPDK in the guest (16.11.x, as per the Ubuntu package),
run a testpmd application with the easiest possible configuration:
I could reduce the problem to just a single data interface, without any need to pin anything, or do any complex flow configuration. The key is in getting guest application, qemu and dpdk in the right state. The data plane traffic is flowing from a traffic generator on another host.
1. Interfaces and Bridges
# ovs-vsctl show
9edee1e2-c16b-41cf-bf9d-ce1ffbc096d2
Bridge ovs-br0
fail_mode: standalone
datapath_type: netdev
Port eth0
Interface eth0
Port dpdkvhostuser0
Interface dpdkvhostuser0
type: dpdkvhostuserclient
options: {vhost-server-path="/tmp/dpdkvhostuser0"}
Port ovs-br0
Interface ovs-br0
type: internal
Port data0
Interface data0
type: dpdk
options: {dpdk-devargs="0000:17:00.1", dpdk-lsc-interrupt="true", n_rxq="2", n_txq_desc="2048"}
ovs_version: "2.14.2"
2. QEMU VM XML configuration snippet:
<interface type='vhostuser'>
<mac address='00:00:00:00:00:01'/>
<source type='unix' path='/tmp/dpdkvhostuser0' mode='server'/>
<target dev='dpdkvhostuser0'/>
<model type='virtio'/>
<driver name='vhost' queues='4' rx_queue_size='1024' tx_queue_size='1024'>
<host mrg_rxbuf='off'/>
</driver>
<alias name='net0'/>
<address type='pci' domain='0x0000' bus='0x07' slot='0x00' function='0x0'/>
</interface>
<interface type='network'>
<mac address='xx:xx:xx:xx:xx:xx'/>
<source network='default' portid='...' bridge='virbr0'/>
<target dev='vnet9'/>
<model type='virtio'/>
<alias name='net1'/>
<address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
</interface>
3. Inside the VM bind the device.
$ cat devbind.sh
modprobe uio_pci_generic
dpdk-stable-16.11.11/tools/dpdk-devbind.py -b uio_pci_generic 0000:07:00.0
4. Prepare the test scripts inside the VM:
Here are the scripts to use:
$ cat start_testpmd.sh
#! /bin/bash
while true ; do
testpmd --log-level=8 -c 0x1e -n 4 --socket-mem 512 -- -i --nb-cores=3 --port-topology=chained --disable-hw-vlan --forward-mode=macswap --auto-start --rxq=4 --txq=4 --rxd=512 --txd=512 --burst=32
done
$ cat kill_testpmd.sh
#! /bin/bash
while true ; do
kill -9 `pgrep -x testpmd`
sleep 2
done
5. Run the test scripts in separate ssh sessions or consoles:
session 1: ./start_testpmd.sh
session 2: ./kill_testpmd.sh
this ensures that the test is continuosly started and killed after a short time running.
If the "sleep 2" is too little (necessary if you are using older qemu or DPDK on the host) you can increase it to "sleep 10".
But with upstream DPDK and decent hardware, "sleep 2" will make you hit the issue quicker.
The issue will appear as a segfault of the ovs daemon. It can be captured via gdb, or monitored via the ovs monitor,
with the backtrace that follows:
>
>>
>> The back trace looks roughly like this, depending on the specific
>> rte_memcpy selected, etc, in any case the "src" parameter is garbage
>> (in this example containing 0 + dev->host_hlen(12 = 0xc)).
>>
>> Thread 153 "pmd-c88/id:150" received signal SIGSEGV, Segmentation fault.
>> [Switching to Thread 0x7f64e5e6b700 (LWP 141373)]
>> rte_mov128blocks (n=2048, src=0xc <error: Cannot access memory at 0xc>,
>> dst=0x150da4480) at ../lib/eal/x86/include/rte_memcpy.h:384
>> (gdb) bt
>> 0 rte_mov128blocks (n=2048, src=0xc, dst=0x150da4480)
>> 1 rte_memcpy_generic (n=2048, src=0xc, dst=0x150da4480)
>> 2 rte_memcpy (n=2048, src=0xc, dst=<optimized out>)
>> 3 sync_fill_seg
>> 4 desc_to_mbuf
>> 5 virtio_dev_tx_split
>> 6 virtio_dev_tx_split_legacy
>> 7 0x00007f676fea0fef in rte_vhost_dequeue_burst
>> 8 0x00007f6772005a62 in netdev_dpdk_vhost_rxq_recv
>> 9 0x00007f6771f38116 in netdev_rxq_recv
>> 10 0x00007f6771f03d96 in dp_netdev_process_rxq_port
>> 11 0x00007f6771f04239 in pmd_thread_main
>> 12 0x00007f6771f92aff in ovsthread_wrapper
>> 13 0x00007f6771c1b6ea in start_thread
>> 14 0x00007f6771933a8f in clone
>
>
> It is a fix, so it should contains the Fixes tag, and also cc
> stable@dpdk.org.
After one year, it is time for Redhat and Intel or whatever the governance of this project is,
to mention if there is any intention to fix this or not, before I or anyone else at SUSE will invest any more of our time and efforts here.
Any tags you need you can add as required.
Thanks,
Claudio
>
>>
>> Tested-by: Claudio Fontana <cfontana@suse.de>
>
> Tested-by can be removed as you're already the author.
>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>> lib/vhost/virtio_net.c | 11 ++++++++---
>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
>> index 35fa4670fd..eb19e54c2b 100644
>> --- a/lib/vhost/virtio_net.c
>> +++ b/lib/vhost/virtio_net.c
>> @@ -1153,7 +1153,7 @@ mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
>> struct virtio_net_hdr_mrg_rxbuf tmp_hdr, *hdr = NULL;
>> struct vhost_async *async = vq->async;
>>
>> - if (unlikely(m == NULL))
>> + if (unlikely(m == NULL) || nr_vec == 0)
>
> I think nr_vec == 0 is also unlikely.
>
>> return -1;
>>
>> buf_addr = buf_vec[vec_idx].buf_addr;
>> @@ -2673,6 +2673,9 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
>> struct vhost_async *async = vq->async;
>> struct async_inflight_info *pkts_info;
>>
>> + if (unlikely(nr_vec == 0))
>> + return -1;
>> +
>> buf_addr = buf_vec[vec_idx].buf_addr;
>> buf_iova = buf_vec[vec_idx].buf_iova;
>> buf_len = buf_vec[vec_idx].buf_len;
>> @@ -2917,9 +2920,11 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
>> vq->last_avail_idx + i,
>> &nr_vec, buf_vec,
>> &head_idx, &buf_len,
>> - VHOST_ACCESS_RO) < 0))
>> + VHOST_ACCESS_RO) < 0)) {
>> + dropped += 1;
>> + i++;
>> break;
>> -
>> + }
>> update_shadow_used_ring_split(vq, head_idx, 0);
>>
>> err = virtio_dev_pktmbuf_prep(dev, pkts[i], buf_len);
>
28/09/2022 17:21, Claudio Fontana:
> On 9/28/22 16:37, Maxime Coquelin wrote:
> > The title should be reworded, maybe something like below?
> > "vhost: fix possible out of bound access in buffer vectors"
>
> possible, I leave it to you and other maintainers here now to figure out.
Maxime is suggesting a reword to you for your next version.
> > On 8/2/22 02:49, Claudio Fontana wrote:
[...]
> >> This should fix errors that have been reported in multiple occasions
> >> from telcos to the DPDK, OVS and QEMU projects, as this affects in
> >> particular the openvswitch/DPDK, QEMU vhost-user setup when the
> >> guest DPDK application abruptly goes away via SIGKILL and then
> >> reconnects.
What are the "multiple occasions"? Is there an entry in bugs.dpdk.org?
[...]
> > I'm going to try to reproduce the issue, but I'm not sure I will
> > succeed. Could you please share the Vhost logs when the issue is
> > reproduced and you face the crash?
>
> With vacations and lab work it's unlikely anything can be done from my side for the next 2-3 weeks.
We can probably wait 3 more weeks.
> This issue has been reported multiple times from multiple telco customers for about a year, it's all over the mailing lists
> between ovs, dpdk and qemu, with all the details.
What was the reply on the DPDK mailing list? Any link?
> Something in the governance of these Open Source projects is clearly not working right, probably too much inward-focus between a small number of companies, but I digress.
Contributors to DPDK are from multiple companies,
but I agree we may need more help.
Thank you for bringing your help with this fix.
> I think Chenbo Xia already knows the context, and I suspect this is now considered a "security issue". The problem is, the information about all of this has been public already for a year.
OK
> I will again repost how to reproduce here:
Thanks it helps to have all infos in the same place.
[...]
> > It is a fix, so it should contains the Fixes tag, and also cc
> > stable@dpdk.org.
>
> After one year, it is time for Redhat and Intel or whatever the governance of this project is,
The DPDK governance is not owned by any company.
If you think there is an issue in a decision,
you can alert the Technical Board at techboard@dpdk.org.
> to mention if there is any intention to fix this or not,
> before I or anyone else at SUSE will invest any more of our time and efforts here.
I don't understand why we would not fix any issue.
I think the project is quite dynamic and fixing issues,
I am sorry if you have a different opinion.
> Any tags you need you can add as required.
It would be nice to add the tags as suggested in the next version.
The most important would be to know where the issue comes from.
If you can identify the original commit introducing the bug,
you can mark it with:
Fixes: <commit-sha1> ("<commit-title>")
This way, maintainers and users know where it should be backported.
If any question, feel free to ask, we are here to help.
Thanks for the effort
On 9/28/22 18:03, Thomas Monjalon wrote:
> 28/09/2022 17:21, Claudio Fontana:
>> On 9/28/22 16:37, Maxime Coquelin wrote:
>>> The title should be reworded, maybe something like below?
>>> "vhost: fix possible out of bound access in buffer vectors"
>>
>> possible, I leave it to you and other maintainers here now to figure out.
>
> Maxime is suggesting a reword to you for your next version.
>
>>> On 8/2/22 02:49, Claudio Fontana wrote:
> [...]
>>>> This should fix errors that have been reported in multiple occasions
>>>> from telcos to the DPDK, OVS and QEMU projects, as this affects in
>>>> particular the openvswitch/DPDK, QEMU vhost-user setup when the
>>>> guest DPDK application abruptly goes away via SIGKILL and then
>>>> reconnects.
>
> What are the "multiple occasions"? Is there an entry in bugs.dpdk.org?
>
> [...]
>>> I'm going to try to reproduce the issue, but I'm not sure I will
>>> succeed. Could you please share the Vhost logs when the issue is
>>> reproduced and you face the crash?
>>
>> With vacations and lab work it's unlikely anything can be done from my side for the next 2-3 weeks.
>
> We can probably wait 3 more weeks.
Yes please, because I fail to reproduce it (Fc35 on host, Ubuntu 18.05
in guest).
What I can see is that on guest testpmd crash, the host backend receives
VHOST_USER_GET_VRING_BASE requests that stop the vring processing. on
reconnect, the rings start to be processed only when it received all the
configuration requests from QEMU.
Note that I have tested it with VFIO in the guest because I could not
find the uio_pci_generic module in Ubuntu 18.04 cloud image.
>> This issue has been reported multiple times from multiple telco customers for about a year, it's all over the mailing lists
>> between ovs, dpdk and qemu, with all the details.
>
> What was the reply on the DPDK mailing list? Any link?
Please provide the links, it may help to understand the root cause if
these mail threads contain logs.
>
>> Something in the governance of these Open Source projects is clearly not working right, probably too much inward-focus between a small number of companies, but I digress.
>
> Contributors to DPDK are from multiple companies,
> but I agree we may need more help.
> Thank you for bringing your help with this fix.
>
>> I think Chenbo Xia already knows the context, and I suspect this is now considered a "security issue". The problem is, the information about all of this has been public already for a year.
>
> OK
>
>> I will again repost how to reproduce here:
>
> Thanks it helps to have all infos in the same place.
>
> [...]
>
>>> It is a fix, so it should contains the Fixes tag, and also cc
>>> stable@dpdk.org.
>>
>> After one year, it is time for Redhat and Intel or whatever the governance of this project is,
>
> The DPDK governance is not owned by any company.
> If you think there is an issue in a decision,
> you can alert the Technical Board at techboard@dpdk.org.
>
>> to mention if there is any intention to fix this or not,
>> before I or anyone else at SUSE will invest any more of our time and efforts here.
>
> I don't understand why we would not fix any issue.
> I think the project is quite dynamic and fixing issues,
> I am sorry if you have a different opinion.
>
>> Any tags you need you can add as required.
>
> It would be nice to add the tags as suggested in the next version.
> The most important would be to know where the issue comes from.
> If you can identify the original commit introducing the bug,
> you can mark it with:
> Fixes: <commit-sha1> ("<commit-title>")
> This way, maintainers and users know where it should be backported.
>
> If any question, feel free to ask, we are here to help.
> Thanks for the effort
>
>
On 8/2/22 02:49, Claudio Fontana wrote:
> in virtio_dev_split we cannot currently call desc_to_mbuf with
> nr_vec == 0, or we end up trying to rte_memcpy from a source address
> buf_vec[0] that is an uninitialized stack variable.
>
> Improve this in general by having desc_to_mbuf and mbuf_to_desc
> return -1 when called with an invalid nr_vec == 0, which should
> fix any other instance of this problem.
>
> This should fix errors that have been reported in multiple occasions
> from telcos to the DPDK, OVS and QEMU projects, as this affects in
> particular the openvswitch/DPDK, QEMU vhost-user setup when the
> guest DPDK application abruptly goes away via SIGKILL and then
> reconnects.
>
> The back trace looks roughly like this, depending on the specific
> rte_memcpy selected, etc, in any case the "src" parameter is garbage
> (in this example containing 0 + dev->host_hlen(12 = 0xc)).
>
> Thread 153 "pmd-c88/id:150" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7f64e5e6b700 (LWP 141373)]
> rte_mov128blocks (n=2048, src=0xc <error: Cannot access memory at 0xc>,
> dst=0x150da4480) at ../lib/eal/x86/include/rte_memcpy.h:384
> (gdb) bt
> 0 rte_mov128blocks (n=2048, src=0xc, dst=0x150da4480)
> 1 rte_memcpy_generic (n=2048, src=0xc, dst=0x150da4480)
> 2 rte_memcpy (n=2048, src=0xc, dst=<optimized out>)
> 3 sync_fill_seg
> 4 desc_to_mbuf
> 5 virtio_dev_tx_split
> 6 virtio_dev_tx_split_legacy
> 7 0x00007f676fea0fef in rte_vhost_dequeue_burst
> 8 0x00007f6772005a62 in netdev_dpdk_vhost_rxq_recv
> 9 0x00007f6771f38116 in netdev_rxq_recv
> 10 0x00007f6771f03d96 in dp_netdev_process_rxq_port
> 11 0x00007f6771f04239 in pmd_thread_main
> 12 0x00007f6771f92aff in ovsthread_wrapper
> 13 0x00007f6771c1b6ea in start_thread
> 14 0x00007f6771933a8f in clone
>
> Tested-by: Claudio Fontana <cfontana@suse.de>
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
> lib/vhost/virtio_net.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
This patch is also no more necessary since CVE-2022-2132 has been fixed.
Latests LTS versions and upstream main branch contain the fixes:
dc1516e260a0 ("vhost: fix header spanned across more than two descriptors")
71bd0cc536ad ("vhost: discard too small descriptor chains")
desc_to_mbuf callers now check that the descriptors are at least the
size of the virtio_net header, so nr_vec cannot be 0 in desc_to_mbuf.
Since I cannot reproduce, if you are willing to try please let us know
the results.
Maxime
On 10/5/22 17:06, Maxime Coquelin wrote:
>
>
> On 8/2/22 02:49, Claudio Fontana wrote:
>> in virtio_dev_split we cannot currently call desc_to_mbuf with
>> nr_vec == 0, or we end up trying to rte_memcpy from a source address
>> buf_vec[0] that is an uninitialized stack variable.
>>
>> Improve this in general by having desc_to_mbuf and mbuf_to_desc
>> return -1 when called with an invalid nr_vec == 0, which should
>> fix any other instance of this problem.
>>
>> This should fix errors that have been reported in multiple occasions
>> from telcos to the DPDK, OVS and QEMU projects, as this affects in
>> particular the openvswitch/DPDK, QEMU vhost-user setup when the
>> guest DPDK application abruptly goes away via SIGKILL and then
>> reconnects.
>>
>> The back trace looks roughly like this, depending on the specific
>> rte_memcpy selected, etc, in any case the "src" parameter is garbage
>> (in this example containing 0 + dev->host_hlen(12 = 0xc)).
>>
>> Thread 153 "pmd-c88/id:150" received signal SIGSEGV, Segmentation fault.
>> [Switching to Thread 0x7f64e5e6b700 (LWP 141373)]
>> rte_mov128blocks (n=2048, src=0xc <error: Cannot access memory at 0xc>,
>> dst=0x150da4480) at ../lib/eal/x86/include/rte_memcpy.h:384
>> (gdb) bt
>> 0 rte_mov128blocks (n=2048, src=0xc, dst=0x150da4480)
>> 1 rte_memcpy_generic (n=2048, src=0xc, dst=0x150da4480)
>> 2 rte_memcpy (n=2048, src=0xc, dst=<optimized out>)
>> 3 sync_fill_seg
>> 4 desc_to_mbuf
>> 5 virtio_dev_tx_split
>> 6 virtio_dev_tx_split_legacy
>> 7 0x00007f676fea0fef in rte_vhost_dequeue_burst
>> 8 0x00007f6772005a62 in netdev_dpdk_vhost_rxq_recv
>> 9 0x00007f6771f38116 in netdev_rxq_recv
>> 10 0x00007f6771f03d96 in dp_netdev_process_rxq_port
>> 11 0x00007f6771f04239 in pmd_thread_main
>> 12 0x00007f6771f92aff in ovsthread_wrapper
>> 13 0x00007f6771c1b6ea in start_thread
>> 14 0x00007f6771933a8f in clone
>>
>> Tested-by: Claudio Fontana <cfontana@suse.de>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>> lib/vhost/virtio_net.c | 11 ++++++++---
>> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> This patch is also no more necessary since CVE-2022-2132 has been fixed.
> Latests LTS versions and upstream main branch contain the fixes:
>
> dc1516e260a0 ("vhost: fix header spanned across more than two descriptors")
> 71bd0cc536ad ("vhost: discard too small descriptor chains")
>
> desc_to_mbuf callers now check that the descriptors are at least the
> size of the virtio_net header, so nr_vec cannot be 0 in desc_to_mbuf.
>
> Since I cannot reproduce, if you are willing to try please let us know
> the results.
>
> Maxime
>
Hello Maxime,
which versions of DPDK did you get to test in the guest? The problem seems to be easier to reproduce when DPDK 16.x is in the guest.
The guest OS where the problem was encountered in the field is "Ubuntu 16.04", but we were able to reproduce this also in our lab with ubuntu20.04.
For reproduction we have used a few network cards, mainly Intel X520.
I'll let you know our results as I have them.
Thanks,
CLaudio
On 11/2/22 11:34, Claudio Fontana wrote:
> On 10/5/22 17:06, Maxime Coquelin wrote:
>>
>>
>> On 8/2/22 02:49, Claudio Fontana wrote:
>>> in virtio_dev_split we cannot currently call desc_to_mbuf with
>>> nr_vec == 0, or we end up trying to rte_memcpy from a source address
>>> buf_vec[0] that is an uninitialized stack variable.
>>>
>>> Improve this in general by having desc_to_mbuf and mbuf_to_desc
>>> return -1 when called with an invalid nr_vec == 0, which should
>>> fix any other instance of this problem.
>>>
>>> This should fix errors that have been reported in multiple occasions
>>> from telcos to the DPDK, OVS and QEMU projects, as this affects in
>>> particular the openvswitch/DPDK, QEMU vhost-user setup when the
>>> guest DPDK application abruptly goes away via SIGKILL and then
>>> reconnects.
>>>
>>> The back trace looks roughly like this, depending on the specific
>>> rte_memcpy selected, etc, in any case the "src" parameter is garbage
>>> (in this example containing 0 + dev->host_hlen(12 = 0xc)).
>>>
>>> Thread 153 "pmd-c88/id:150" received signal SIGSEGV, Segmentation fault.
>>> [Switching to Thread 0x7f64e5e6b700 (LWP 141373)]
>>> rte_mov128blocks (n=2048, src=0xc <error: Cannot access memory at 0xc>,
>>> dst=0x150da4480) at ../lib/eal/x86/include/rte_memcpy.h:384
>>> (gdb) bt
>>> 0 rte_mov128blocks (n=2048, src=0xc, dst=0x150da4480)
>>> 1 rte_memcpy_generic (n=2048, src=0xc, dst=0x150da4480)
>>> 2 rte_memcpy (n=2048, src=0xc, dst=<optimized out>)
>>> 3 sync_fill_seg
>>> 4 desc_to_mbuf
>>> 5 virtio_dev_tx_split
>>> 6 virtio_dev_tx_split_legacy
>>> 7 0x00007f676fea0fef in rte_vhost_dequeue_burst
>>> 8 0x00007f6772005a62 in netdev_dpdk_vhost_rxq_recv
>>> 9 0x00007f6771f38116 in netdev_rxq_recv
>>> 10 0x00007f6771f03d96 in dp_netdev_process_rxq_port
>>> 11 0x00007f6771f04239 in pmd_thread_main
>>> 12 0x00007f6771f92aff in ovsthread_wrapper
>>> 13 0x00007f6771c1b6ea in start_thread
>>> 14 0x00007f6771933a8f in clone
>>>
>>> Tested-by: Claudio Fontana <cfontana@suse.de>
>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>> ---
>>> lib/vhost/virtio_net.c | 11 ++++++++---
>>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> This patch is also no more necessary since CVE-2022-2132 has been fixed.
>> Latests LTS versions and upstream main branch contain the fixes:
>>
>> dc1516e260a0 ("vhost: fix header spanned across more than two descriptors")
>> 71bd0cc536ad ("vhost: discard too small descriptor chains")
>>
>> desc_to_mbuf callers now check that the descriptors are at least the
>> size of the virtio_net header, so nr_vec cannot be 0 in desc_to_mbuf.
>>
>> Since I cannot reproduce, if you are willing to try please let us know
>> the results.
>>
>> Maxime
>>
>
> Hello Maxime,
>
> which versions of DPDK did you get to test in the guest? The problem seems to be easier to reproduce when DPDK 16.x is in the guest.
> The guest OS where the problem was encountered in the field is "Ubuntu 16.04", but we were able to reproduce this also in our lab with ubuntu20.04.
> For reproduction we have used a few network cards, mainly Intel X520.
>
> I'll let you know our results as I have them.
>
> Thanks,
>
> CLaudio
Just to follow up on this, the problem is fully addressed by CVE-2022-2132.
Thanks,
Claudio
@@ -1153,7 +1153,7 @@ mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
struct virtio_net_hdr_mrg_rxbuf tmp_hdr, *hdr = NULL;
struct vhost_async *async = vq->async;
- if (unlikely(m == NULL))
+ if (unlikely(m == NULL) || nr_vec == 0)
return -1;
buf_addr = buf_vec[vec_idx].buf_addr;
@@ -2673,6 +2673,9 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
struct vhost_async *async = vq->async;
struct async_inflight_info *pkts_info;
+ if (unlikely(nr_vec == 0))
+ return -1;
+
buf_addr = buf_vec[vec_idx].buf_addr;
buf_iova = buf_vec[vec_idx].buf_iova;
buf_len = buf_vec[vec_idx].buf_len;
@@ -2917,9 +2920,11 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
vq->last_avail_idx + i,
&nr_vec, buf_vec,
&head_idx, &buf_len,
- VHOST_ACCESS_RO) < 0))
+ VHOST_ACCESS_RO) < 0)) {
+ dropped += 1;
+ i++;
break;
-
+ }
update_shadow_used_ring_split(vq, head_idx, 0);
err = virtio_dev_pktmbuf_prep(dev, pkts[i], buf_len);