thr3ads.net - Linux Virtualization - [PATCH v11 2/2] s390: virtio: PV needs VIRTIO I/O device protection [Sep 2020]

If this information is useful, please help other people find it:
Share via:

Pierre Morel

2020-Sep-07 09:39 UTC

[PATCH v11 0/2] s390: virtio: let arch validate VIRTIO features

Hi all,

The goal of the series is to give a chance to the architecture
to validate VIRTIO device features.

The tests are back to virtio_finalize_features.

No more argument for the architecture callback which only reports
if the architecture needs guest memory access restrictions for
VIRTIO.

I renamed the callback to arch_has_restricted_virtio_memory_access,
the config option to ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS,
and VIRTIO_F_IOMMU_PLATFORM to VIRTIO_F_ACCESS_PLATFORM.

Regards,
Pierre

Pierre Morel (2):
  virtio: let arch advertise guest's memory access restrictions
  s390: virtio: PV needs VIRTIO I/O device protection

 arch/s390/Kconfig             |  1 +
 arch/s390/mm/init.c           | 10 ++++++++++
 drivers/virtio/Kconfig        |  6 ++++++
 drivers/virtio/virtio.c       | 15 +++++++++++++++
 include/linux/virtio_config.h | 10 ++++++++++
 5 files changed, 42 insertions(+)

-- 
2.17.1

Changelog

to v11:
- replaced VIRTIO_F_IOMMU_PLATFORM with VIRTIO_F_ACCESS_PLATFORM

to v10:
- removed virtio_config.h unnecessary include
- wording
  (Connie)

to v9:

- move virtio tests back to virtio_finalize_features
  (Connie)

- remove virtio device argument

to v8:

- refactoring by using an optional callback
  (Connie)

to v7:

- typo in warning message
  (Connie)
to v6:

- rewording warning messages
  (Connie, Halil)

to v5:

- return directly from S390 arch_validate_virtio_features()
  when the guest is not protected.
  (Connie)

- Somme rewording
  (Connie, Michael)

- moved back code from arch/s390/ ...kernel/uv.c to ...mm/init.c
  (Christian)

to v4:

- separate virtio and arch code
  (Pierre)

- moved code from arch/s390/mm/init.c to arch/s390/kernel/uv.c
  (as interpreted from Heiko's comment)

- moved validation inside the arch code
  (Connie)

- moved the call to arch validation before VIRTIO_F_1 test
  (Michael)

to v3:

- add warning
  (Connie, Christian)

- add comment
  (Connie)

- change hook name
  (Halil, Connie)

to v2:

- put the test in virtio_finalize_features()
  (Connie)

- put the test inside VIRTIO core
  (Jason)

- pass a virtio device as parameter
  (Halil)

Pierre Morel

2020-Sep-07 09:39 UTC

head link

[PATCH v11 1/2] virtio: let arch advertise guest's memory access restrictions

An architecture may restrict host access to guest memory,
e.g. IBM s390 Secure Execution or AMD SEV.

Provide a new Kconfig entry the architecture can select,
CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS, when it provides
the arch_has_restricted_virtio_memory_access callback to advertise
to VIRTIO common code when the architecture restricts memory access
from the host.

The common code can then fail the probe for any device where
VIRTIO_F_ACCESS_PLATFORM is required, but not set.

Signed-off-by: Pierre Morel <pmorel at linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck at redhat.com>
---
 drivers/virtio/Kconfig        |  6 ++++++
 drivers/virtio/virtio.c       | 15 +++++++++++++++
 include/linux/virtio_config.h | 10 ++++++++++
 3 files changed, 31 insertions(+)

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 5c92e4a50882..3999b411624c 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -6,6 +6,12 @@ config VIRTIO
 	  bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG
 	  or CONFIG_S390_GUEST.
 
+config ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
+	bool
+	help
+	  This option is selected if the architecture may need to enforce
+	  VIRTIO_F_IOMMU_PLATFORM.
+
 menuconfig VIRTIO_MENU
 	bool "Virtio drivers"
 	default y
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index a977e32a88f2..a2b3f12e10a2 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -176,6 +176,21 @@ int virtio_finalize_features(struct virtio_device *dev)
 	if (ret)
 		return ret;
 
+	ret = arch_has_restricted_virtio_memory_access();
+	if (ret) {
+		if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
+			dev_warn(&dev->dev,
+				 "device must provide VIRTIO_F_VERSION_1\n");
+			return -ENODEV;
+		}
+
+		if (!virtio_has_feature(dev, VIRTIO_F_ACCESS_PLATFORM)) {
+			dev_warn(&dev->dev,
+				 "device must provide VIRTIO_F_ACCESS_PLATFORM\n");
+			return -ENODEV;
+		}
+	}
+
 	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
 		return 0;
 
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 8fe857e27ef3..3f697c8c8205 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -540,4 +540,14 @@ static inline void virtio_cwrite64(struct virtio_device
*vdev,
 			virtio_cread_le((vdev), structname, member, ptr); \
 		_r;							\
 	})
+
+#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
+int arch_has_restricted_virtio_memory_access(void);
+#else
+static inline int arch_has_restricted_virtio_memory_access(void)
+{
+	return 0;
+}
+#endif /* CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS */
+
 #endif /* _LINUX_VIRTIO_CONFIG_H */
-- 
2.17.1

Pierre Morel

2020-Sep-07 09:39 UTC

head link

[PATCH v11 2/2] s390: virtio: PV needs VIRTIO I/O device protection

If protected virtualization is active on s390, VIRTIO has only retricted
access to the guest memory.
Define CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS and export
arch_has_restricted_virtio_memory_access to advertize VIRTIO if that's
the case, preventing a host error on access attempt.

Signed-off-by: Pierre Morel <pmorel at linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck at redhat.com>
---
 arch/s390/Kconfig   |  1 +
 arch/s390/mm/init.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index b29fcc66ec39..938246200d39 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -820,6 +820,7 @@ menu "Virtualization"
 config PROTECTED_VIRTUALIZATION_GUEST
 	def_bool n
 	prompt "Protected virtualization guest support"
+	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
 	help
 	  Select this option, if you want to be able to run this
 	  kernel as a protected virtualization KVM guest.
diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 0d282081dc1f..f40b9b63d3d6 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -160,6 +160,16 @@ bool force_dma_unencrypted(struct device *dev)
 	return is_prot_virt_guest();
 }
 
+#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
+
+int arch_has_restricted_virtio_memory_access(void)
+{
+	return is_prot_virt_guest();
+}
+EXPORT_SYMBOL(arch_has_restricted_virtio_memory_access);
+
+#endif
+
 /* protected virtualization */
 static void pv_init(void)
 {
-- 
2.17.1

Halil Pasic

2020-Sep-07 22:22 UTC

head link

[PATCH v11 1/2] virtio: let arch advertise guest's memory access restrictions

On Mon,  7 Sep 2020 11:39:06 +0200
Pierre Morel <pmorel at linux.ibm.com> wrote:
> An architecture may restrict host access to guest memory,
> e.g. IBM s390 Secure Execution or AMD SEV.
> 
> Provide a new Kconfig entry the architecture can select,
> CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS, when it provides
> the arch_has_restricted_virtio_memory_access callback to advertise
> to VIRTIO common code when the architecture restricts memory access
> from the host.
> 
> The common code can then fail the probe for any device where
> VIRTIO_F_ACCESS_PLATFORM is required, but not set.
> 
> Signed-off-by: Pierre Morel <pmorel at linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck at redhat.com>
Reviewed-by: Halil Pasic <pasic at linux.ibm.com>

[..]>  
> +config ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> +	bool
> +	help
> +	  This option is selected if the architecture may need to enforce
> +	  VIRTIO_F_IOMMU_PLATFORM.
> +
A small nit: you use F_ACCESS_PLATFORM everywhere but here.

Regards,
Halil

Halil Pasic

2020-Sep-07 22:37 UTC

head link

[PATCH v11 2/2] s390: virtio: PV needs VIRTIO I/O device protection

On Mon,  7 Sep 2020 11:39:07 +0200
Pierre Morel <pmorel at linux.ibm.com> wrote:
> If protected virtualization is active on s390, VIRTIO has only retricted
> access to the guest memory.
> Define CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS and export
> arch_has_restricted_virtio_memory_access to advertize VIRTIO if that's
> the case, preventing a host error on access attempt.
The description is a little inaccurate, but I don't care hence the r-b.

The function arch_has_restricted_virtio_memory_access() returning true
can not prevent the host from attempting to access memory if it decides
to do so. And as far as I know there was no host error on access attempt.
The page gets exported, and the host will operate on the encrypted
page. But in the end we do run into trouble, which is usually fatal for
the guest (not the host).

What we actually do here is the following. If we detect
an ill configured device we fail it (device status field), because
attempting to drive it is a recipe for disaster.
> 
> Signed-off-by: Pierre Morel <pmorel at linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck at redhat.com>
Reviewed-by: Halil Pasic <pasic at linux.ibm.com>

Halil Pasic

2020-Sep-07 22:39 UTC

head link

[PATCH v11 0/2] s390: virtio: let arch validate VIRTIO features

On Mon,  7 Sep 2020 11:39:05 +0200
Pierre Morel <pmorel at linux.ibm.com> wrote:
> Hi all,
> 
> The goal of the series is to give a chance to the architecture
> to validate VIRTIO device features.
Michael, is this going in via your tree?

Cornelia Huck

2020-Sep-08 06:55 UTC

head link

[PATCH v11 0/2] s390: virtio: let arch validate VIRTIO features

On Tue, 8 Sep 2020 00:39:51 +0200
Halil Pasic <pasic at linux.ibm.com> wrote:
> On Mon,  7 Sep 2020 11:39:05 +0200
> Pierre Morel <pmorel at linux.ibm.com> wrote:
> 
> > Hi all,
> > 
> > The goal of the series is to give a chance to the architecture
> > to validate VIRTIO device features.  
> 
> Michael, is this going in via your tree?
> 
I believe Michael's tree is the right place for this, but I can also
queue it if I get an ack on patch 1.

Michael S. Tsirkin

2020-Sep-08 07:57 UTC

head link

[PATCH v11 0/2] s390: virtio: let arch validate VIRTIO features

On Tue, Sep 08, 2020 at 12:39:51AM +0200, Halil Pasic
wrote:> On Mon,  7 Sep 2020 11:39:05 +0200
> Pierre Morel <pmorel at linux.ibm.com> wrote:
> 
> > Hi all,
> > 
> > The goal of the series is to give a chance to the architecture
> > to validate VIRTIO device features.
> 
> Michael, is this going in via your tree?
I guess so. Still not really happy about second-guessing
the hypervisor, but this got acks from others
so maybe I'm wrong in this instance. Won't be the first time.

-- 
MST

kernel test robot

2020-Sep-08 13:39 UTC

head link

[PATCH v11 2/2] s390: virtio: PV needs VIRTIO I/O device protection

Hi Pierre,

I love your patch! Perhaps something to improve:

[auto build test WARNING on s390/features]
[also build test WARNING on linus/master v5.9-rc4 next-20200903]
[cannot apply to linux/master kvms390/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:   
https://github.com/0day-ci/linux/commits/Pierre-Morel/s390-virtio-let-arch-validate-VIRTIO-features/20200907-174101
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: s390-randconfig-r006-20200908 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project
5f5a0bb0872a9673bad08b38bc0b14c42263902a)
reproduce (this is a W=1 build):
        wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp at intel.com>

All warnings (new ones prefixed by >>):

   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro
'__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:20:12: note: expanded from macro
'___constant_swab32'
           (((__u32)(x) & (__u32)0x0000ff00UL) <<  8) |            \
                     ^
   In file included from arch/s390/mm/init.c:20:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:22:
   In file included from include/linux/writeback.h:14:
   In file included from include/linux/blk-cgroup.h:23:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro
'__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro
'__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:21:12: note: expanded from macro
'___constant_swab32'
           (((__u32)(x) & (__u32)0x00ff0000UL) >>  8) |            \
                     ^
   In file included from arch/s390/mm/init.c:20:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:22:
   In file included from include/linux/writeback.h:14:
   In file included from include/linux/blk-cgroup.h:23:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro
'__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro
'__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:22:12: note: expanded from macro
'___constant_swab32'
           (((__u32)(x) & (__u32)0xff000000UL) >> 24)))
                     ^
   In file included from arch/s390/mm/init.c:20:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:22:
   In file included from include/linux/writeback.h:14:
   In file included from include/linux/blk-cgroup.h:23:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro
'__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:120:12: note: expanded from macro
'__swab32'
           __fswab32(x))
                     ^
   In file included from arch/s390/mm/init.c:20:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:22:
   In file included from include/linux/writeback.h:14:
   In file included from include/linux/blk-cgroup.h:23:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^>> arch/s390/mm/init.c:165:5: warning: no previous prototype for function
'arch_has_restricted_virtio_memory_access' [-Wmissing-prototypes]   int arch_has_restricted_virtio_memory_access(void)
       ^
   arch/s390/mm/init.c:165:1: note: declare 'static' if the function is
not intended to be used outside of this translation unit
   int arch_has_restricted_virtio_memory_access(void)
   ^
   static 
   In file included from arch/s390/mm/init.c:11:
   In file included from include/linux/signal.h:5:
   In file included from include/linux/bug.h:5:
   In file included from arch/s390/include/asm/bug.h:68:
   In file included from include/asm-generic/bug.h:20:
   In file included from include/linux/kernel.h:12:
   In file included from include/linux/bitops.h:29:
   arch/s390/include/asm/bitops.h:69:4: error: invalid operand in inline asm:
'oi	$0,${1:b}'
                           "oi     %0,%b1\n"
                           ^
   arch/s390/include/asm/bitops.h:69:4: error: invalid operand in inline asm:
'oi	$0,${1:b}'
   arch/s390/include/asm/bitops.h:69:4: error: invalid operand in inline asm:
'oi	$0,${1:b}'
   21 warnings and 3 errors generated.

#
https://github.com/0day-ci/linux/commit/e047ab912af7232ee2d97fd0cb83046c10aca7cf
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Pierre-Morel/s390-virtio-let-arch-validate-VIRTIO-features/20200907-174101
git checkout e047ab912af7232ee2d97fd0cb83046c10aca7cf
vim +/arch_has_restricted_virtio_memory_access +165 arch/s390/mm/init.c

   164	
 > 165	int arch_has_restricted_virtio_memory_access(void)
   166	{
   167		return is_prot_virt_guest();
   168	}
   169	EXPORT_SYMBOL(arch_has_restricted_virtio_memory_access);
   170	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all at lists.01.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 19570 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20200908/94a29aba/attachment-0001.gz>

Possibly Parallel Threads

Search for more apparently analagous threads

Linux Virtualization - Sep 2020 - [PATCH v11 2/2] s390: virtio: PV needs VIRTIO I/O device protection

[PATCH v11 0/2] s390: virtio: let arch validate VIRTIO features

[PATCH v11 1/2] virtio: let arch advertise guest's memory access restrictions

[PATCH v11 2/2] s390: virtio: PV needs VIRTIO I/O device protection

[PATCH v11 1/2] virtio: let arch advertise guest's memory access restrictions

[PATCH v11 2/2] s390: virtio: PV needs VIRTIO I/O device protection

[PATCH v11 0/2] s390: virtio: let arch validate VIRTIO features

[PATCH v11 0/2] s390: virtio: let arch validate VIRTIO features

[PATCH v11 0/2] s390: virtio: let arch validate VIRTIO features

[PATCH v11 2/2] s390: virtio: PV needs VIRTIO I/O device protection

Possibly Parallel Threads