diff mbox series

[3/3] /dev/mem: Do not map unaccepted memory

Message ID 20230906073902.4229-4-adrian.hunter@intel.com
State New
Headers show
Series Do not map unaccepted memory | expand

Commit Message

Adrian Hunter Sept. 6, 2023, 7:39 a.m. UTC
Support for unaccepted memory was added recently, refer commit
dcdfdd40fa82 ("mm: Add support for unaccepted memory"), whereby
a virtual machine may need to accept memory before it can be used.

Do not map unaccepted memory because it can cause the guest to fail.

For /dev/mem, this means a read of unaccepted memory will return zeros,
a write to unaccepted memory will be ignored, but an mmap of unaccepted
memory will return an error.

Fixes: dcdfdd40fa82 ("mm: Add support for unaccepted memory")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/char/mem.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Kirill A. Shutemov Sept. 7, 2023, 10:06 a.m. UTC | #1
On Wed, Sep 06, 2023 at 10:39:02AM +0300, Adrian Hunter wrote:
> Support for unaccepted memory was added recently, refer commit
> dcdfdd40fa82 ("mm: Add support for unaccepted memory"), whereby
> a virtual machine may need to accept memory before it can be used.
> 
> Do not map unaccepted memory because it can cause the guest to fail.
> 
> For /dev/mem, this means a read of unaccepted memory will return zeros,
> a write to unaccepted memory will be ignored, but an mmap of unaccepted
> memory will return an error.

I am unsure who currently uses /dev/mem. The change to the mmap path has the
potential to cause issues as it is a new behavior. However, it appears to
be a common practice as we also fail to mmap if PAT is set on a page in
the rang. I suppose it is acceptable.

Another option is to accept the memory on mmap, but it seems excessive at
this point.
Dave Hansen Sept. 7, 2023, 2:15 p.m. UTC | #2
On 9/6/23 00:39, Adrian Hunter wrote:
> Support for unaccepted memory was added recently, refer commit
> dcdfdd40fa82 ("mm: Add support for unaccepted memory"), whereby
> a virtual machine may need to accept memory before it can be used.
> 
> Do not map unaccepted memory because it can cause the guest to fail.

Doesn't /dev/mem already provide a billion ways for someone to shoot
themselves in the foot?  TDX seems to have added the 1,000,000,001st.
Is this really worth patching?
Kirill A. Shutemov Sept. 7, 2023, 2:25 p.m. UTC | #3
On Thu, Sep 07, 2023 at 07:15:21AM -0700, Dave Hansen wrote:
> On 9/6/23 00:39, Adrian Hunter wrote:
> > Support for unaccepted memory was added recently, refer commit
> > dcdfdd40fa82 ("mm: Add support for unaccepted memory"), whereby
> > a virtual machine may need to accept memory before it can be used.
> > 
> > Do not map unaccepted memory because it can cause the guest to fail.
> 
> Doesn't /dev/mem already provide a billion ways for someone to shoot
> themselves in the foot?  TDX seems to have added the 1,000,000,001st.
> Is this really worth patching?

Is it better to let TD die silently? I don't think so.
Dave Hansen Sept. 7, 2023, 2:46 p.m. UTC | #4
On 9/7/23 07:25, Kirill A. Shutemov wrote:
> On Thu, Sep 07, 2023 at 07:15:21AM -0700, Dave Hansen wrote:
>> On 9/6/23 00:39, Adrian Hunter wrote:
>>> Support for unaccepted memory was added recently, refer commit
>>> dcdfdd40fa82 ("mm: Add support for unaccepted memory"), whereby
>>> a virtual machine may need to accept memory before it can be used.
>>>
>>> Do not map unaccepted memory because it can cause the guest to fail.
>> Doesn't /dev/mem already provide a billion ways for someone to shoot
>> themselves in the foot?  TDX seems to have added the 1,000,000,001st.
>> Is this really worth patching?
> Is it better to let TD die silently? I don't think so.

First, let's take a look at all of the distro kernels that folks will
run under TDX.  Do they have STRICT_DEVMEM set?

> config STRICT_DEVMEM
...
>           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
>           data regions.  This is sufficient for dosemu and X and all common
>           users of /dev/mem.

Can a line of code in this patch even run in the face of IO_STRICT_DEVMEM=y?

I think basically everybody sets that option and has for over a decade.
If there are any distros out there _not_ setting this, we should
probably have a chat with them to find out more.

I suspect any practical use of this patch is limited to folks who:

1. Compile sensible security-related options out of their kernel
2. Go and reads random pages with /dev/mem in their "secure" VM

They get to hold the pieces, and they can and will get a notification
from their VMM that the VM did something nasty.

BTW, Ubuntu at least also sets HARDENED_USERCOPY which will *also*
enable STRICT_DEVMEM.  So someone would have to _really_ go to some
trouble to shoot themselves in the foot here.  If they're _that_
determined, it would be a shame to thwart their efforts with this patch.
Dave Hansen Sept. 7, 2023, 3:04 p.m. UTC | #5
On 9/7/23 07:46, Dave Hansen wrote:
> Can a line of code in this patch even run in the face of IO_STRICT_DEVMEM=y?

Gah, I meant plain old STRICT_DEVMEM=y.
David Hildenbrand Sept. 11, 2023, 8:09 a.m. UTC | #6
On 07.09.23 16:46, Dave Hansen wrote:
> On 9/7/23 07:25, Kirill A. Shutemov wrote:
>> On Thu, Sep 07, 2023 at 07:15:21AM -0700, Dave Hansen wrote:
>>> On 9/6/23 00:39, Adrian Hunter wrote:
>>>> Support for unaccepted memory was added recently, refer commit
>>>> dcdfdd40fa82 ("mm: Add support for unaccepted memory"), whereby
>>>> a virtual machine may need to accept memory before it can be used.
>>>>
>>>> Do not map unaccepted memory because it can cause the guest to fail.
>>> Doesn't /dev/mem already provide a billion ways for someone to shoot
>>> themselves in the foot?  TDX seems to have added the 1,000,000,001st.
>>> Is this really worth patching?
>> Is it better to let TD die silently? I don't think so.
> 
> First, let's take a look at all of the distro kernels that folks will
> run under TDX.  Do they have STRICT_DEVMEM set?

For virtio-mem, we do

	config VIRTIO_MEM
		...
		depends on EXCLUSIVE_SYSTEM_RAM

Which in turn:

	config EXCLUSIVE_SYSTEM_RAM
		...
		depends on !DEVMEM || STRICT_DEVMEM


Not supported on all archs, but at least on RHEL9 on x86_64 and aarch64.

So, making unaccepted memory similarly depend on "!DEVMEM || 
STRICT_DEVMEM" does not sound too far off ...
Dave Hansen Sept. 11, 2023, 2:32 p.m. UTC | #7
On 9/11/23 01:09, David Hildenbrand wrote:
> So, making unaccepted memory similarly depend on "!DEVMEM ||
> STRICT_DEVMEM" does not sound too far off ...

Yeah, considering all of the invasive work folks want to do to "harden"
the kernel for TDX, doing that ^ is just about the best
bang-for-your-buck "hardening" that you can get.
diff mbox series

Patch

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 1052b0f2d4cf..1a7c5728783c 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -147,7 +147,8 @@  static ssize_t read_mem(struct file *file, char __user *buf,
 			goto failed;
 
 		err = -EFAULT;
-		if (allowed == 2) {
+		if (allowed == 2 ||
+		    range_contains_unaccepted_memory(p, p + sz)) {
 			/* Show zeros for restricted memory. */
 			remaining = clear_user(buf, sz);
 		} else {
@@ -226,7 +227,8 @@  static ssize_t write_mem(struct file *file, const char __user *buf,
 			return -EPERM;
 
 		/* Skip actual writing when a page is marked as restricted. */
-		if (allowed == 1) {
+		if (allowed == 1 &&
+		    !range_contains_unaccepted_memory(p, p + sz)) {
 			/*
 			 * On ia64 if a page has been mapped somewhere as
 			 * uncached, then it must also be accessed uncached
@@ -378,6 +380,9 @@  static int mmap_mem(struct file *file, struct vm_area_struct *vma)
 						&vma->vm_page_prot))
 		return -EINVAL;
 
+	if (range_contains_unaccepted_memory(offset, offset + size))
+		return -EINVAL;
+
 	vma->vm_page_prot = phys_mem_access_prot(file, vma->vm_pgoff,
 						 size,
 						 vma->vm_page_prot);