Do not map unaccepted memory

PATCH In review Adrian Hunter 4 participants 24 messages Sep 6, 2023 – Sep 11, 2023

Subsystems (9)

Memory management VFS Char and misc drivers Proc filesystem +5 more subsystems

lore

Share & tools

lore

Adrian Hunter

author

2023-09-06 07:39 UTC #1 lore raw

PATCH 1/3 proc/vmcore: Do not map unaccepted memory

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 /proc/vmcore, which is read-only, this means a read or mmap of unaccepted memory will return zeros.

Fixes dcdfdd40fa82 ("mm: Add support for unaccepted memory")

Signed-off-by Adrian Hunter <[email protected]>

---
 fs/proc/vmcore.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

fs/proc/vmcore.c +12 -3

index 1fb213f379a5..a28da2033ce8 100644
@@ -118,6 +118,13 @@ static bool pfn_is_ram(unsigned long pfn)
 	return ret;
 }
 
+static bool pfn_is_unaccepted_memory(unsigned long pfn)
+{
+	phys_addr_t paddr = pfn << PAGE_SHIFT;
+
+	return range_contains_unaccepted_memory(paddr, paddr + PAGE_SIZE);
+}
+
 static int open_vmcore(struct inode *inode, struct file *file)
 {
 	spin_lock(&vmcore_cb_lock);

@@ -150,7 +157,7 @@ ssize_t read_from_oldmem(struct iov_iter *iter, size_t count,
 			nr_bytes = count;
 
 		/* If pfn is not ram, return zeros for sparse dump files */
-		if (!pfn_is_ram(pfn)) {
+		if (!pfn_is_ram(pfn) || pfn_is_unaccepted_memory(pfn)) {
 			tmp = iov_iter_zero(nr_bytes, iter);
 		} else {
 			if (encrypted)

@@ -511,7 +518,7 @@ static int remap_oldmem_pfn_checked(struct vm_area_struct *vma,
 	pos_end = pfn + (size >> PAGE_SHIFT);
 
 	for (pos = pos_start; pos < pos_end; ++pos) {
-		if (!pfn_is_ram(pos)) {
+		if (!pfn_is_ram(pos) || pfn_is_unaccepted_memory(pos)) {
 			/*
 			 * We hit a page which is not ram. Remap the continuous
 			 * region between pos_start and pos-1 and replace

@@ -552,6 +559,7 @@ static int vmcore_remap_oldmem_pfn(struct vm_area_struct *vma,
 			    unsigned long from, unsigned long pfn,
 			    unsigned long size, pgprot_t prot)
 {
+	phys_addr_t paddr = pfn << PAGE_SHIFT;
 	int ret, idx;
 
 	/*

@@ -559,7 +567,8 @@ static int vmcore_remap_oldmem_pfn(struct vm_area_struct *vma,
 	 * pages without a reason.
 	 */
 	idx = srcu_read_lock(&vmcore_cb_srcu);
-	if (!list_empty(&vmcore_cb_list))
+	if (!list_empty(&vmcore_cb_list) ||
+	    range_contains_unaccepted_memory(paddr, paddr + size))
 		ret = remap_oldmem_pfn_checked(vma, from, pfn, size, prot);
 	else
 		ret = remap_oldmem_pfn_range(vma, from, pfn, size, prot);

Signature · 1 line

2.34.1

Dave Hansen

2023-09-07 15:39 UTC #2 lore raw

Re: [PATCH 1/3] proc/vmcore: Do not map unaccepted memory

fs/proc/vmcore.c static int vmcore_remap_oldmem_pfn(struct vm_area_struct *vma, · line 574

Quoted 10 lines · Adrian Hunter

fs/proc/vmcore.c +2 -1

@@ -559,7 +567,8 @@ static int vmcore_remap_oldmem_pfn(struct vm_area_struct *vma,
 	 * pages without a reason.
 	 */
 	idx = srcu_read_lock(&vmcore_cb_srcu);
-	if (!list_empty(&vmcore_cb_list))
+	if (!list_empty(&vmcore_cb_list) ||
+	    range_contains_unaccepted_memory(paddr, paddr + size))
 		ret = remap_oldmem_pfn_checked(vma, from, pfn, size, prot);
 	else
 		ret = remap_oldmem_pfn_range(vma, from, pfn, size, prot);

The whole callback mechanism which fs/proc/vmcore.c::pfn_is_ram() implements seems to be in place to ensure that there aren't a billion different "ram" checks in here.

Is there a reason you can't register_vmcore_cb() a callback to check for unaccepted memory?

Adrian Hunter

author

2023-09-07 15:47 UTC #3 lore raw

Re: [PATCH 1/3] proc/vmcore: Do not map unaccepted memory

Quoted 16 lines · Dave Hansen

fs/proc/vmcore.c static int vmcore_remap_oldmem_pfn(struct vm_area_struct *vma, · line 574
Quoted 10 lines · Adrian Hunter
fs/proc/vmcore.c +2 -1
@@ -559,7 +567,8 @@ static int vmcore_remap_oldmem_pfn(struct vm_area_struct *vma,
 	 * pages without a reason.
 	 */
 	idx = srcu_read_lock(&vmcore_cb_srcu);
-	if (!list_empty(&vmcore_cb_list))
+	if (!list_empty(&vmcore_cb_list) ||
+	    range_contains_unaccepted_memory(paddr, paddr + size))
 		ret = remap_oldmem_pfn_checked(vma, from, pfn, size, prot);
 	else
 		ret = remap_oldmem_pfn_range(vma, from, pfn, size, prot);
The whole callback mechanism which fs/proc/vmcore.c::pfn_is_ram() implements seems to be in place to ensure that there aren't a billion different "ram" checks in here.

Quoted 2 lines

Is there a reason you can't register_vmcore_cb() a callback to check for unaccepted memory?

Someone asked for the change to be in arch-independent code... ;-)

Dave Hansen

2023-09-07 15:51 UTC #4 lore raw

Re: [PATCH 1/3] proc/vmcore: Do not map unaccepted memory

Quoted 16 lines · Adrian Hunter

Quoted 15 lines · Dave Hansen
fs/proc/vmcore.c static int vmcore_remap_oldmem_pfn(struct vm_area_struct *vma, · line 574
Quoted 10 lines · Adrian Hunter
fs/proc/vmcore.c +2 -1
@@ -559,7 +567,8 @@ static int vmcore_remap_oldmem_pfn(struct vm_area_struct *vma,
 	 * pages without a reason.
 	 */
 	idx = srcu_read_lock(&vmcore_cb_srcu);
-	if (!list_empty(&vmcore_cb_list))
+	if (!list_empty(&vmcore_cb_list) ||
+	    range_contains_unaccepted_memory(paddr, paddr + size))
 		ret = remap_oldmem_pfn_checked(vma, from, pfn, size, prot);
 	else
 		ret = remap_oldmem_pfn_range(vma, from, pfn, size, prot);
The whole callback mechanism which fs/proc/vmcore.c::pfn_is_ram() implements seems to be in place to ensure that there aren't a billion different "ram" checks in here.

Quoted 3 lines

Quoted 2 lines
Is there a reason you can't register_vmcore_cb() a callback to check for unaccepted memory?
Someone asked for the change to be in arch-independent code... 😉

That doesn't really answer my question. virtio_mem_init_kdump(), for instance, is in arch-independent code.

David Hildenbrand

2023-09-11 08:03 UTC #5 lore raw

Re: [PATCH 1/3] proc/vmcore: Do not map unaccepted memory

In reply to Adrian Hunter

Quoted 8 lines · Adrian Hunter

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 /proc/vmcore, which is read-only, this means a read or mmap of unaccepted memory will return zeros.

Does a second (kdump) kernel that exposes /proc/vmcore reliably get access to the information whether memory of the first kernel is unaccepted (IOW, not its memory, but the memory of the first kernel it is supposed to expose via /proc/vmcore)?

I recall there might be other kdump-related issues for TDX and friends to solve. Especially, which information the second kernel gets provided by the first kernel.

So can this patch even be tested reasonably (IOW, get into a kdump kernel in an environment where the first kernel has unaccepted memory, and verify that unaccepted memory is handled accordingly? ... while kdump doing anything reasonable in such an environment at all?)

Signature · 3 lines

Cheers,

David / dhildenb

Kirill A. Shutemov

2023-09-11 08:41 UTC #6 lore raw

Re: [PATCH 1/3] proc/vmcore: Do not map unaccepted memory

Quoted 7 lines · David Hildenbrand

Quoted 6 lines · Adrian Hunter
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.

Quoted 7 lines

Quoted 2 lines
For /proc/vmcore, which is read-only, this means a read or mmap of unaccepted memory will return zeros.
Does a second (kdump) kernel that exposes /proc/vmcore reliably get access to the information whether memory of the first kernel is unaccepted (IOW, not its memory, but the memory of the first kernel it is supposed to expose via /proc/vmcore)?

There are few patches in my queue to few related issue, but generally, yes, the information is available to the target kernel via EFI configuration table.

Signature · 1 line

  Kiryl Shutsemau / Kirill A. Shutemov

David Hildenbrand

2023-09-11 08:42 UTC #7 lore raw

Re: [PATCH 1/3] proc/vmcore: Do not map unaccepted memory

Quoted 11 lines · Kirill A. Shutemov

Quoted 10 lines · David Hildenbrand
Quoted 8 lines · Adrian Hunter
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 /proc/vmcore, which is read-only, this means a read or mmap of unaccepted memory will return zeros.

Quoted 8 lines

Quoted 4 lines
Does a second (kdump) kernel that exposes /proc/vmcore reliably get access to the information whether memory of the first kernel is unaccepted (IOW, not its memory, but the memory of the first kernel it is supposed to expose via /proc/vmcore)?
There are few patches in my queue to few related issue, but generally, yes, the information is available to the target kernel via EFI configuration table.

I assume that table provided by the first kernel, and not read directly from HW, correct?

Signature · 3 lines

Cheers,

David / dhildenb

Kirill A. Shutemov

2023-09-11 09:27 UTC #8 lore raw

Re: [PATCH 1/3] proc/vmcore: Do not map unaccepted memory

Quoted 17 lines · David Hildenbrand

Quoted 16 lines · Kirill A. Shutemov
Quoted 14 lines · David Hildenbrand
Quoted 8 lines · Adrian Hunter
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 /proc/vmcore, which is read-only, this means a read or mmap of unaccepted memory will return zeros.
Does a second (kdump) kernel that exposes /proc/vmcore reliably get access to the information whether memory of the first kernel is unaccepted (IOW, not its memory, but the memory of the first kernel it is supposed to expose via /proc/vmcore)?

Quoted 6 lines

Quoted 3 lines
There are few patches in my queue to few related issue, but generally, yes, the information is available to the target kernel via EFI configuration table.
I assume that table provided by the first kernel, and not read directly from HW, correct?

The table is constructed by the EFI stub in the first kernel based on EFI memory map.

Signature · 1 line

  Kiryl Shutsemau / Kirill A. Shutemov

David Hildenbrand

2023-09-11 09:50 UTC #9 lore raw

Re: [PATCH 1/3] proc/vmcore: Do not map unaccepted memory

Quoted 22 lines · Kirill A. Shutemov

Quoted 21 lines · David Hildenbrand
Quoted 19 lines · Kirill A. Shutemov
Quoted 7 lines · David Hildenbrand
Quoted 6 lines · Adrian Hunter
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.
Quoted 7 lines
Quoted 2 lines
For /proc/vmcore, which is read-only, this means a read or mmap of unaccepted memory will return zeros.
Does a second (kdump) kernel that exposes /proc/vmcore reliably get access to the information whether memory of the first kernel is unaccepted (IOW, not its memory, but the memory of the first kernel it is supposed to expose via /proc/vmcore)?
There are few patches in my queue to few related issue, but generally, yes, the information is available to the target kernel via EFI configuration table.

Quoted 6 lines

Quoted 2 lines
I assume that table provided by the first kernel, and not read directly from HW, correct?
The table is constructed by the EFI stub in the first kernel based on EFI memory map.

Okay, should work then once that's done by the first kernel.

Maybe include this patch in your series?

Signature · 3 lines

Cheers,

David / dhildenb

Kirill A. Shutemov

2023-09-11 10:06 UTC #10 lore raw

Re: [PATCH 1/3] proc/vmcore: Do not map unaccepted memory

Quoted 26 lines · David Hildenbrand

Quoted 25 lines · Kirill A. Shutemov
Quoted 23 lines · David Hildenbrand
Quoted 11 lines · Kirill A. Shutemov
Quoted 10 lines · David Hildenbrand
Quoted 8 lines · Adrian Hunter
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 /proc/vmcore, which is read-only, this means a read or mmap of unaccepted memory will return zeros.
Quoted 8 lines
Quoted 4 lines
Does a second (kdump) kernel that exposes /proc/vmcore reliably get access to the information whether memory of the first kernel is unaccepted (IOW, not its memory, but the memory of the first kernel it is supposed to expose via /proc/vmcore)?
There are few patches in my queue to few related issue, but generally, yes, the information is available to the target kernel via EFI configuration table.
I assume that table provided by the first kernel, and not read directly from HW, correct?

Quoted 7 lines

Quoted 3 lines
The table is constructed by the EFI stub in the first kernel based on EFI memory map.
Okay, should work then once that's done by the first kernel.
Maybe include this patch in your series?

Can do. But the other two patches are not related to kexec. Hm.

Signature · 1 line

  Kiryl Shutsemau / Kirill A. Shutemov

David Hildenbrand

2023-09-11 14:33 UTC #11 lore raw

Re: [PATCH 1/3] proc/vmcore: Do not map unaccepted memory

Quoted 31 lines · Kirill A. Shutemov

Quoted 30 lines · David Hildenbrand
Quoted 28 lines · Kirill A. Shutemov
Quoted 17 lines · David Hildenbrand
Quoted 16 lines · Kirill A. Shutemov
Quoted 14 lines · David Hildenbrand
Quoted 8 lines · Adrian Hunter
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 /proc/vmcore, which is read-only, this means a read or mmap of unaccepted memory will return zeros.
Does a second (kdump) kernel that exposes /proc/vmcore reliably get access to the information whether memory of the first kernel is unaccepted (IOW, not its memory, but the memory of the first kernel it is supposed to expose via /proc/vmcore)?
Quoted 6 lines
Quoted 3 lines
There are few patches in my queue to few related issue, but generally, yes, the information is available to the target kernel via EFI configuration table.
I assume that table provided by the first kernel, and not read directly from HW, correct?
The table is constructed by the EFI stub in the first kernel based on EFI memory map.

Quoted 5 lines

Quoted 3 lines
Okay, should work then once that's done by the first kernel.
Maybe include this patch in your series?
Can do. But the other two patches are not related to kexec. Hm.

Yes, the others can go in separately. But this here really needs other kexec/kdump changes.

Signature · 3 lines

Cheers,

David / dhildenb

Adrian Hunter

author

2023-09-06 07:39 UTC #12 lore raw

PATCH 2/3 proc/kcore: Do not map unaccepted memory

In reply to Adrian Hunter

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 /proc/kcore, which is read-only and does not support mmap, this means a read of unaccepted memory will return zeros.

Fixes dcdfdd40fa82 ("mm: Add support for unaccepted memory")

Signed-off-by Adrian Hunter <[email protected]>

---
 fs/proc/kcore.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

fs/proc/kcore.c +9 -1

index 23fc24d16b31..a3091c5ed871 100644
@@ -76,6 +76,13 @@ static int pfn_is_ram(unsigned long pfn)
 		return 1;
 }
 
+static bool pfn_is_unaccepted_memory(unsigned long pfn)
+{
+	phys_addr_t paddr = pfn << PAGE_SHIFT;
+
+	return range_contains_unaccepted_memory(paddr, paddr + PAGE_SIZE);
+}
+
 /* This doesn't grab kclist_lock, so it should only be used at init time. */
 void __init kclist_add(struct kcore_list *new, void *addr, size_t size,
 		       int type)

@@ -546,7 +553,8 @@ static ssize_t read_kcore_iter(struct kiocb *iocb, struct iov_iter *iter)
 			 * and explicitly excluded physical ranges.
 			 */
 			if (!page || PageOffline(page) ||
-			    is_page_hwpoison(page) || !pfn_is_ram(pfn)) {
+			    is_page_hwpoison(page) || !pfn_is_ram(pfn) ||
+			    pfn_is_unaccepted_memory(pfn)) {
 				if (iov_iter_zero(tsz, iter) != tsz) {
 					ret = -EFAULT;
 					goto out;

Signature · 1 line

2.34.1

Dave Hansen

2023-09-07 15:36 UTC #13 lore raw

Re: [PATCH 2/3] proc/kcore: Do not map unaccepted memory

fs/proc/kcore.c static int pfn_is_ram(unsigned long pfn) · line 84

Quoted 6 lines · Adrian Hunter

fs/proc/kcore.c +6 -0

+static bool pfn_is_unaccepted_memory(unsigned long pfn)
+{
+	phys_addr_t paddr = pfn << PAGE_SHIFT;
+
+	return range_contains_unaccepted_memory(paddr, paddr + PAGE_SIZE);
+}

Please just add this as an inline helper to common code rather than copying and pasting two copies around the tree.

Dave Hansen

2023-09-07 15:43 UTC #14 lore raw

Re: [PATCH 2/3] proc/kcore: Do not map unaccepted memory

In reply to Adrian Hunter

Quoted 4 lines · Adrian Hunter

Do not map unaccepted memory because it can cause the guest to fail.
For /proc/kcore, which is read-only and does not support mmap, this means a read of unaccepted memory will return zeros.

I'm confused by this changelog and subject.

What is getting "mapped" here if mmap() isn't in play?

Adrian Hunter

author

2023-09-06 07:39 UTC #15 lore raw

PATCH 3/3 /dev/mem: Do not map unaccepted memory

In reply to Adrian Hunter

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 <[email protected]>

---
 drivers/char/mem.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/char/mem.c +7 -2

index 1052b0f2d4cf..1a7c5728783c 100644
@@ -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);

Signature · 1 line

2.34.1

Kirill A. Shutemov

2023-09-07 10:06 UTC #16 lore raw

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

Quoted 9 lines · Adrian Hunter

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.

Signature · 1 line

  Kiryl Shutsemau / Kirill A. Shutemov

Dave Hansen

2023-09-07 14:15 UTC #17 lore raw

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

In reply to Adrian Hunter

Quoted 5 lines · Adrian Hunter

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

2023-09-07 14:25 UTC #18 lore raw

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

Quoted 10 lines · Dave Hansen

Quoted 5 lines · Adrian Hunter
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.

Signature · 1 line

  Kiryl Shutsemau / Kirill A. Shutemov

Dave Hansen

2023-09-07 14:56 UTC #19 lore raw

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

Quoted 11 lines · Kirill A. Shutemov

Quoted 9 lines · Dave Hansen
Quoted 5 lines · Adrian Hunter
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?

Quoted 1 line

config STRICT_DEVMEM

...

Quoted 4 lines

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

2023-09-07 15:07 UTC #20 lore raw

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

Quoted 1 line · Dave Hansen

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

2023-09-11 08:09 UTC #21 lore raw

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

In reply to Dave Hansen

Quoted 7 lines · Dave Hansen

Quoted 6 lines · Kirill A. Shutemov
Quoted 5 lines · Dave Hansen
Quoted 4 lines · Adrian Hunter
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.

Quoted 8 lines

Quoted 5 lines
Quoted 4 lines
Quoted 1 line
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 ...

Signature · 3 lines

Cheers,

David / dhildenb

Dave Hansen

2023-09-11 14:32 UTC #22 lore raw

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

Quoted 2 lines · David Hildenbrand

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.

Kirill A. Shutemov

Reviewed-by on the v1 series

2023-09-07 10:09 UTC #23 lore raw

In reply to Adrian Hunter

Quoted 8 lines · Adrian Hunter

Hi
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.
Plug a few gaps where RAM is exposed without checking if it is unaccepted memory.

Thanks for catching this. Looks good to me.

Reviewed-by Kirill A. Shutemov <[email protected]>

Signature · 1 line

  Kiryl Shutsemau / Kirill A. Shutemov