From a5459a26cb0644f078f7b782b8849037b4dcf4b3 Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Mon, 30 Sep 2024 15:07:34 -0700 Subject: [PATCH] Add memmap.MMapOpts.RequirePlatformEffect and use where appropriate. Per the comment for vfs.GenericProxyDeviceConfigureMMap(), this ensures that if invalid arguments are provided to application mmap() for a proxy device file, then an error is returned immediately (from host mmap()) rather than when demand paging causes host mmap() to be invoked. PiperOrigin-RevId: 680733678 --- pkg/sentry/devices/nvproxy/frontend_mmap.go | 5 +- pkg/sentry/devices/nvproxy/uvm_mmap.go | 5 +- .../devices/tpuproxy/accel/accel_fd_mmap.go | 2 +- .../tpuproxy/vfio/pci_device_fd_mmap.go | 2 +- .../devices/tpuproxy/vfio/tpu_fd_mmap.go | 2 +- .../devices/tpuproxy/vfio/vfio_fd_mmap.go | 2 +- pkg/sentry/memmap/memmap.go | 9 +++ pkg/sentry/mm/syscalls.go | 71 +++++++++++-------- pkg/sentry/vfs/file_description_impl_util.go | 13 ++++ 9 files changed, 69 insertions(+), 42 deletions(-) diff --git a/pkg/sentry/devices/nvproxy/frontend_mmap.go b/pkg/sentry/devices/nvproxy/frontend_mmap.go index a13ae9d2e..8f15a2c49 100644 --- a/pkg/sentry/devices/nvproxy/frontend_mmap.go +++ b/pkg/sentry/devices/nvproxy/frontend_mmap.go @@ -26,10 +26,7 @@ func (fd *frontendFD) ConfigureMMap(ctx context.Context, opts *memmap.MMapOpts) // 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) + return vfs.GenericProxyDeviceConfigureMMap(&fd.vfsfd, fd, opts) } // AddMapping implements memmap.Mappable.AddMapping. diff --git a/pkg/sentry/devices/nvproxy/uvm_mmap.go b/pkg/sentry/devices/nvproxy/uvm_mmap.go index 6ce0e5920..f063b6c25 100644 --- a/pkg/sentry/devices/nvproxy/uvm_mmap.go +++ b/pkg/sentry/devices/nvproxy/uvm_mmap.go @@ -27,10 +27,7 @@ func (fd *uvmFD) ConfigureMMap(ctx context.Context, opts *memmap.MMapOpts) error // UVM_VALIDATE_VA_RANGE, and probably other ioctls, expect that // application mmaps of /dev/nvidia-uvm are immediately visible to the // driver. - if opts.PlatformEffect < memmap.PlatformEffectPopulate { - opts.PlatformEffect = memmap.PlatformEffectPopulate - } - return vfs.GenericConfigureMMap(&fd.vfsfd, fd, opts) + return vfs.GenericProxyDeviceConfigureMMap(&fd.vfsfd, fd, opts) } // AddMapping implements memmap.Mappable.AddMapping. diff --git a/pkg/sentry/devices/tpuproxy/accel/accel_fd_mmap.go b/pkg/sentry/devices/tpuproxy/accel/accel_fd_mmap.go index df36d5a09..ee0648485 100644 --- a/pkg/sentry/devices/tpuproxy/accel/accel_fd_mmap.go +++ b/pkg/sentry/devices/tpuproxy/accel/accel_fd_mmap.go @@ -26,7 +26,7 @@ import ( // ConfigureMMap implements vfs.FileDescriptionImpl.ConfigureMMap. func (fd *accelFD) ConfigureMMap(ctx context.Context, opts *memmap.MMapOpts) error { - return vfs.GenericConfigureMMap(&fd.vfsfd, fd, opts) + return vfs.GenericProxyDeviceConfigureMMap(&fd.vfsfd, fd, opts) } // AddMapping implements memmap.Mappable.AddMapping. 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 91afe3449..426804806 100644 --- a/pkg/sentry/devices/tpuproxy/vfio/pci_device_fd_mmap.go +++ b/pkg/sentry/devices/tpuproxy/vfio/pci_device_fd_mmap.go @@ -25,7 +25,7 @@ import ( // ConfigureMMap implements vfs.FileDescriptionImpl.ConfigureMMap. func (fd *pciDeviceFD) ConfigureMMap(ctx context.Context, opts *memmap.MMapOpts) error { - return vfs.GenericConfigureMMap(&fd.vfsfd, fd, opts) + return vfs.GenericProxyDeviceConfigureMMap(&fd.vfsfd, fd, opts) } // AddMapping implements memmap.Mappable.AddMapping. diff --git a/pkg/sentry/devices/tpuproxy/vfio/tpu_fd_mmap.go b/pkg/sentry/devices/tpuproxy/vfio/tpu_fd_mmap.go index c8dcb97d6..7e98dfa3b 100644 --- a/pkg/sentry/devices/tpuproxy/vfio/tpu_fd_mmap.go +++ b/pkg/sentry/devices/tpuproxy/vfio/tpu_fd_mmap.go @@ -26,7 +26,7 @@ import ( // ConfigureMMap implements vfs.FileDescriptionImpl.ConfigureMMap. func (fd *tpuFD) ConfigureMMap(ctx context.Context, opts *memmap.MMapOpts) error { - return vfs.GenericConfigureMMap(&fd.vfsfd, fd, opts) + return vfs.GenericProxyDeviceConfigureMMap(&fd.vfsfd, fd, opts) } // AddMapping implements memmap.Mappable.AddMapping. diff --git a/pkg/sentry/devices/tpuproxy/vfio/vfio_fd_mmap.go b/pkg/sentry/devices/tpuproxy/vfio/vfio_fd_mmap.go index c4ddd8664..361a0cc61 100644 --- a/pkg/sentry/devices/tpuproxy/vfio/vfio_fd_mmap.go +++ b/pkg/sentry/devices/tpuproxy/vfio/vfio_fd_mmap.go @@ -26,7 +26,7 @@ import ( // ConfigureMMap implements vfs.FileDescriptionImpl.ConfigureMMap. func (fd *vfioFD) ConfigureMMap(ctx context.Context, opts *memmap.MMapOpts) error { - return vfs.GenericConfigureMMap(&fd.vfsfd, fd, opts) + return vfs.GenericProxyDeviceConfigureMMap(&fd.vfsfd, fd, opts) } // AddMapping implements memmap.Mappable.AddMapping. diff --git a/pkg/sentry/memmap/memmap.go b/pkg/sentry/memmap/memmap.go index 90303bd24..254265da2 100644 --- a/pkg/sentry/memmap/memmap.go +++ b/pkg/sentry/memmap/memmap.go @@ -360,6 +360,8 @@ type MMapOpts struct { // Linux. Stack bool + // PlatformEffect controls the synchronous effect of this call on the + // underlying platform.AddressSpace. PlatformEffect MMapPlatformEffect // MLockMode specifies the memory locking behavior of the mapping. @@ -378,6 +380,13 @@ type MMapOpts struct { // If Force is true, Unmap and Fixed must be true. Force bool + // If RequirePlatformEffect is false, PlatformEffect is best-effort; + // failure to create mappings in the platform.AddressSpace are silently + // ignored. If RequirePlatformEffect is true, failure to create mappings in + // the platform.AddressSpace cause MMap() to fail. (If PlatformEffect is + // PlatformEffectDefault, RequirePlatformEffect is ignored.) + RequirePlatformEffect bool + // SentryOwnedContent indicates the sentry exclusively controls the // underlying memory backing the mapping thus the memory content is // guaranteed not to be modified outside the sentry's purview. diff --git a/pkg/sentry/mm/syscalls.go b/pkg/sentry/mm/syscalls.go index fc6bac91f..86b1d33ae 100644 --- a/pkg/sentry/mm/syscalls.go +++ b/pkg/sentry/mm/syscalls.go @@ -114,12 +114,16 @@ func (mm *MemoryManager) MMap(ctx context.Context, opts memmap.MMapOpts) (hostar } // Get the new vma. - var droppedIDs []memmap.MappingIdentity mm.mappingMu.Lock() if opts.MLockMode < mm.defMLockMode { opts.MLockMode = mm.defMLockMode } - vseg, ar, droppedIDs, err := mm.createVMALocked(ctx, opts, droppedIDs) + vseg, ar, droppedIDs, err := mm.createVMALocked(ctx, opts, nil /* droppedIDs */) + defer func() { + for _, id := range droppedIDs { + id.DecRef(ctx) + } + }() if err != nil { mm.mappingMu.Unlock() return 0, err @@ -131,7 +135,19 @@ func (mm *MemoryManager) MMap(ctx context.Context, opts memmap.MMapOpts) (hostar // mm/util.c:vm_mmap_pgoff() => mm/gup.c:__mm_populate() => // populate_vma_page_range(). Confirm this behavior. switch { - case opts.PlatformEffect >= memmap.PlatformEffectPopulate || opts.MLockMode == memmap.MLockEager: + case opts.PlatformEffect >= memmap.PlatformEffectPopulate: + if opts.RequirePlatformEffect { + if err := mm.populateVMA(ctx, vseg, ar, opts.PlatformEffect); err != nil { + _, droppedIDs = mm.unmapLocked(ctx, ar, droppedIDs) + mm.mappingMu.Unlock() + return 0, err + } + mm.mappingMu.Unlock() + break + } + fallthrough + + case opts.MLockMode == memmap.MLockEager: // Get pmas and map as requested. mm.populateVMAAndUnlock(ctx, vseg, ar, opts.PlatformEffect) @@ -148,10 +164,6 @@ func (mm *MemoryManager) MMap(ctx context.Context, opts memmap.MMapOpts) (hostar mm.mappingMu.Unlock() } - for _, id := range droppedIDs { - id.DecRef(ctx) - } - return ar.Start, nil } @@ -161,11 +173,11 @@ func (mm *MemoryManager) MMap(ctx context.Context, opts memmap.MMapOpts) (hostar // Preconditions: // - mm.mappingMu must be locked. // - vseg.Range().IsSupersetOf(ar). -func (mm *MemoryManager) populateVMA(ctx context.Context, vseg vmaIterator, ar hostarch.AddrRange, platformEffect memmap.MMapPlatformEffect) { +func (mm *MemoryManager) populateVMA(ctx context.Context, vseg vmaIterator, ar hostarch.AddrRange, platformEffect memmap.MMapPlatformEffect) error { if !vseg.ValuePtr().effectivePerms.Any() { - // Linux doesn't populate inaccessible pages. See - // mm/gup.c:populate_vma_page_range. - return + // mm.mapASLocked() will no-op due to platform.AddressSpace.MapFile() + // precondition. + return nil } mm.activeMu.Lock() @@ -175,32 +187,29 @@ func (mm *MemoryManager) populateVMA(ctx context.Context, vseg vmaIterator, ar h // AddressSpace. if mm.as == nil { mm.activeMu.Unlock() - return + return nil } // Ensure that we have usable pmas. pseg, _, err := mm.getPMAsLocked(ctx, vseg, ar, hostarch.NoAccess, platformEffect == memmap.PlatformEffectCommit) if err != nil { - // mm/util.c:vm_mmap_pgoff() ignores the error, if any, from - // mm/gup.c:mm_populate(). If it matters, we'll get it again when - // userspace actually tries to use the failing page. mm.activeMu.Unlock() - return + return err } // Downgrade to a read-lock on activeMu since we don't need to mutate pmas // anymore. mm.activeMu.DowngradeLock() - - // As above, errors are silently ignored. - mm.mapASLocked(pseg, ar, platformEffect) + err = mm.mapASLocked(pseg, ar, platformEffect) mm.activeMu.RUnlock() + return err } // populateVMAAndUnlock is equivalent to populateVMA, but also unconditionally -// unlocks mm.mappingMu. In cases where populateVMAAndUnlock is usable, it is -// preferable to populateVMA since it unlocks mm.mappingMu before performing -// expensive operations that don't require it to be locked. +// unlocks mm.mappingMu and discards errors. In cases where +// populateVMAAndUnlock is usable, it is preferable to populateVMA since it +// unlocks mm.mappingMu before performing expensive operations that don't +// require it to be locked. // // Preconditions: // - mm.mappingMu must be locked for writing. @@ -209,13 +218,15 @@ func (mm *MemoryManager) populateVMA(ctx context.Context, vseg vmaIterator, ar h // Postconditions: mm.mappingMu will be unlocked. // +checklocksrelease:mm.mappingMu func (mm *MemoryManager) populateVMAAndUnlock(ctx context.Context, vseg vmaIterator, ar hostarch.AddrRange, platformEffect memmap.MMapPlatformEffect) { - // See populateVMA above for commentary. if !vseg.ValuePtr().effectivePerms.Any() { + // Linux doesn't populate inaccessible pages. See + // mm/gup.c:populate_vma_page_range. mm.mappingMu.Unlock() return } mm.activeMu.Lock() + // Can't defer mm.activeMu.Unlock(); see below. if mm.as == nil { mm.activeMu.Unlock() @@ -229,10 +240,14 @@ func (mm *MemoryManager) populateVMAAndUnlock(ctx context.Context, vseg vmaItera pseg, _, err := mm.getPMAsLocked(ctx, vseg, ar, hostarch.NoAccess, platformEffect == memmap.PlatformEffectCommit) mm.mappingMu.RUnlock() if err != nil { + // mm/util.c:vm_mmap_pgoff() ignores the error, if any, from + // mm/gup.c:mm_populate(). If it matters, we'll get it again when + // userspace actually tries to use the failing page. mm.activeMu.Unlock() return } + // As above, errors are silently ignored. mm.activeMu.DowngradeLock() mm.mapASLocked(pseg, ar, platformEffect) mm.activeMu.RUnlock() @@ -268,11 +283,8 @@ func (mm *MemoryManager) MapStack(ctx context.Context) (hostarch.AddrRange, erro return hostarch.AddrRange{}, linuxerr.ENOMEM } stackStart := stackEnd - szaddr - var droppedIDs []memmap.MappingIdentity - var ar hostarch.AddrRange - var err error mm.mappingMu.Lock() - _, ar, droppedIDs, err = mm.createVMALocked(ctx, memmap.MMapOpts{ + _, ar, droppedIDs, err := mm.createVMALocked(ctx, memmap.MMapOpts{ Length: sz, Addr: stackStart, Perms: hostarch.ReadWrite, @@ -281,7 +293,7 @@ func (mm *MemoryManager) MapStack(ctx context.Context) (hostarch.AddrRange, erro GrowsDown: true, MLockMode: mm.defMLockMode, Hint: "[stack]", - }, droppedIDs) + }, nil /* droppedIDs */) mm.mappingMu.Unlock() for _, id := range droppedIDs { id.DecRef(ctx) @@ -306,9 +318,8 @@ func (mm *MemoryManager) MUnmap(ctx context.Context, addr hostarch.Addr, length return linuxerr.EINVAL } - var droppedIDs []memmap.MappingIdentity mm.mappingMu.Lock() - _, droppedIDs = mm.unmapLocked(ctx, ar, droppedIDs) + _, droppedIDs := mm.unmapLocked(ctx, ar, nil /* droppedIDs */) mm.mappingMu.Unlock() for _, id := range droppedIDs { diff --git a/pkg/sentry/vfs/file_description_impl_util.go b/pkg/sentry/vfs/file_description_impl_util.go index 3c8455163..9a25b80a9 100644 --- a/pkg/sentry/vfs/file_description_impl_util.go +++ b/pkg/sentry/vfs/file_description_impl_util.go @@ -426,6 +426,19 @@ func GenericConfigureMMap(fd *FileDescription, m memmap.Mappable, opts *memmap.M return nil } +// GenericProxyDeviceConfigureMMap may be used by most implementations of +// FileDescriptionImpl.ConfigureMMap for which the underlying memmap.File is a +// host device file, whose implementation of mmap() may have unusual +// requirements and so should be called immediately (during application mmap()) +// to propagate any errors. +func GenericProxyDeviceConfigureMMap(fd *FileDescription, m memmap.Mappable, opts *memmap.MMapOpts) error { + if opts.PlatformEffect < memmap.PlatformEffectPopulate { + opts.PlatformEffect = memmap.PlatformEffectPopulate + } + opts.RequirePlatformEffect = true + return GenericConfigureMMap(fd, m, opts) +} + // LockFD may be used by most implementations of FileDescriptionImpl.Lock* // functions. Caller must call Init(). //