| Message ID | 20230906073902.4229-4-adrian.hunter@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Do not map unaccepted memory | expand |
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.
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.
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 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.
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 ...
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 --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);
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(-)