[PATCH 0/3] Do not map unaccepted memory

[PATCH 0/3] Do not map unaccepted memory

[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.


Adrian Hunter (3):
      proc/vmcore: Do not map unaccepted memory
      proc/kcore: Do not map unaccepted memory
      /dev/mem: Do not map unaccepted memory

 drivers/char/mem.c |  9 +++++++--
 fs/proc/kcore.c    | 10 +++++++++-
 fs/proc/vmcore.c   | 15 ++++++++++++---
 3 files changed, 28 insertions(+), 6 deletions(-)


Regards
Adrian

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

[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.

Fixes: dcdfdd40fa82 ("mm: Add support for unaccepted memory")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 fs/proc/vmcore.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
index 1fb213f379a5..a28da2033ce8 100644
--- a/fs/proc/vmcore.c
+++ b/fs/proc/vmcore.c
@@ -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);
-- 
2.34.1

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

[Dave Hansen]

On 9/6/23 00:39, Adrian Hunter wrote:
> @@ -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?

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

[Adrian Hunter]

On 7/09/23 18:39, Dave Hansen wrote:
> On 9/6/23 00:39, Adrian Hunter wrote:
>> @@ -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?

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

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

[Dave Hansen]

On 9/7/23 08:44, Adrian Hunter wrote:
> On 7/09/23 18:39, Dave Hansen wrote:
>> On 9/6/23 00:39, Adrian Hunter wrote:
>>> @@ -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?
> 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.

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

[David Hildenbrand]

On 06.09.23 09: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.
> 
> 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?)

-- 
Cheers,

David / dhildenb

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

[Kirill A. Shutemov]

On Mon, Sep 11, 2023 at 10:03:36AM +0200, David Hildenbrand wrote:
> On 06.09.23 09: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.
> > 
> > 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.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[David Hildenbrand]

On 11.09.23 10:41, Kirill A. Shutemov wrote:
> On Mon, Sep 11, 2023 at 10:03:36AM +0200, David Hildenbrand wrote:
>> On 06.09.23 09: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.
>>>
>>> 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.

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

-- 
Cheers,

David / dhildenb

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

[Kirill A. Shutemov]

On Mon, Sep 11, 2023 at 10:42:51AM +0200, David Hildenbrand wrote:
> On 11.09.23 10:41, Kirill A. Shutemov wrote:
> > On Mon, Sep 11, 2023 at 10:03:36AM +0200, David Hildenbrand wrote:
> > > On 06.09.23 09: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.
> > > > 
> > > > 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.
> 
> 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.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[David Hildenbrand]

On 11.09.23 11:27, Kirill A. Shutemov wrote:
> On Mon, Sep 11, 2023 at 10:42:51AM +0200, David Hildenbrand wrote:
>> On 11.09.23 10:41, Kirill A. Shutemov wrote:
>>> On Mon, Sep 11, 2023 at 10:03:36AM +0200, David Hildenbrand wrote:
>>>> On 06.09.23 09: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.
>>>>>
>>>>> 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.
>>
>> 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?

-- 
Cheers,

David / dhildenb

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

[Kirill A. Shutemov]

On Mon, Sep 11, 2023 at 11:50:31AM +0200, David Hildenbrand wrote:
> On 11.09.23 11:27, Kirill A. Shutemov wrote:
> > On Mon, Sep 11, 2023 at 10:42:51AM +0200, David Hildenbrand wrote:
> > > On 11.09.23 10:41, Kirill A. Shutemov wrote:
> > > > On Mon, Sep 11, 2023 at 10:03:36AM +0200, David Hildenbrand wrote:
> > > > > On 06.09.23 09: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.
> > > > > > 
> > > > > > 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.
> > > 
> > > 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?

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

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[David Hildenbrand]

On 11.09.23 12:05, Kirill A. Shutemov wrote:
> On Mon, Sep 11, 2023 at 11:50:31AM +0200, David Hildenbrand wrote:
>> On 11.09.23 11:27, Kirill A. Shutemov wrote:
>>> On Mon, Sep 11, 2023 at 10:42:51AM +0200, David Hildenbrand wrote:
>>>> On 11.09.23 10:41, Kirill A. Shutemov wrote:
>>>>> On Mon, Sep 11, 2023 at 10:03:36AM +0200, David Hildenbrand wrote:
>>>>>> On 06.09.23 09: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.
>>>>>>>
>>>>>>> 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.
>>>>
>>>> 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?
> 
> 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.

-- 
Cheers,

David / dhildenb

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

[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 <adrian.hunter@intel.com>
---
 fs/proc/kcore.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 23fc24d16b31..a3091c5ed871 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -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;
-- 
2.34.1

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

[Dave Hansen]

On 9/6/23 00:39, Adrian Hunter wrote:
> +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.

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

[Dave Hansen]

On 9/6/23 00:39, Adrian Hunter wrote:
> 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?

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

[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 <adrian.hunter@intel.com>
---
 drivers/char/mem.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

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);
-- 
2.34.1

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

[Kirill A. Shutemov]

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.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[Dave Hansen]

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?

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

[Kirill A. Shutemov]

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.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[Dave Hansen]

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.

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

[Dave Hansen]

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.

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

[David Hildenbrand]

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 ...


-- 
Cheers,

David / dhildenb

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

[Dave Hansen]

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.

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

[Kirill A. Shutemov]

On Wed, Sep 06, 2023 at 10:38:59AM +0300, Adrian Hunter wrote:
> 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 <kirill.shutemov@linux.intel.com>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov