From 86ceb5c26ae9164a515bb556ed42352b985c2a34 Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Thu, 26 Sep 2024 17:56:00 -0700 Subject: [PATCH] Fix memmap.Translation.Perms returns. memmap.Mappable.Translate() is passed a hostarch.AccessType indicating what permissions are *immediately* required; it returns permissions in memmap.Translation.Perms that are granted *to MM* until invalidation. MM ensures that the permissions granted to the application are the intersection of those granted by Translate, and those granted by VMA permissions; see determination of pma.effectivePerms in mm.MemoryManager.getPMAsInternalLocked(). This mechanism is used to avoid marking pages dirty in the sentry's page cache for gofer-backed files until PROT_WRITE pages are actually written to; see gofer.dentry.Translate(). In most other cases, granting all supported permissions (to MM) up-front avoids a redundant page fault for pages that are touched first for reading, and later for writing. Also: - Prevent PROT_WRITE mappings of erofs files at mmap()/mprotect() time, rather than raising SIGBUS when writing to such mappings. - Map nvproxy.frontendFD with PlatformEffectPopulate. This is the original goal of this CL; however, before this rest of this CL, MM.MMap() => MM.populateVMAAndUnlock() => MM.getPMAsLocked(at=hostarch.NoAccess) => MM.getPMAsInternalLocked(at=hostarch.NoAccess) => nvproxy.frontendFD.Translate(at=hostarch.NoAccess) returns Translations with no permissions, causing MM.mapASLocked() to no-op. PiperOrigin-RevId: 679360594 --- pkg/sentry/devices/nvproxy/frontend_mmap.go | 8 +++++++- pkg/sentry/devices/nvproxy/uvm_mmap.go | 4 +--- pkg/sentry/devices/tpuproxy/accel/accel_fd_mmap.go | 2 +- .../devices/tpuproxy/vfio/pci_device_fd_mmap.go | 2 +- pkg/sentry/devices/tpuproxy/vfio/tpu_fd_mmap.go | 2 +- pkg/sentry/devices/tpuproxy/vfio/vfio_fd_mmap.go | 2 +- pkg/sentry/fsimpl/erofs/BUILD | 1 + pkg/sentry/fsimpl/erofs/regular_file.go | 12 +++++++++++- pkg/sentry/fsimpl/gofer/regular_file.go | 5 +---- pkg/sentry/fsimpl/iouringfs/iouringfs.go | 4 ++-- 10 files changed, 27 insertions(+), 15 deletions(-) diff --git a/pkg/sentry/devices/nvproxy/frontend_mmap.go b/pkg/sentry/devices/nvproxy/frontend_mmap.go index c3e538477..a13ae9d2e 100644 --- a/pkg/sentry/devices/nvproxy/frontend_mmap.go +++ b/pkg/sentry/devices/nvproxy/frontend_mmap.go @@ -23,6 +23,12 @@ import ( // ConfigureMMap implements vfs.FileDescriptionImpl.ConfigureMMap. func (fd *frontendFD) ConfigureMMap(ctx context.Context, opts *memmap.MMapOpts) error { + // Nvidia kernel driver: kernel-open/nvidia/nv-mmap.c:nvidia_mmap_helper() + // requires vm_pgoff == 0, so trying to lazily fault any subset of the + // mapping that doesn't include the beginning will fail. + if opts.PlatformEffect < memmap.PlatformEffectPopulate { + opts.PlatformEffect = memmap.PlatformEffectPopulate + } return vfs.GenericConfigureMMap(&fd.vfsfd, fd, opts) } @@ -47,7 +53,7 @@ func (fd *frontendFD) Translate(ctx context.Context, required, optional memmap.M Source: optional, File: &fd.memmapFile, Offset: optional.Start, - Perms: at, + Perms: hostarch.AnyAccess, }, }, nil } diff --git a/pkg/sentry/devices/nvproxy/uvm_mmap.go b/pkg/sentry/devices/nvproxy/uvm_mmap.go index eb9f8f7b3..6ce0e5920 100644 --- a/pkg/sentry/devices/nvproxy/uvm_mmap.go +++ b/pkg/sentry/devices/nvproxy/uvm_mmap.go @@ -54,9 +54,7 @@ func (fd *uvmFD) Translate(ctx context.Context, required, optional memmap.Mappab Source: optional, File: &fd.memmapFile, Offset: optional.Start, - // kernel-open/nvidia-uvm/uvm.c:uvm_mmap() requires mappings to be - // PROT_READ|PROT_WRITE. - Perms: hostarch.ReadWrite, + Perms: hostarch.AnyAccess, }, }, nil } diff --git a/pkg/sentry/devices/tpuproxy/accel/accel_fd_mmap.go b/pkg/sentry/devices/tpuproxy/accel/accel_fd_mmap.go index 1e11c7ba8..df36d5a09 100644 --- a/pkg/sentry/devices/tpuproxy/accel/accel_fd_mmap.go +++ b/pkg/sentry/devices/tpuproxy/accel/accel_fd_mmap.go @@ -50,7 +50,7 @@ func (fd *accelFD) Translate(ctx context.Context, required, optional memmap.Mapp Source: optional, File: &fd.memmapFile, Offset: optional.Start, - Perms: at, + Perms: hostarch.AnyAccess, }, }, nil } diff --git a/pkg/sentry/devices/tpuproxy/vfio/pci_device_fd_mmap.go b/pkg/sentry/devices/tpuproxy/vfio/pci_device_fd_mmap.go index d8e640db9..91afe3449 100644 --- a/pkg/sentry/devices/tpuproxy/vfio/pci_device_fd_mmap.go +++ b/pkg/sentry/devices/tpuproxy/vfio/pci_device_fd_mmap.go @@ -61,7 +61,7 @@ func (fd *pciDeviceFD) Translate(ctx context.Context, required, optional memmap. Source: optional, File: &fd.memmapFile, Offset: optional.Start, - Perms: at, + Perms: hostarch.AnyAccess, }, }, nil } diff --git a/pkg/sentry/devices/tpuproxy/vfio/tpu_fd_mmap.go b/pkg/sentry/devices/tpuproxy/vfio/tpu_fd_mmap.go index 494d6859b..c8dcb97d6 100644 --- a/pkg/sentry/devices/tpuproxy/vfio/tpu_fd_mmap.go +++ b/pkg/sentry/devices/tpuproxy/vfio/tpu_fd_mmap.go @@ -50,7 +50,7 @@ func (fd *tpuFD) Translate(ctx context.Context, required, optional memmap.Mappab Source: optional, File: &fd.memmapFile, Offset: optional.Start, - Perms: at, + Perms: hostarch.AnyAccess, }, }, nil } diff --git a/pkg/sentry/devices/tpuproxy/vfio/vfio_fd_mmap.go b/pkg/sentry/devices/tpuproxy/vfio/vfio_fd_mmap.go index b57a535db..c4ddd8664 100644 --- a/pkg/sentry/devices/tpuproxy/vfio/vfio_fd_mmap.go +++ b/pkg/sentry/devices/tpuproxy/vfio/vfio_fd_mmap.go @@ -50,7 +50,7 @@ func (fd *vfioFD) Translate(ctx context.Context, required, optional memmap.Mappa Source: optional, File: &fd.memmapFile, Offset: optional.Start, - Perms: at, + Perms: hostarch.AnyAccess, }, }, nil } diff --git a/pkg/sentry/fsimpl/erofs/BUILD b/pkg/sentry/fsimpl/erofs/BUILD index 7fd92e831..f4808f4a9 100644 --- a/pkg/sentry/fsimpl/erofs/BUILD +++ b/pkg/sentry/fsimpl/erofs/BUILD @@ -61,6 +61,7 @@ go_library( "//pkg/errors/linuxerr", "//pkg/fspath", "//pkg/hostarch", + "//pkg/log", "//pkg/refs", "//pkg/safemem", "//pkg/sentry/fsimpl/lock", diff --git a/pkg/sentry/fsimpl/erofs/regular_file.go b/pkg/sentry/fsimpl/erofs/regular_file.go index b2f6ea76f..6d5617153 100644 --- a/pkg/sentry/fsimpl/erofs/regular_file.go +++ b/pkg/sentry/fsimpl/erofs/regular_file.go @@ -23,6 +23,7 @@ import ( "gvisor.dev/gvisor/pkg/erofs" "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/hostarch" + "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/safemem" "gvisor.dev/gvisor/pkg/sentry/memmap" "gvisor.dev/gvisor/pkg/sentry/vfs" @@ -126,6 +127,9 @@ func (fd *regularFileFD) Seek(ctx context.Context, offset int64, whence int32) ( // ConfigureMMap implements vfs.FileDescriptionImpl.ConfigureMMap. func (fd *regularFileFD) ConfigureMMap(ctx context.Context, opts *memmap.MMapOpts) error { + if opts.MaxPerms.Write && !opts.Private { + return linuxerr.EINVAL + } return vfs.GenericConfigureMMap(&fd.vfsfd, fd.inode(), opts) } @@ -163,6 +167,10 @@ func (i *inode) Translate(ctx context.Context, required, optional memmap.Mappabl optional.End = pgend } if at.Write { + // This shouldn't be possible due to the check in ConfigureMMap(). + inodeTranslateWriteWarnOnce.Do(func() { + log.Traceback("erofs.inode.Translate: unexpected access type %v", at) + }) return nil, &memmap.BusError{linuxerr.EROFS} } offset, err := i.DataOffset() @@ -175,11 +183,13 @@ func (i *inode) Translate(ctx context.Context, required, optional memmap.Mappabl Source: mr, File: &i.fs.mf, Offset: mr.Start + offset, - Perms: at, + Perms: hostarch.ReadExecute, }, }, nil } +var inodeTranslateWriteWarnOnce sync.Once + // InvalidateUnsavable implements memmap.Mappable.InvalidateUnsavable. func (i *inode) InvalidateUnsavable(ctx context.Context) error { i.mapsMu.Lock() diff --git a/pkg/sentry/fsimpl/gofer/regular_file.go b/pkg/sentry/fsimpl/gofer/regular_file.go index 8f029cb9d..42836a376 100644 --- a/pkg/sentry/fsimpl/gofer/regular_file.go +++ b/pkg/sentry/fsimpl/gofer/regular_file.go @@ -809,10 +809,7 @@ func (d *dentry) Translate(ctx context.Context, required, optional memmap.Mappab segMR := seg.Range().Intersect(optional) // TODO(jamieliu): Make Translations writable even if writability is // not required if already kept-dirty by another writable translation. - perms := hostarch.AccessType{ - Read: true, - Execute: true, - } + perms := hostarch.ReadExecute if at.Write { // From this point forward, this memory can be dirtied through the // mapping at any time. diff --git a/pkg/sentry/fsimpl/iouringfs/iouringfs.go b/pkg/sentry/fsimpl/iouringfs/iouringfs.go index f7ecebdda..1a576b19d 100644 --- a/pkg/sentry/fsimpl/iouringfs/iouringfs.go +++ b/pkg/sentry/fsimpl/iouringfs/iouringfs.go @@ -570,7 +570,7 @@ func (sqemf *sqEntriesFile) Translate(ctx context.Context, required, optional me Source: source, File: pgalloc.MemoryFileFromContext(ctx), Offset: sqemf.fr.Start + source.Start, - Perms: at, + Perms: hostarch.AnyAccess, }, }, nil } @@ -616,7 +616,7 @@ func (rbmf *ringsBufferFile) Translate(ctx context.Context, required, optional m Source: source, File: pgalloc.MemoryFileFromContext(ctx), Offset: rbmf.fr.Start + source.Start, - Perms: at, + Perms: hostarch.AnyAccess, }, }, nil }