[dpdk-dev] [PATCH 02/10] vdpa/sfc: add support for device initialization

[Xia, Chenbo]

Hi Vijay,

> -----Original Message-----
> From: Vijay Kumar Srivastava <vsrivast at xilinx.com>
> Sent: Monday, October 18, 2021 6:06 PM
> To: Xia, Chenbo <chenbo.xia at intel.com>; dev at dpdk.org
> Cc: maxime.coquelin at redhat.com; andrew.rybchenko at oktetlabs.ru; Harpreet Singh
> Anand <hanand at xilinx.com>; Praveen Kumar Jain <praveenj at xilinx.com>
> Subject: RE: [PATCH 02/10] vdpa/sfc: add support for device initialization
> 
> Hi Chenbo,
> 
> >-----Original Message-----
> >From: Xia, Chenbo <chenbo.xia at intel.com>
> >Sent: Saturday, October 9, 2021 8:36 AM
> >To: Vijay Kumar Srivastava <vsrivast at xilinx.com>; dev at dpdk.org
> >Cc: maxime.coquelin at redhat.com; andrew.rybchenko at oktetlabs.ru; Harpreet
> >Singh Anand <hanand at xilinx.com>; Praveen Kumar Jain <praveenj at xilinx.com>
> >Subject: RE: [PATCH 02/10] vdpa/sfc: add support for device initialization
> >
> >Hi Vijay,
> >
> >> -----Original Message-----
> >> From: Vijay Kumar Srivastava <vsrivast at xilinx.com>
> >> Sent: Saturday, October 2, 2021 1:32 AM
> >> To: Xia, Chenbo <chenbo.xia at intel.com>; dev at dpdk.org
> >> Cc: maxime.coquelin at redhat.com; andrew.rybchenko at oktetlabs.ru;
> >> Harpreet Singh Anand <hanand at xilinx.com>; Praveen Kumar Jain
> >> <praveenj at xilinx.com>
> >> Subject: RE: [PATCH 02/10] vdpa/sfc: add support for device
> >> initialization
> >>
> >> Hi Chenbo,
> >>
> >> >-----Original Message-----
> >> >From: Xia, Chenbo <chenbo.xia at intel.com>
> >> >Sent: Monday, September 6, 2021 8:32 AM
> >> >To: Vijay Kumar Srivastava <vsrivast at xilinx.com>; dev at dpdk.org
> >> >Cc: maxime.coquelin at redhat.com; andrew.rybchenko at oktetlabs.ru;
> >> >Harpreet Singh Anand <hanand at xilinx.com>; Praveen Kumar Jain
> >> ><praveenj at xilinx.com>
> >> >Subject: RE: [PATCH 02/10] vdpa/sfc: add support for device
> >> initialization
> 
> [Snip]
> 
> >I think your vdpa HW (let's say a VF) have two DMA regions: one in guest (w/o
> >vIOMMU) and the other in vdpa app. Both share the same IOVA address space,
> >and we don't want them overlap. Let's say we can make sure no overlap will
> >happen and take an example here: guest DMA region's IOVA (GPA) range is
> >0x0000 to 0x1000 and vdpa app's is 0x1000 to 0x2000. A malicious guest could
> >use a malicious driver to write 0x1500 in its virtio RX ring, so that HW will
> DMA
> >to that address when packets come. Then the malicious guest performed an
> >DMA to host memory. Although the guest does not know IOVA range of vdpa
> >app, he can randomly guess to do the attack.
> >
> >Any solution your HW/driver can prevent this from happening without PASID?
> >Or do I miss something here ?
> 
> Rx packet will carry headers making highly unlikely any proper MCDI data can
> be written to the IOVA address (for MCDI buffer) to work with by the FW.
> Writing to the buffer does not imply to issue the MCDI message. Even if MCDI
> is sent then FW is resilient enough to identify the incorrect MCDI and will
> reject the message.
> 
> This is going to affect only to VF on which malicious guest is present, as
> this MCDI buffer is specific to the corresponding VF.
> So it won't affect any control path operation on the any other VF or host.

OK. So it's very hard to do attack with the FW detection. But about 'won't affect
host', I think it depends on how you handle the DMA-ed control messages. Take a bad
example: if one DMA address saves a pointer and the malicious DMA makes the pointer
be NULL, it will segfaults the program (But I don't think this will happen in your driver,
just help you understand my point). So please check the control messages handling
is robust.

And in the future, I would like to see this problem solved by PASID when your HW has
the support.

> 
> For SW assisted Live migration implemented in the ifcvf vDPA driver it uses
> hard coded IOVA addresses for mediated vring. Could it have similar issue ?

Good point. It will and I think we may also need to check if will affect the host program,
or deprecated the feature later.

Thanks,
Chenbo