[PATCH v3 0/2] vhost fixes for OVS SIGSEGV in PMD

[PATCH v3 0/2] vhost fixes for OVS SIGSEGV in PMD

[Claudio Fontana]

This is an alternative, more general fix compared with PATCH v1,
and fixes style issues in v2.

The series fixes a segmentation fault in the OVS PMD thread when
resynchronizing with QEMU after the guest application has been killed
with SIGKILL (patch 1/2),

The segmentation fault can be caused by the guest DPDK application,
which is able this way to crash the OVS process on the host,
see the backtrace in patch 1/2.

Patch 2/2 is an additional improvement in the current error handling.

---
Changes from v2: fix warnings from checkpatch.
---

Changes from v1:

* patch 1/2: instead of only fixing virtio_dev_tx_split, put the check
  for nr_vec == 0 inside desc_to_mbuf and mbuf_to_desc, so that in no
  case they attempt to read and dereference addresses from the buf_vec[]
  array when it does not contain any valid elements.

---

For your review and comments,

Claudio

Claudio Fontana (2):
  vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc
  vhost: improve error handling in desc_to_mbuf

 lib/vhost/virtio_net.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

-- 
2.26.2

[PATCH v3 1/2] vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc

[Claudio Fontana]

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

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

[PATCH v3 2/2] vhost: improve error handling in desc_to_mbuf

[Claudio Fontana]

check when increasing vec_idx that it is still valid
in the (buf_len < dev->vhost_hlen) case too.

Tested-by: Claudio Fontana <cfontana@suse.de>
Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 lib/vhost/virtio_net.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index eb19e54c2b..20ed951979 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -2704,12 +2704,15 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	if (unlikely(buf_len < dev->vhost_hlen)) {
 		buf_offset = dev->vhost_hlen - buf_len;
 		vec_idx++;
+		if (unlikely(vec_idx >= nr_vec))
+			goto error;
 		buf_addr = buf_vec[vec_idx].buf_addr;
 		buf_iova = buf_vec[vec_idx].buf_iova;
 		buf_len = buf_vec[vec_idx].buf_len;
 		buf_avail  = buf_len - buf_offset;
 	} else if (buf_len == dev->vhost_hlen) {
-		if (unlikely(++vec_idx >= nr_vec))
+		vec_idx++;
+		if (unlikely(vec_idx >= nr_vec))
 			goto error;
 		buf_addr = buf_vec[vec_idx].buf_addr;
 		buf_iova = buf_vec[vec_idx].buf_iova;
-- 
2.26.2

Re: [PATCH v3 1/2] vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc

[Stephen Hemminger]

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.

Re: [PATCH v3 0/2] vhost fixes for OVS SIGSEGV in PMD

[Stephen Hemminger]

On Tue,  2 Aug 2022 02:49:36 +0200
Claudio Fontana <cfontana@suse.de> wrote:

> This is an alternative, more general fix compared with PATCH v1,
> and fixes style issues in v2.
> 
> The series fixes a segmentation fault in the OVS PMD thread when
> resynchronizing with QEMU after the guest application has been killed
> with SIGKILL (patch 1/2),
> 
> The segmentation fault can be caused by the guest DPDK application,
> which is able this way to crash the OVS process on the host,
> see the backtrace in patch 1/2.
> 
> Patch 2/2 is an additional improvement in the current error handling.

Checking for NULL and 0 is good on host side.
But guest should probably not be sending such a useless request?

Re: [PATCH v3 0/2] vhost fixes for OVS SIGSEGV in PMD

[Claudio Fontana]

On 8/2/22 03:40, Stephen Hemminger wrote:
> On Tue,  2 Aug 2022 02:49:36 +0200
> Claudio Fontana <cfontana@suse.de> wrote:
> 
>> This is an alternative, more general fix compared with PATCH v1,
>> and fixes style issues in v2.
>>
>> The series fixes a segmentation fault in the OVS PMD thread when
>> resynchronizing with QEMU after the guest application has been killed
>> with SIGKILL (patch 1/2),
>>
>> The segmentation fault can be caused by the guest DPDK application,
>> which is able this way to crash the OVS process on the host,
>> see the backtrace in patch 1/2.
>>
>> Patch 2/2 is an additional improvement in the current error handling.
> 
> Checking for NULL and 0 is good on host side.
> But guest should probably not be sending such a useless request?


Right, I focused on hardening the host side, as that is what the customer required.

This happens specifically when the guest application goes away abruptly and has no chance to signal anything (SIGKILL),
and at restart issues a virtio reset on the device, which in qemu causes also a (actually two) virtio_net set_status, which attempt to stop the queues (twice).

DPDK seems to think at that point that it needs to drain the queue, and tries to process MAX_PKT_BURST buffers
("about to dequeue 32 buffers"),

then calls fill_vec_buf_split and gets absolutely nothing.

I think this should also address the reports in this thread:

https://inbox.dpdk.org/dev/SA1PR08MB713373B0D19329C38C7527BB839A9@SA1PR08MB7133.namprd08.prod.outlook.com/

in addition to my specific customer request,

Thanks,

Claudio

Re: [PATCH v3 0/2] vhost fixes for OVS SIGSEGV in PMD

[Claudio Fontana]

On 8/2/22 19:20, Claudio Fontana wrote:
> On 8/2/22 03:40, Stephen Hemminger wrote:
>> On Tue,  2 Aug 2022 02:49:36 +0200
>> Claudio Fontana <cfontana@suse.de> wrote:
>>
>>> This is an alternative, more general fix compared with PATCH v1,
>>> and fixes style issues in v2.
>>>
>>> The series fixes a segmentation fault in the OVS PMD thread when
>>> resynchronizing with QEMU after the guest application has been killed
>>> with SIGKILL (patch 1/2),
>>>
>>> The segmentation fault can be caused by the guest DPDK application,
>>> which is able this way to crash the OVS process on the host,
>>> see the backtrace in patch 1/2.
>>>
>>> Patch 2/2 is an additional improvement in the current error handling.
>>
>> Checking for NULL and 0 is good on host side.
>> But guest should probably not be sending such a useless request?
> 
> 
> Right, I focused on hardening the host side, as that is what the customer required.
> 
> This happens specifically when the guest application goes away abruptly and has no chance to signal anything (SIGKILL),
> and at restart issues a virtio reset on the device, which in qemu causes also a (actually two) virtio_net set_status, which attempt to stop the queues (twice).
> 
> DPDK seems to think at that point that it needs to drain the queue, and tries to process MAX_PKT_BURST buffers
> ("about to dequeue 32 buffers"),
> 
> then calls fill_vec_buf_split and gets absolutely nothing.
> 
> I think this should also address the reports in this thread:
> 
> https://inbox.dpdk.org/dev/SA1PR08MB713373B0D19329C38C7527BB839A9@SA1PR08MB7133.namprd08.prod.outlook.com/
> 
> in addition to my specific customer request,
> 
> Thanks,
> 
> Claudio

anything more required from my side? Do you need a respin without the "Tested-by" tag?

Thanks,

Claudio

Re: [PATCH v3 0/2] vhost fixes for OVS SIGSEGV in PMD

[Claudio Fontana]

A weekly ping on this one,

any chance to get this fix for a guest-triggered host crash included?

Thanks,

Claudio

On 8/2/22 02:49, Claudio Fontana wrote:
> This is an alternative, more general fix compared with PATCH v1,
> and fixes style issues in v2.
> 
> The series fixes a segmentation fault in the OVS PMD thread when
> resynchronizing with QEMU after the guest application has been killed
> with SIGKILL (patch 1/2),
> 
> The segmentation fault can be caused by the guest DPDK application,
> which is able this way to crash the OVS process on the host,
> see the backtrace in patch 1/2.
> 
> Patch 2/2 is an additional improvement in the current error handling.
> 
> ---
> Changes from v2: fix warnings from checkpatch.
> ---
> 
> Changes from v1:
> 
> * patch 1/2: instead of only fixing virtio_dev_tx_split, put the check
>   for nr_vec == 0 inside desc_to_mbuf and mbuf_to_desc, so that in no
>   case they attempt to read and dereference addresses from the buf_vec[]
>   array when it does not contain any valid elements.
> 
> ---
> 
> For your review and comments,
> 
> Claudio
> 
> Claudio Fontana (2):
>   vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc
>   vhost: improve error handling in desc_to_mbuf
> 
>  lib/vhost/virtio_net.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 

Re: [PATCH v3 1/2] vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc

[Maxime Coquelin]

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

Re: [PATCH v3 1/2] vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc

[Claudio Fontana]

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

Re: [PATCH v3 1/2] vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc

[Thomas Monjalon]

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

Re: [PATCH v3 1/2] vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc

[Maxime Coquelin]

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

Re: [PATCH v3 2/2] vhost: improve error handling in desc_to_mbuf

[Maxime Coquelin]

On 8/2/22 02:49, Claudio Fontana wrote:
> check when increasing vec_idx that it is still valid
> in the (buf_len < dev->vhost_hlen) case too.
> 
> Tested-by: Claudio Fontana <cfontana@suse.de>
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>   lib/vhost/virtio_net.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index eb19e54c2b..20ed951979 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -2704,12 +2704,15 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
>   	if (unlikely(buf_len < dev->vhost_hlen)) {
>   		buf_offset = dev->vhost_hlen - buf_len;
>   		vec_idx++;
> +		if (unlikely(vec_idx >= nr_vec))
> +			goto error;
>   		buf_addr = buf_vec[vec_idx].buf_addr;
>   		buf_iova = buf_vec[vec_idx].buf_iova;
>   		buf_len = buf_vec[vec_idx].buf_len;
>   		buf_avail  = buf_len - buf_offset;
>   	} else if (buf_len == dev->vhost_hlen) {
> -		if (unlikely(++vec_idx >= nr_vec))
> +		vec_idx++;
> +		if (unlikely(vec_idx >= nr_vec))
>   			goto error;
>   		buf_addr = buf_vec[vec_idx].buf_addr;
>   		buf_iova = buf_vec[vec_idx].buf_iova;

This patch is no more required since fixes for CVE-2022-2132 takes care
of this:
dc1516e260a0 ("vhost: fix header spanned across more than two descriptors")
71bd0cc536ad ("vhost: discard too small descriptor chains")

Maxime

Re: [PATCH v3 1/2] vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc

[Maxime Coquelin]

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

Re: [PATCH v3 1/2] vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc

[Claudio Fontana]

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

Re: [PATCH v3 1/2] vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc

[Claudio Fontana]

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