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.1On 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
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?
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
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.
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
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.
© 2016 - 2026 Red Hat, Inc.