diff mbox series

[v4,3/3] virtio-mem: disallow mapping virtio-mem memory via /dev/mem

Message ID 20210902160919.25683-4-david@redhat.com (mailing list archive)
State New
Headers show
Series virtio-mem: disallow mapping virtio-mem memory via /dev/mem | expand

Commit Message

David Hildenbrand Sept. 2, 2021, 4:09 p.m. UTC 
We don't want user space to be able to map virtio-mem device memory
directly (e.g., via /dev/mem) in order to have guarantees that in a sane
setup we'll never accidentially access unplugged memory within the
device-managed region of a virtio-mem device, just as required by the
virtio-spec.

As soon as the virtio-mem driver is loaded, the device region is visible
in /proc/iomem via the parent device region. From that point on user space
is aware of the device region and we want to disallow mapping anything
inside that region (where we will dynamically (un)plug memory) until
the driver has been unloaded cleanly and e.g., another driver might take
over.

By creating our parent IORESOURCE_SYSTEM_RAM resource with
IORESOURCE_EXCLUSIVE, we will disallow any /dev/mem access to our
device region until the driver was unloaded cleanly and removed the
parent region. This will work even though only some memory blocks are
actually currently added to Linux and appear as busy in the resource tree.

So access to the region from user space is only possible
a) if we don't load the virtio-mem driver.
b) after unloading the virtio-mem driver cleanly.

Don't build virtio-mem if access to /dev/mem cannot be restricticted --
if we have CONFIG_DEVMEM=y but CONFIG_STRICT_DEVMEM is not set.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 drivers/virtio/Kconfig      | 1 +
 drivers/virtio/virtio_mem.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

Comments

Michael S. Tsirkin Sept. 3, 2021, 7:02 a.m. UTC | #1 
On Thu, Sep 02, 2021 at 06:09:19PM +0200, David Hildenbrand wrote:
> We don't want user space to be able to map virtio-mem device memory
> directly (e.g., via /dev/mem) in order to have guarantees that in a sane
> setup we'll never accidentially access unplugged memory within the
> device-managed region of a virtio-mem device, just as required by the
> virtio-spec.
> 
> As soon as the virtio-mem driver is loaded, the device region is visible
> in /proc/iomem via the parent device region. From that point on user space
> is aware of the device region and we want to disallow mapping anything
> inside that region (where we will dynamically (un)plug memory) until
> the driver has been unloaded cleanly and e.g., another driver might take
> over.
> 
> By creating our parent IORESOURCE_SYSTEM_RAM resource with
> IORESOURCE_EXCLUSIVE, we will disallow any /dev/mem access to our
> device region until the driver was unloaded cleanly and removed the
> parent region. This will work even though only some memory blocks are
> actually currently added to Linux and appear as busy in the resource tree.
> 
> So access to the region from user space is only possible
> a) if we don't load the virtio-mem driver.
> b) after unloading the virtio-mem driver cleanly.
> 
> Don't build virtio-mem if access to /dev/mem cannot be restricticted --
> if we have CONFIG_DEVMEM=y but CONFIG_STRICT_DEVMEM is not set.
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>


> ---
>  drivers/virtio/Kconfig      | 1 +
>  drivers/virtio/virtio_mem.c | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index ce1b3f6ec325..ff80cd03f1d1 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -101,6 +101,7 @@ config VIRTIO_MEM
>  	depends on MEMORY_HOTPLUG_SPARSE
>  	depends on MEMORY_HOTREMOVE
>  	depends on CONTIG_ALLOC
> +	depends on !DEVMEM || STRICT_DEVMEM
>  	help
>  	 This driver provides access to virtio-mem paravirtualized memory
>  	 devices, allowing to hotplug and hotunplug memory.

It would be nicer if there was a symbol in the MEMORY_ namespace
we can depend on exported by mm and depending on !DEVMEM ||
STRICT_DEVMEM.

E.g.

config MEMORY_EXCLUSIVE
        def_bool y
        depends on !DEVMEM || STRICT_DEVMEM

and then in virtio
	depends on MEMORY_EXCLUSIVE


the virtio change itself is ok though:

Acked-by: Michael S. Tsirkin <mst@redhat.com>

for merging through -mm.

> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> index b91bc810a87e..c2d93492cf0f 100644
> --- a/drivers/virtio/virtio_mem.c
> +++ b/drivers/virtio/virtio_mem.c
> @@ -2523,8 +2523,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
David Hildenbrand Sept. 14, 2021, 9:45 a.m. UTC | #2 
On 03.09.21 09:02, Michael S. Tsirkin wrote:
> On Thu, Sep 02, 2021 at 06:09:19PM +0200, David Hildenbrand wrote:
>> We don't want user space to be able to map virtio-mem device memory
>> directly (e.g., via /dev/mem) in order to have guarantees that in a sane
>> setup we'll never accidentially access unplugged memory within the
>> device-managed region of a virtio-mem device, just as required by the
>> virtio-spec.
>>
>> As soon as the virtio-mem driver is loaded, the device region is visible
>> in /proc/iomem via the parent device region. From that point on user space
>> is aware of the device region and we want to disallow mapping anything
>> inside that region (where we will dynamically (un)plug memory) until
>> the driver has been unloaded cleanly and e.g., another driver might take
>> over.
>>
>> By creating our parent IORESOURCE_SYSTEM_RAM resource with
>> IORESOURCE_EXCLUSIVE, we will disallow any /dev/mem access to our
>> device region until the driver was unloaded cleanly and removed the
>> parent region. This will work even though only some memory blocks are
>> actually currently added to Linux and appear as busy in the resource tree.
>>
>> So access to the region from user space is only possible
>> a) if we don't load the virtio-mem driver.
>> b) after unloading the virtio-mem driver cleanly.
>>
>> Don't build virtio-mem if access to /dev/mem cannot be restricticted --
>> if we have CONFIG_DEVMEM=y but CONFIG_STRICT_DEVMEM is not set.
>>
>> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> 
>> ---
>>   drivers/virtio/Kconfig      | 1 +
>>   drivers/virtio/virtio_mem.c | 4 +++-
>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
>> index ce1b3f6ec325..ff80cd03f1d1 100644
>> --- a/drivers/virtio/Kconfig
>> +++ b/drivers/virtio/Kconfig
>> @@ -101,6 +101,7 @@ config VIRTIO_MEM
>>   	depends on MEMORY_HOTPLUG_SPARSE
>>   	depends on MEMORY_HOTREMOVE
>>   	depends on CONTIG_ALLOC
>> +	depends on !DEVMEM || STRICT_DEVMEM
>>   	help
>>   	 This driver provides access to virtio-mem paravirtualized memory
>>   	 devices, allowing to hotplug and hotunplug memory.
> 
> It would be nicer if there was a symbol in the MEMORY_ namespace
> we can depend on exported by mm and depending on !DEVMEM ||
> STRICT_DEVMEM.
> 
> E.g.
> 
> config MEMORY_EXCLUSIVE
>          def_bool y
>          depends on !DEVMEM || STRICT_DEVMEM
> 
> and then in virtio
> 	depends on MEMORY_EXCLUSIVE
> 
> 

Yes, but I'm not able to come up with an expressive name. 
MEMORY_EXCLUSIVE can be highly misleading ...


> the virtio change itself is ok though:
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Thanks!
Michael S. Tsirkin Sept. 14, 2021, 11:52 a.m. UTC | #3 
On Tue, Sep 14, 2021 at 11:45:25AM +0200, David Hildenbrand wrote:
> On 03.09.21 09:02, Michael S. Tsirkin wrote:
> > On Thu, Sep 02, 2021 at 06:09:19PM +0200, David Hildenbrand wrote:
> > > We don't want user space to be able to map virtio-mem device memory
> > > directly (e.g., via /dev/mem) in order to have guarantees that in a sane
> > > setup we'll never accidentially access unplugged memory within the
> > > device-managed region of a virtio-mem device, just as required by the
> > > virtio-spec.
> > > 
> > > As soon as the virtio-mem driver is loaded, the device region is visible
> > > in /proc/iomem via the parent device region. From that point on user space
> > > is aware of the device region and we want to disallow mapping anything
> > > inside that region (where we will dynamically (un)plug memory) until
> > > the driver has been unloaded cleanly and e.g., another driver might take
> > > over.
> > > 
> > > By creating our parent IORESOURCE_SYSTEM_RAM resource with
> > > IORESOURCE_EXCLUSIVE, we will disallow any /dev/mem access to our
> > > device region until the driver was unloaded cleanly and removed the
> > > parent region. This will work even though only some memory blocks are
> > > actually currently added to Linux and appear as busy in the resource tree.
> > > 
> > > So access to the region from user space is only possible
> > > a) if we don't load the virtio-mem driver.
> > > b) after unloading the virtio-mem driver cleanly.
> > > 
> > > Don't build virtio-mem if access to /dev/mem cannot be restricticted --
> > > if we have CONFIG_DEVMEM=y but CONFIG_STRICT_DEVMEM is not set.
> > > 
> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: David Hildenbrand <david@redhat.com>
> > 
> > 
> > > ---
> > >   drivers/virtio/Kconfig      | 1 +
> > >   drivers/virtio/virtio_mem.c | 4 +++-
> > >   2 files changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > > index ce1b3f6ec325..ff80cd03f1d1 100644
> > > --- a/drivers/virtio/Kconfig
> > > +++ b/drivers/virtio/Kconfig
> > > @@ -101,6 +101,7 @@ config VIRTIO_MEM
> > >   	depends on MEMORY_HOTPLUG_SPARSE
> > >   	depends on MEMORY_HOTREMOVE
> > >   	depends on CONTIG_ALLOC
> > > +	depends on !DEVMEM || STRICT_DEVMEM
> > >   	help
> > >   	 This driver provides access to virtio-mem paravirtualized memory
> > >   	 devices, allowing to hotplug and hotunplug memory.
> > 
> > It would be nicer if there was a symbol in the MEMORY_ namespace
> > we can depend on exported by mm and depending on !DEVMEM ||
> > STRICT_DEVMEM.
> > 
> > E.g.
> > 
> > config MEMORY_EXCLUSIVE
> >          def_bool y
> >          depends on !DEVMEM || STRICT_DEVMEM
> > 
> > and then in virtio
> > 	depends on MEMORY_EXCLUSIVE
> > 
> > 
> 
> Yes, but I'm not able to come up with an expressive name. MEMORY_EXCLUSIVE
> can be highly misleading ...


I mean ... it enables IORESOURCE_EXCLUSIVE ... but ok ...
MEMORY_EXCLUSIVE_KERNEL_MAP ?

> 
> > the virtio change itself is ok though:
> > 
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Thanks!
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb
David Hildenbrand Sept. 14, 2021, 11:57 a.m. UTC | #4 
On 14.09.21 13:52, Michael S. Tsirkin wrote:
> On Tue, Sep 14, 2021 at 11:45:25AM +0200, David Hildenbrand wrote:
>> On 03.09.21 09:02, Michael S. Tsirkin wrote:
>>> On Thu, Sep 02, 2021 at 06:09:19PM +0200, David Hildenbrand wrote:
>>>> We don't want user space to be able to map virtio-mem device memory
>>>> directly (e.g., via /dev/mem) in order to have guarantees that in a sane
>>>> setup we'll never accidentially access unplugged memory within the
>>>> device-managed region of a virtio-mem device, just as required by the
>>>> virtio-spec.
>>>>
>>>> As soon as the virtio-mem driver is loaded, the device region is visible
>>>> in /proc/iomem via the parent device region. From that point on user space
>>>> is aware of the device region and we want to disallow mapping anything
>>>> inside that region (where we will dynamically (un)plug memory) until
>>>> the driver has been unloaded cleanly and e.g., another driver might take
>>>> over.
>>>>
>>>> By creating our parent IORESOURCE_SYSTEM_RAM resource with
>>>> IORESOURCE_EXCLUSIVE, we will disallow any /dev/mem access to our
>>>> device region until the driver was unloaded cleanly and removed the
>>>> parent region. This will work even though only some memory blocks are
>>>> actually currently added to Linux and appear as busy in the resource tree.
>>>>
>>>> So access to the region from user space is only possible
>>>> a) if we don't load the virtio-mem driver.
>>>> b) after unloading the virtio-mem driver cleanly.
>>>>
>>>> Don't build virtio-mem if access to /dev/mem cannot be restricticted --
>>>> if we have CONFIG_DEVMEM=y but CONFIG_STRICT_DEVMEM is not set.
>>>>
>>>> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>
>>>
>>>> ---
>>>>    drivers/virtio/Kconfig      | 1 +
>>>>    drivers/virtio/virtio_mem.c | 4 +++-
>>>>    2 files changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
>>>> index ce1b3f6ec325..ff80cd03f1d1 100644
>>>> --- a/drivers/virtio/Kconfig
>>>> +++ b/drivers/virtio/Kconfig
>>>> @@ -101,6 +101,7 @@ config VIRTIO_MEM
>>>>    	depends on MEMORY_HOTPLUG_SPARSE
>>>>    	depends on MEMORY_HOTREMOVE
>>>>    	depends on CONTIG_ALLOC
>>>> +	depends on !DEVMEM || STRICT_DEVMEM
>>>>    	help
>>>>    	 This driver provides access to virtio-mem paravirtualized memory
>>>>    	 devices, allowing to hotplug and hotunplug memory.
>>>
>>> It would be nicer if there was a symbol in the MEMORY_ namespace
>>> we can depend on exported by mm and depending on !DEVMEM ||
>>> STRICT_DEVMEM.
>>>
>>> E.g.
>>>
>>> config MEMORY_EXCLUSIVE
>>>           def_bool y
>>>           depends on !DEVMEM || STRICT_DEVMEM
>>>
>>> and then in virtio
>>> 	depends on MEMORY_EXCLUSIVE
>>>
>>>
>>
>> Yes, but I'm not able to come up with an expressive name. MEMORY_EXCLUSIVE
>> can be highly misleading ...
> 
> 
> I mean ... it enables IORESOURCE_EXCLUSIVE ... but ok ...
> MEMORY_EXCLUSIVE_KERNEL_MAP ?

It enables IORESOURCE_EXCLUSIVE for IORESOURCE_SYSTEM_RAM ... and 
notably not for IORESOURCE_MEM.

MEMORY_EXCLUSIVE_SYSTEM_RAM ?

> 
>>
>>> the virtio change itself is ok though:
>>>
>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> Thanks!
>>
>>
>> -- 
>> Thanks,
>>
>> David / dhildenb
>
Michael S. Tsirkin Sept. 14, 2021, 12:04 p.m. UTC | #5 
On Tue, Sep 14, 2021 at 01:57:25PM +0200, David Hildenbrand wrote:
> On 14.09.21 13:52, Michael S. Tsirkin wrote:
> > On Tue, Sep 14, 2021 at 11:45:25AM +0200, David Hildenbrand wrote:
> > > On 03.09.21 09:02, Michael S. Tsirkin wrote:
> > > > On Thu, Sep 02, 2021 at 06:09:19PM +0200, David Hildenbrand wrote:
> > > > > We don't want user space to be able to map virtio-mem device memory
> > > > > directly (e.g., via /dev/mem) in order to have guarantees that in a sane
> > > > > setup we'll never accidentially access unplugged memory within the
> > > > > device-managed region of a virtio-mem device, just as required by the
> > > > > virtio-spec.
> > > > > 
> > > > > As soon as the virtio-mem driver is loaded, the device region is visible
> > > > > in /proc/iomem via the parent device region. From that point on user space
> > > > > is aware of the device region and we want to disallow mapping anything
> > > > > inside that region (where we will dynamically (un)plug memory) until
> > > > > the driver has been unloaded cleanly and e.g., another driver might take
> > > > > over.
> > > > > 
> > > > > By creating our parent IORESOURCE_SYSTEM_RAM resource with
> > > > > IORESOURCE_EXCLUSIVE, we will disallow any /dev/mem access to our
> > > > > device region until the driver was unloaded cleanly and removed the
> > > > > parent region. This will work even though only some memory blocks are
> > > > > actually currently added to Linux and appear as busy in the resource tree.
> > > > > 
> > > > > So access to the region from user space is only possible
> > > > > a) if we don't load the virtio-mem driver.
> > > > > b) after unloading the virtio-mem driver cleanly.
> > > > > 
> > > > > Don't build virtio-mem if access to /dev/mem cannot be restricticted --
> > > > > if we have CONFIG_DEVMEM=y but CONFIG_STRICT_DEVMEM is not set.
> > > > > 
> > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > > Signed-off-by: David Hildenbrand <david@redhat.com>
> > > > 
> > > > 
> > > > > ---
> > > > >    drivers/virtio/Kconfig      | 1 +
> > > > >    drivers/virtio/virtio_mem.c | 4 +++-
> > > > >    2 files changed, 4 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > > > > index ce1b3f6ec325..ff80cd03f1d1 100644
> > > > > --- a/drivers/virtio/Kconfig
> > > > > +++ b/drivers/virtio/Kconfig
> > > > > @@ -101,6 +101,7 @@ config VIRTIO_MEM
> > > > >    	depends on MEMORY_HOTPLUG_SPARSE
> > > > >    	depends on MEMORY_HOTREMOVE
> > > > >    	depends on CONTIG_ALLOC
> > > > > +	depends on !DEVMEM || STRICT_DEVMEM
> > > > >    	help
> > > > >    	 This driver provides access to virtio-mem paravirtualized memory
> > > > >    	 devices, allowing to hotplug and hotunplug memory.
> > > > 
> > > > It would be nicer if there was a symbol in the MEMORY_ namespace
> > > > we can depend on exported by mm and depending on !DEVMEM ||
> > > > STRICT_DEVMEM.
> > > > 
> > > > E.g.
> > > > 
> > > > config MEMORY_EXCLUSIVE
> > > >           def_bool y
> > > >           depends on !DEVMEM || STRICT_DEVMEM
> > > > 
> > > > and then in virtio
> > > > 	depends on MEMORY_EXCLUSIVE
> > > > 
> > > > 
> > > 
> > > Yes, but I'm not able to come up with an expressive name. MEMORY_EXCLUSIVE
> > > can be highly misleading ...
> > 
> > 
> > I mean ... it enables IORESOURCE_EXCLUSIVE ... but ok ...
> > MEMORY_EXCLUSIVE_KERNEL_MAP ?
> 
> It enables IORESOURCE_EXCLUSIVE for IORESOURCE_SYSTEM_RAM ... and notably
> not for IORESOURCE_MEM.
> 
> MEMORY_EXCLUSIVE_SYSTEM_RAM ?


OK.


> > 
> > > 
> > > > the virtio change itself is ok though:
> > > > 
> > > > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > 
> > > Thanks!
> > > 
> > > 
> > > -- 
> > > Thanks,
> > > 
> > > David / dhildenb
> > 
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb
diff mbox series

Patch

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index ce1b3f6ec325..ff80cd03f1d1 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -101,6 +101,7 @@  config VIRTIO_MEM
 	depends on MEMORY_HOTPLUG_SPARSE
 	depends on MEMORY_HOTREMOVE
 	depends on CONTIG_ALLOC
+	depends on !DEVMEM || STRICT_DEVMEM
 	help
 	 This driver provides access to virtio-mem paravirtualized memory
 	 devices, allowing to hotplug and hotunplug memory.
diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index b91bc810a87e..c2d93492cf0f 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -2523,8 +2523,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");