Date: Sun, 19 Mar 2017 16:01:44 +0000 (UTC) From: Konstantin Belousov <kib@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: svn commit: r315563 - in stable/11: lib/libc/sys sys/kern sys/vm Message-ID: <201703191601.v2JG1iBO005845@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: kib Date: Sun Mar 19 16:01:44 2017 New Revision: 315563 URL: https://svnweb.freebsd.org/changeset/base/315563 Log: MFC r313690: Consistently handle negative or wrapping offsets in the mmap(2) syscalls. MFC r315158: Fix two missed places where vm_object offset to index calculation should use unsigned shift. Modified: stable/11/lib/libc/sys/mmap.2 stable/11/sys/kern/uipc_shm.c stable/11/sys/kern/vfs_vnops.c stable/11/sys/vm/device_pager.c stable/11/sys/vm/sg_pager.c stable/11/sys/vm/vm_map.c stable/11/sys/vm/vm_object.h Directory Properties: stable/11/ (props changed) Modified: stable/11/lib/libc/sys/mmap.2 ============================================================================== --- stable/11/lib/libc/sys/mmap.2 Sun Mar 19 15:56:06 2017 (r315562) +++ stable/11/lib/libc/sys/mmap.2 Sun Mar 19 16:01:44 2017 (r315563) @@ -28,7 +28,7 @@ .\" @(#)mmap.2 8.4 (Berkeley) 5/11/95 .\" $FreeBSD$ .\" -.Dd November 25, 2016 +.Dd February 4, 2017 .Dt MMAP 2 .Os .Sh NAME @@ -53,11 +53,37 @@ starting at byte offset .Fa offset . If .Fa len -is not a multiple of the pagesize, the mapped region may extend past the +is not a multiple of the page size, the mapped region may extend past the specified range. Any such extension beyond the end of the mapped object will be zero-filled. .Pp If +.Fa fd +references a regular file or a shared memory object, the range of +bytes starting at +.Fa offset +and continuing for +.Fa len +bytes must be legitimate for the possible (not necessarily +current) offsets in the object. +In particular, the +.Fa offset +value cannot be negative. +If the object is truncated and the process later accesses a pages that +is wholly within the truncated region, the access is aborted and a +.Dv SIGBUS +signal is delivered to the process. +.Pp +If +.Fa fd +references a device file, the interpretation of the +.Fa offset +value is device specific and defined by the device driver. +The virtual memory subsystem does not impose any restrictitions on the +.Fa offset +value in this case, passing it unchanged to the driver. +.Pp +If .Fa addr is non-zero, it is used as a hint to the system. (As a convenience to the system, the actual address of the region may differ @@ -157,7 +183,7 @@ If .Dv MAP_FIXED is specified, .Fa addr -must be a multiple of the pagesize. +must be a multiple of the page size. If .Dv MAP_EXCL is not specified, a successful @@ -361,6 +387,12 @@ The argument is not a valid open file descriptor. .It Bq Er EINVAL +An invalid (negative) value was passed in the +.Fa offset +argument, when +.Fa fd +referenced a regular file or shared memory. +.It Bq Er EINVAL An invalid value was passed in the .Fa prot argument. Modified: stable/11/sys/kern/uipc_shm.c ============================================================================== --- stable/11/sys/kern/uipc_shm.c Sun Mar 19 15:56:06 2017 (r315562) +++ stable/11/sys/kern/uipc_shm.c Sun Mar 19 16:01:44 2017 (r315563) @@ -886,20 +886,20 @@ shm_mmap(struct file *fp, vm_map_t map, return (EACCES); maxprot &= cap_maxprot; + /* See comment in vn_mmap(). */ + if ( +#ifdef _LP64 + objsize > OFF_MAX || +#endif + foff < 0 || foff > OFF_MAX - objsize) + return (EINVAL); + #ifdef MAC error = mac_posixshm_check_mmap(td->td_ucred, shmfd, prot, flags); if (error != 0) return (error); #endif - /* - * XXXRW: This validation is probably insufficient, and subject to - * sign errors. It should be fixed. - */ - if (foff >= shmfd->shm_size || - foff + objsize > round_page(shmfd->shm_size)) - return (EINVAL); - mtx_lock(&shm_timestamp_lock); vfs_timestamp(&shmfd->shm_atime); mtx_unlock(&shm_timestamp_lock); Modified: stable/11/sys/kern/vfs_vnops.c ============================================================================== --- stable/11/sys/kern/vfs_vnops.c Sun Mar 19 15:56:06 2017 (r315562) +++ stable/11/sys/kern/vfs_vnops.c Sun Mar 19 16:01:44 2017 (r315563) @@ -2452,6 +2452,24 @@ vn_mmap(struct file *fp, vm_map_t map, v } maxprot &= cap_maxprot; + /* + * For regular files and shared memory, POSIX requires that + * the value of foff be a legitimate offset within the data + * object. In particular, negative offsets are invalid. + * Blocking negative offsets and overflows here avoids + * possible wraparound or user-level access into reserved + * ranges of the data object later. In contrast, POSIX does + * not dictate how offsets are used by device drivers, so in + * the case of a device mapping a negative offset is passed + * on. + */ + if ( +#ifdef _LP64 + size > OFF_MAX || +#endif + foff < 0 || foff > OFF_MAX - size) + return (EINVAL); + writecounted = FALSE; error = vm_mmap_vnode(td, size, prot, &maxprot, &flags, vp, &foff, &object, &writecounted); Modified: stable/11/sys/vm/device_pager.c ============================================================================== --- stable/11/sys/vm/device_pager.c Sun Mar 19 15:56:06 2017 (r315562) +++ stable/11/sys/vm/device_pager.c Sun Mar 19 16:01:44 2017 (r315563) @@ -139,8 +139,18 @@ cdev_pager_allocate(void *handle, enum o if (foff & PAGE_MASK) return (NULL); + /* + * Treat the mmap(2) file offset as an unsigned value for a + * device mapping. This, in effect, allows a user to pass all + * possible off_t values as the mapping cookie to the driver. At + * this point, we know that both foff and size are a multiple + * of the page size. Do a check to avoid wrap. + */ size = round_page(size); - pindex = OFF_TO_IDX(foff + size); + pindex = UOFF_TO_IDX(foff) + UOFF_TO_IDX(size); + if (pindex > OBJ_MAX_SIZE || pindex < UOFF_TO_IDX(foff) || + pindex < UOFF_TO_IDX(size)) + return (NULL); if (ops->cdev_pg_ctor(handle, size, prot, foff, cred, &color) != 0) return (NULL); Modified: stable/11/sys/vm/sg_pager.c ============================================================================== --- stable/11/sys/vm/sg_pager.c Sun Mar 19 15:56:06 2017 (r315562) +++ stable/11/sys/vm/sg_pager.c Sun Mar 19 16:01:44 2017 (r315563) @@ -96,8 +96,9 @@ sg_pager_alloc(void *handle, vm_ooffset_ * to map beyond that. */ size = round_page(size); - pindex = OFF_TO_IDX(foff + size); - if (pindex > npages) + pindex = UOFF_TO_IDX(foff) + UOFF_TO_IDX(size); + if (pindex > npages || pindex < UOFF_TO_IDX(foff) || + pindex < UOFF_TO_IDX(size)) return (NULL); /* Modified: stable/11/sys/vm/vm_map.c ============================================================================== --- stable/11/sys/vm/vm_map.c Sun Mar 19 15:56:06 2017 (r315562) +++ stable/11/sys/vm/vm_map.c Sun Mar 19 16:01:44 2017 (r315563) @@ -4124,7 +4124,7 @@ RetryLookup:; * Return the object/offset from this entry. If the entry was * copy-on-write or empty, it has been fixed up. */ - *pindex = OFF_TO_IDX((vaddr - entry->start) + entry->offset); + *pindex = UOFF_TO_IDX((vaddr - entry->start) + entry->offset); *object = entry->object.vm_object; *out_prot = prot; @@ -4205,7 +4205,7 @@ vm_map_lookup_locked(vm_map_t *var_map, * Return the object/offset from this entry. If the entry was * copy-on-write or empty, it has been fixed up. */ - *pindex = OFF_TO_IDX((vaddr - entry->start) + entry->offset); + *pindex = UOFF_TO_IDX((vaddr - entry->start) + entry->offset); *object = entry->object.vm_object; *out_prot = prot; Modified: stable/11/sys/vm/vm_object.h ============================================================================== --- stable/11/sys/vm/vm_object.h Sun Mar 19 15:56:06 2017 (r315562) +++ stable/11/sys/vm/vm_object.h Sun Mar 19 16:01:44 2017 (r315563) @@ -195,8 +195,23 @@ struct vm_object { #define OBJ_DISCONNECTWNT 0x4000 /* disconnect from vnode wanted */ #define OBJ_TMPFS 0x8000 /* has tmpfs vnode allocated */ +/* + * Helpers to perform conversion between vm_object page indexes and offsets. + * IDX_TO_OFF() converts an index into an offset. + * OFF_TO_IDX() converts an offset into an index. Since offsets are signed + * by default, the sign propagation in OFF_TO_IDX(), when applied to + * negative offsets, is intentional and returns a vm_object page index + * that cannot be created by a userspace mapping. + * UOFF_TO_IDX() treats the offset as an unsigned value and converts it + * into an index accordingly. Use it only when the full range of offset + * values are allowed. Currently, this only applies to device mappings. + * OBJ_MAX_SIZE specifies the maximum page index corresponding to the + * maximum unsigned offset. + */ #define IDX_TO_OFF(idx) (((vm_ooffset_t)(idx)) << PAGE_SHIFT) #define OFF_TO_IDX(off) ((vm_pindex_t)(((vm_ooffset_t)(off)) >> PAGE_SHIFT)) +#define UOFF_TO_IDX(off) (((vm_pindex_t)(off)) >> PAGE_SHIFT) +#define OBJ_MAX_SIZE (UOFF_TO_IDX(UINT64_MAX) + 1) #ifdef _KERNEL
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201703191601.v2JG1iBO005845>