[PATCH v1 0/3] virtio-mem: disallow mapping virtio-mem memory via /dev/mem

[PATCH v1 0/3] virtio-mem: disallow mapping virtio-mem memory via /dev/mem

[David Hildenbrand]

Let's add the basic infrastructure to exclude some physical memory
regions completely from /dev/mem access, on any architecture and under
any system configuration (independent of CONFIG_STRICT_DEVMEM and
independent of "iomem=").

Use it for virtio-mem, to disallow mapping any virtio-mem memory via
/dev/mem to user space after the virtio-mem driver was loaded: there is
no sane use case to access the device-managed memory region via /dev/mem
once the driver is actively (un)plugging memory within that region and
we want to make sure that nobody will accidentially access unplugged
memory in a sane environment.

Details can be found in patch #1.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Hanjun Guo <guohanjun@huawei.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: virtualization@lists.linux-foundation.org
Cc: linux-mm@kvack.org

David Hildenbrand (3):
  /dev/mem: disallow access to explicitly excluded system RAM regions
  virtio-mem: disallow mapping virtio-mem memory via /dev/mem
  kernel/resource: cleanup and optimize iomem_is_exclusive()

 drivers/char/mem.c          | 22 ++++++-------
 drivers/virtio/virtio_mem.c |  4 ++-
 include/linux/ioport.h      |  1 +
 kernel/resource.c           | 61 ++++++++++++++++++++++++++++++++-----
 lib/Kconfig.debug           |  4 ++-
 5 files changed, 69 insertions(+), 23 deletions(-)

-- 
2.31.1

[PATCH v1 1/3] /dev/mem: disallow access to explicitly excluded system RAM regions

[David Hildenbrand]

virtio-mem dynamically exposes memory inside a device memory region as
system RAM to Linux, coordinating with the hypervisor which parts are
actually "plugged" and consequently usable/accessible. On the one hand, the
virtio-mem driver adds/removes whole memory blocks, creating/removing busy
IORESOURCE_SYSTEM_RAM resources, on the other hand, it logically (un)plugs
memory inside added memory blocks, dynamically either exposing them to
the buddy or hiding them from the buddy and marking them PG_offline.

virtio-mem wants to make sure that in a sane environment, nobody
"accidentially" accesses unplugged memory inside the device managed
region. After /proc/kcore has been sanitized and /dev/kmem has been
removed, /dev/mem is the remaining interface that still allows uncontrolled
access to the device-managed region of virtio-mem devices from user
space.

There is no known sane use case for mapping virtio-mem device memory
via /dev/mem while virtio-mem driver concurrently (un)plugs memory inside
that region. So once the driver was loaded and detected the device
along the device-managed region, we just want to disallow any access via
/dev/mem to it.

Let's add the basic infrastructure to exclude some physical memory
regions completely from /dev/mem access, on any architecture and under
any system configuration (independent of CONFIG_STRICT_DEVMEM and
independent of "iomem=").

Any range marked with "IORESOURCE_SYSTEM_RAM | IORESOURCE_EXCLUSIVE"
will be excluded, even if not busy. For now, there are no applicable
ranges and we'll modify virtio-mem next to properly set
IORESOURCE_EXCLUSIVE on the parent resource.

As next_resource() will iterate over children although we might want to
skip a certain range completely, let's add and use
next_range_skip_children() to optimize that case, avoding having to
traverse subtrees that are not of interest.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 drivers/char/mem.c     | 22 +++++++++-------------
 include/linux/ioport.h |  1 +
 kernel/resource.c      | 42 ++++++++++++++++++++++++++++++++++++++++++
 lib/Kconfig.debug      |  4 +++-
 4 files changed, 55 insertions(+), 14 deletions(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 1c596b5cdb27..bb6d95daab45 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -60,13 +60,18 @@ static inline int valid_mmap_phys_addr_range(unsigned long pfn, size_t size)
 }
 #endif
 
-#ifdef CONFIG_STRICT_DEVMEM
 static inline int page_is_allowed(unsigned long pfn)
 {
-	return devmem_is_allowed(pfn);
+#ifdef CONFIG_STRICT_DEVMEM
+	if (!devmem_is_allowed(pfn))
+		return 0;
+#endif /* CONFIG_STRICT_DEVMEM */
+	return !iomem_range_contains_excluded(PFN_PHYS(pfn), PAGE_SIZE);
 }
+
 static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 {
+#ifdef CONFIG_STRICT_DEVMEM
 	u64 from = ((u64)pfn) << PAGE_SHIFT;
 	u64 to = from + size;
 	u64 cursor = from;
@@ -77,18 +82,9 @@ static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 		cursor += PAGE_SIZE;
 		pfn++;
 	}
-	return 1;
-}
-#else
-static inline int page_is_allowed(unsigned long pfn)
-{
-	return 1;
-}
-static inline int range_is_allowed(unsigned long pfn, unsigned long size)
-{
-	return 1;
+#endif /* CONFIG_STRICT_DEVMEM */
+	return !iomem_range_contains_excluded(PFN_PHYS(pfn), size);
 }
-#endif
 
 #ifndef unxlate_dev_mem_ptr
 #define unxlate_dev_mem_ptr unxlate_dev_mem_ptr
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 8359c50f9988..50523c28a5f1 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -308,6 +308,7 @@ extern struct resource * __devm_request_region(struct device *dev,
 extern void __devm_release_region(struct device *dev, struct resource *parent,
 				  resource_size_t start, resource_size_t n);
 extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size);
+extern bool iomem_range_contains_excluded(u64 addr, u64 size);
 extern bool iomem_is_exclusive(u64 addr);
 
 extern int
diff --git a/kernel/resource.c b/kernel/resource.c
index ca9f5198a01f..2938cf520ca3 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -73,6 +73,13 @@ static struct resource *next_resource(struct resource *p)
 	return p->sibling;
 }
 
+static struct resource *next_resource_skip_children(struct resource *p)
+{
+	while (!p->sibling && p->parent)
+		p = p->parent;
+	return p->sibling;
+}
+
 static void *r_next(struct seq_file *m, void *v, loff_t *pos)
 {
 	struct resource *p = v;
@@ -1700,6 +1707,41 @@ int iomem_map_sanity_check(resource_size_t addr, unsigned long size)
 	return err;
 }
 
+/*
+ * Check if a physical memory range is completely excluded from getting
+ * mapped/accessed via /dev/mem.
+ */
+bool iomem_range_contains_excluded(u64 addr, u64 size)
+{
+	const unsigned int flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_EXCLUSIVE;
+	bool excluded = false;
+	struct resource *p;
+
+	read_lock(&resource_lock);
+	for (p = iomem_resource.child; p ;) {
+		if (p->start >= addr + size)
+			break;
+		if (p->end < addr) {
+			/* No need to consider children */
+			p = next_resource_skip_children(p);
+			continue;
+		}
+		/*
+		 * A system RAM resource is excluded if IORESOURCE_EXCLUSIVE
+		 * is set, even if not busy and even if we don't have strict
+		 * checks enabled -- no ifs or buts.
+		 */
+		if ((p->flags & flags) == flags) {
+			excluded = true;
+			break;
+		}
+		p = next_resource(p);
+	}
+	read_unlock(&resource_lock);
+
+	return excluded;
+}
+
 #ifdef CONFIG_STRICT_DEVMEM
 static int strict_iomem_checks = 1;
 #else
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5ddd575159fb..d0ce6e23a6db 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1780,7 +1780,9 @@ config STRICT_DEVMEM
 	  access to this is obviously disastrous, but specific access can
 	  be used by people debugging the kernel. Note that with PAT support
 	  enabled, even in this case there are restrictions on /dev/mem
-	  use due to the cache aliasing requirements.
+	  use due to the cache aliasing requirements. Further, some drivers
+	  will still restrict access to some physical memory regions either
+	  already used or to be used in the future as system RAM.
 
 	  If this option is switched on, and IO_STRICT_DEVMEM=n, the /dev/mem
 	  file only allows userspace access to PCI space and the BIOS code and
-- 
2.31.1

[PATCH v1 2/3] virtio-mem: disallow mapping virtio-mem memory via /dev/mem

[David Hildenbrand]

By creating our parent IORESOURCE_SYSTEM_RAM resource with
IORESOURCE_EXCLUSIVE, we will disallow any /dev/mem access to our
device-managed region.

Note that access to the region would still be possible if someone simply
doesn't load the virtio-mem driver; however, there is no way of
protecting against someone that just wants to do nasty things.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 drivers/virtio/virtio_mem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index 09ed55de07d7..c8f914700a42 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -2516,8 +2516,10 @@ static int virtio_mem_create_resource(struct virtio_mem *vm)
 	if (!name)
 		return -ENOMEM;
 
+	/* Disallow mapping device memory via /dev/mem completely. */
 	vm->parent_resource = __request_mem_region(vm->addr, vm->region_size,
-						   name, IORESOURCE_SYSTEM_RAM);
+						   name, IORESOURCE_SYSTEM_RAM |
+						   IORESOURCE_EXCLUSIVE);
 	if (!vm->parent_resource) {
 		kfree(name);
 		dev_warn(&vm->vdev->dev, "could not reserve device region\n");
-- 
2.31.1

[PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive()

[David Hildenbrand]

Let's clean it up a bit, removing the unnecessary usage of r_next() by
next_resource(), and use next_range_resource() in case we are not
interested in a certain subtree.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 kernel/resource.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 2938cf520ca3..ea853a075a83 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1754,9 +1754,8 @@ static int strict_iomem_checks;
  */
 bool iomem_is_exclusive(u64 addr)
 {
-	struct resource *p = &iomem_resource;
+	struct resource *p;
 	bool err = false;
-	loff_t l;
 	int size = PAGE_SIZE;
 
 	if (!strict_iomem_checks)
@@ -1765,27 +1764,31 @@ bool iomem_is_exclusive(u64 addr)
 	addr = addr & PAGE_MASK;
 
 	read_lock(&resource_lock);
-	for (p = p->child; p ; p = r_next(NULL, p, &l)) {
+	for (p = iomem_resource.child; p ;) {
 		/*
 		 * We can probably skip the resources without
 		 * IORESOURCE_IO attribute?
 		 */
 		if (p->start >= addr + size)
 			break;
-		if (p->end < addr)
+		if (p->end < addr) {
+			/* No need to consider children */
+			p = next_resource_skip_children(p);
 			continue;
+		}
+
 		/*
 		 * A resource is exclusive if IORESOURCE_EXCLUSIVE is set
 		 * or CONFIG_IO_STRICT_DEVMEM is enabled and the
 		 * resource is busy.
 		 */
-		if ((p->flags & IORESOURCE_BUSY) == 0)
-			continue;
-		if (IS_ENABLED(CONFIG_IO_STRICT_DEVMEM)
-				|| p->flags & IORESOURCE_EXCLUSIVE) {
+		if (p->flags & IORESOURCE_BUSY &&
+		    (IS_ENABLED(CONFIG_IO_STRICT_DEVMEM) ||
+		     p->flags & IORESOURCE_EXCLUSIVE)) {
 			err = true;
 			break;
 		}
+		p = next_resource(p);
 	}
 	read_unlock(&resource_lock);
 
-- 
2.31.1

Re: [PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive()

[David Hildenbrand]

On 11.08.21 22:47, Andy Shevchenko wrote:
> 
> 
> On Wednesday, August 11, 2021, David Hildenbrand <david@redhat.com 
> <mailto:david@redhat.com>> wrote:
> 
>     Let's clean it up a bit, removing the unnecessary usage of r_next() by
>     next_resource(), and use next_range_resource() in case we are not
>     interested in a certain subtree.
> 
>     Signed-off-by: David Hildenbrand <david@redhat.com
>     <mailto:david@redhat.com>>
>     ---
>       kernel/resource.c | 19 +++++++++++--------
>       1 file changed, 11 insertions(+), 8 deletions(-)
> 
>     diff --git a/kernel/resource.c b/kernel/resource.c
>     index 2938cf520ca3..ea853a075a83 100644
>     --- a/kernel/resource.c
>     +++ b/kernel/resource.c
>     @@ -1754,9 +1754,8 @@ static int strict_iomem_checks;
>        */
>       bool iomem_is_exclusive(u64 addr)
>       {
>     -       struct resource *p = &iomem_resource;
>     +       struct resource *p;
>              bool err = false;
>     -       loff_t l;
>              int size = PAGE_SIZE;
> 
>              if (!strict_iomem_checks)
>     @@ -1765,27 +1764,31 @@ bool iomem_is_exclusive(u64 addr)
>              addr = addr & PAGE_MASK;
> 
>              read_lock(&resource_lock);
>     -       for (p = p->child; p ; p = r_next(NULL, p, &l)) {
>     +       for (p = iomem_resource.child; p ;) {
> 

Hi Andy,

> 
> I consider the ordinal part of p initialization is slightly better and 
> done outside of read lock.
> 
> Something like
> p= &iomem_res...;
> read lock
> for (p = p->child; ...) {

Why should we care about doing that outside of the lock? That smells 
like a micro-optimization the compiler will most probably overwrite 
either way as the address of iomem_resource is just constant?

Also, for me it's much more readable and compact if we perform a single 
initialization instead of two separate ones in this case.

We're using the pattern I use in, find_next_iomem_res() and 
__region_intersects(), while we use the old pattern in 
iomem_map_sanity_check(), where we also use the same unnecessary 
r_next() call.

I might just cleanup iomem_map_sanity_check() in a similar way.

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive()

[David Hildenbrand]

On 12.08.21 09:14, Andy Shevchenko wrote:
> 
> 
> On Thursday, August 12, 2021, David Hildenbrand <david@redhat.com 
> <mailto:david@redhat.com>> wrote:
> 
>     On 11.08.21 22:47, Andy Shevchenko wrote:
> 
> 
> 
>         On Wednesday, August 11, 2021, David Hildenbrand
>         <david@redhat.com <mailto:david@redhat.com>
>         <mailto:david@redhat.com <mailto:david@redhat.com>>> wrote:
> 
>              Let's clean it up a bit, removing the unnecessary usage of
>         r_next() by
>              next_resource(), and use next_range_resource() in case we
>         are not
>              interested in a certain subtree.
> 
>              Signed-off-by: David Hildenbrand <david@redhat.com
>         <mailto:david@redhat.com>
>              <mailto:david@redhat.com <mailto:david@redhat.com>>>
>              ---
>                kernel/resource.c | 19 +++++++++++--------
>                1 file changed, 11 insertions(+), 8 deletions(-)
> 
>              diff --git a/kernel/resource.c b/kernel/resource.c
>              index 2938cf520ca3..ea853a075a83 100644
>              --- a/kernel/resource.c
>              +++ b/kernel/resource.c
>              @@ -1754,9 +1754,8 @@ static int strict_iomem_checks;
>                 */
>                bool iomem_is_exclusive(u64 addr)
>                {
>              -       struct resource *p = &iomem_resource;
>              +       struct resource *p;
>                       bool err = false;
>              -       loff_t l;
>                       int size = PAGE_SIZE;
> 
>                       if (!strict_iomem_checks)
>              @@ -1765,27 +1764,31 @@ bool iomem_is_exclusive(u64 addr)
>                       addr = addr & PAGE_MASK;
> 
>                       read_lock(&resource_lock);
>              -       for (p = p->child; p ; p = r_next(NULL, p, &l)) {
>              +       for (p = iomem_resource.child; p ;) {
> 
> 
>     Hi Andy,
> 
> 
>         I consider the ordinal part of p initialization is slightly
>         better and done outside of read lock.
> 
>         Something like
>         p= &iomem_res...;
>         read lock
>         for (p = p->child; ...) {
> 
> 
>     Why should we care about doing that outside of the lock? That smells
>     like a micro-optimization the compiler will most probably overwrite
>     either way as the address of iomem_resource is just constant?
> 
>     Also, for me it's much more readable and compact if we perform a
>     single initialization instead of two separate ones in this case.
> 
>     We're using the pattern I use in, find_next_iomem_res() and
>     __region_intersects(), while we use the old pattern in
>     iomem_map_sanity_check(), where we also use the same unnecessary
>     r_next() call.
> 
>     I might just cleanup iomem_map_sanity_check() in a similar way.
> 
> 
> 
> Yes, it’s like micro optimization. If you want your way I suggest then 
> to add a macro
> 
> #define for_each_iomem_resource_child() \
>   for (iomem_resource...)

I think the only thing that really makes sense would be something like this on top (not compiled yet):


diff --git a/kernel/resource.c b/kernel/resource.c
index ea853a075a83..35aaa72df0ce 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -80,6 +80,11 @@ static struct resource *next_resource_skip_children(struct resource *p)
         return p->sibling;
  }
  
+#define for_each_resource(_root, _p, _skip_children) \
+       for ((_p) = (_root)->child; (_p); \
+            (_p) = (_skip_children) ? next_resource_skip_children(_p) : \
+                                      next_resource(_p))
+
  static void *r_next(struct seq_file *m, void *v, loff_t *pos)
  {
         struct resource *p = v;
@@ -1714,16 +1719,16 @@ int iomem_map_sanity_check(resource_size_t addr, unsigned long size)
  bool iomem_range_contains_excluded(u64 addr, u64 size)
  {
         const unsigned int flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_EXCLUSIVE;
-       bool excluded = false;
+       bool skip_children, excluded = false;
         struct resource *p;
  
         read_lock(&resource_lock);
-       for (p = iomem_resource.child; p ;) {
+       for_each_resource(&iomem_resource, p, skip_children) {
                 if (p->start >= addr + size)
                         break;
                 if (p->end < addr) {
                         /* No need to consider children */
-                       p = next_resource_skip_children(p);
+                       skip_children = true;
                         continue;
                 }
                 /*
@@ -1735,7 +1740,7 @@ bool iomem_range_contains_excluded(u64 addr, u64 size)
                         excluded = true;
                         break;
                 }
-               p = next_resource(p);
+               skip_children = false;
         }
         read_unlock(&resource_lock);
  
@@ -1755,7 +1760,7 @@ static int strict_iomem_checks;
  bool iomem_is_exclusive(u64 addr)
  {
         struct resource *p;
-       bool err = false;
+       bool skip_children, err = false;
         int size = PAGE_SIZE;
  
         if (!strict_iomem_checks)
@@ -1764,7 +1769,7 @@ bool iomem_is_exclusive(u64 addr)
         addr = addr & PAGE_MASK;
  
         read_lock(&resource_lock);
-       for (p = iomem_resource.child; p ;) {
+       for_each_resource(&iomem_resource, p, skip_children) {
                 /*
                  * We can probably skip the resources without
                  * IORESOURCE_IO attribute?
@@ -1773,7 +1778,7 @@ bool iomem_is_exclusive(u64 addr)
                         break;
                 if (p->end < addr) {
                         /* No need to consider children */
-                       p = next_resource_skip_children(p);
+                       skip_children = true;
                         continue;
                 }
  
@@ -1788,7 +1793,7 @@ bool iomem_is_exclusive(u64 addr)
                         err = true;
                         break;
                 }
-               p = next_resource(p);
+               skip_children = false;
         }
         read_unlock(&resource_lock);
  


Thoughts?


-- 
Thanks,

David / dhildenb

Re: [PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive()

[Andy Shevchenko]

On Thu, Aug 12, 2021 at 09:34:00AM +0200, David Hildenbrand wrote:
> On 12.08.21 09:14, Andy Shevchenko wrote:
> > On Thursday, August 12, 2021, David Hildenbrand <david@redhat.com
> > <mailto:david@redhat.com>> wrote:
> >     On 11.08.21 22:47, Andy Shevchenko wrote:
> >         On Wednesday, August 11, 2021, David Hildenbrand
> >         <david@redhat.com <mailto:david@redhat.com>
> >         <mailto:david@redhat.com <mailto:david@redhat.com>>> wrote:

> > Yes, it’s like micro optimization. If you want your way I suggest then
> > to add a macro
> > 
> > #define for_each_iomem_resource_child() \
> >   for (iomem_resource...)
> 
> I think the only thing that really makes sense would be something like this on top (not compiled yet):

Makes sense to me, thanks, go for it!

-- 
With Best Regards,
Andy Shevchenko