From 1009a70c49deeb5086104852c2b3f13faadbf852 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Tue, 5 Jul 2022 22:06:33 +0000
Subject: [PATCH 1/6] mmap(2): Avoid overflow in rounding and checking size.
---
sys/uvm/uvm_mmap.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/sys/uvm/uvm_mmap.c b/sys/uvm/uvm_mmap.c
index 618a2d52ed89..941e08959b7c 100644
--- a/sys/uvm/uvm_mmap.c
+++ b/sys/uvm/uvm_mmap.c
@@ -274,7 +274,7 @@ sys_mmap(struct lwp *l, const struct sys_mmap_args *uap, register_t *retval)
struct proc *p = l->l_proc;
vaddr_t addr;
off_t pos;
- vsize_t size, pageoff, newsize;
+ vsize_t size, pageoff;
vm_prot_t prot, maxprot, extraprot;
int flags, fd, advice;
vaddr_t defaddr = 0; /* XXXGCC */
@@ -309,17 +309,21 @@ sys_mmap(struct lwp *l, const struct sys_mmap_args *uap, register_t *retval)
return EINVAL;
/*
- * align file position and save offset. adjust size.
+ * Align file position and save offset into page. Adjust size
+ * so that it is an integral multiple of the page size.
*/
-
- pageoff = (pos & PAGE_MASK);
- pos -= pageoff;
- newsize = size + pageoff; /* add offset */
- newsize = (vsize_t)round_page(newsize); /* round up */
-
- if (newsize < size)
+ pageoff = pos & PAGE_MASK;
+ pos -= pageoff;
+ CTASSERT(PAGE_MASK <= __type_max(vsize_t));
+ CTASSERT((__type_max(vsize_t) - PAGE_SIZE + 1) % PAGE_SIZE == 0);
+ if (size > __type_max(vsize_t) - PAGE_SIZE + 1 - pageoff)
return ENOMEM;
- size = newsize;
+ /*
+ * size + pageoff <= VSIZE_MAX + 1 - PAGE_SIZE, and the
+ * right-hand side is an integral multiple of the page size, so
+ * round_page(size + pageoff) <= VSIZE_MAX + 1 - PAGE_SIZE.
+ */
+ size = round_page(size + pageoff);
/*
* now check (MAP_FIXED) or get (!MAP_FIXED) the "addr"
From 570e9d237e32fbb54f796348afdd4ad1ec806c5e Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Tue, 5 Jul 2022 22:07:57 +0000
Subject: [PATCH 2/6] mmap(2): Assert size != 0 in non-anonymous case.
This is guaranteed by a test earlier; adding the assertion just makes
it clearer that it applies to the branch where we call fo_mmap -- no
functional change intended.
---
sys/uvm/uvm_mmap.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/sys/uvm/uvm_mmap.c b/sys/uvm/uvm_mmap.c
index 941e08959b7c..2f6f47062ce0 100644
--- a/sys/uvm/uvm_mmap.c
+++ b/sys/uvm/uvm_mmap.c
@@ -368,6 +368,8 @@ sys_mmap(struct lwp *l, const struct sys_mmap_args *uap, register_t *retval)
advice = UVM_ADV_NORMAL;
if ((flags & MAP_ANON) == 0) {
+ KASSERT(size != 0);
+
if ((fp = fd_getfile(fd)) == NULL)
return EBADF;
From 7a5b9524ea3fac23c0cdff66c75376b26386f2be Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Tue, 5 Jul 2022 22:33:14 +0000
Subject: [PATCH 3/6] uvm(9): fo_mmap caller guarantees positive size.
No functional change intended, just sprinkling assertions to make it
clearer.
Sprinkle KNF while here.
---
sys/dev/audio/audio.c | 12 ++++++++----
sys/dev/nvmm/nvmm.c | 2 ++
sys/external/bsd/drm2/drm/drm_cdevsw.c | 4 +++-
sys/external/bsd/drm2/drm/drm_gem_vm.c | 4 ++++
sys/external/bsd/drm2/drm/drm_vm.c | 1 +
sys/kern/kern_ksyms.c | 1 +
sys/kern/subr_kcov.c | 2 ++
sys/kern/vfs_vnops.c | 2 ++
sys/uvm/uvm_device.c | 2 ++
sys/uvm/uvm_mmap.c | 2 ++
10 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/sys/dev/audio/audio.c b/sys/dev/audio/audio.c
index ab7bda60cce4..fcbea43ac5de 100644
--- a/sys/dev/audio/audio.c
+++ b/sys/dev/audio/audio.c
@@ -2098,6 +2098,8 @@ audiommap(struct file *fp, off_t *offp, size_t len, int prot, int *flagsp,
int bound;
int error;
+ KASSERT(len > 0);
+
KASSERT(fp->f_audioctx);
file = fp->f_audioctx;
dev = file->dev;
@@ -3551,10 +3553,10 @@ audio_kqfilter(struct audio_softc *sc, audio_file_t *file, struct knote *kn)
/*
* Must be called without sc_lock nor sc_exlock held.
*/
-int
+static int
audio_mmap(struct audio_softc *sc, off_t *offp, size_t len, int prot,
- int *flagsp, int *advicep, struct uvm_object **uobjp, int *maxprotp,
- audio_file_t *file)
+ int *flagsp, int *advicep, struct uvm_object **uobjp, int *maxprotp,
+ audio_file_t *file)
{
audio_track_t *track;
vsize_t vsize;
@@ -3562,6 +3564,8 @@ audio_mmap(struct audio_softc *sc, off_t *offp, size_t len, int prot,
TRACEF(2, file, "off=%lld, prot=%d", (long long)(*offp), prot);
+ KASSERT(len > 0);
+
if (*offp < 0)
return EINVAL;
@@ -3594,7 +3598,7 @@ audio_mmap(struct audio_softc *sc, off_t *offp, size_t len, int prot,
vsize = roundup2(MAX(track->usrbuf.capacity, PAGE_SIZE), PAGE_SIZE);
if (len > vsize)
return EOVERFLOW;
- if (*offp > (uint)(vsize - len))
+ if (*offp > (uint)(vsize - len)) /* XXX integer truncation? */
return EOVERFLOW;
/* XXX TODO: what happens when mmap twice. */
diff --git a/sys/dev/nvmm/nvmm.c b/sys/dev/nvmm/nvmm.c
index 316366c721d2..fe5e58be2ecc 100644
--- a/sys/dev/nvmm/nvmm.c
+++ b/sys/dev/nvmm/nvmm.c
@@ -1099,6 +1099,8 @@ nvmm_mmap(file_t *fp, off_t *offp, size_t size, int prot, int *flagsp,
nvmm_cpuid_t cpuid;
int error;
+ KASESRT(size > 0);
+
if (prot & PROT_EXEC)
return EACCES;
if (size != PAGE_SIZE)
diff --git a/sys/external/bsd/drm2/drm/drm_cdevsw.c b/sys/external/bsd/drm2/drm/drm_cdevsw.c
index 004ffc5ec689..862192fdffff 100644
--- a/sys/external/bsd/drm2/drm/drm_cdevsw.c
+++ b/sys/external/bsd/drm2/drm/drm_cdevsw.c
@@ -513,13 +513,15 @@ drm_stat(struct file *fp, struct stat *st)
static int
drm_fop_mmap(struct file *fp, off_t *offp, size_t len, int prot, int *flagsp,
- int *advicep, struct uvm_object **uobjp, int *maxprotp)
+ int *advicep, struct uvm_object **uobjp, int *maxprotp)
{
struct drm_file *const file = fp->f_data;
struct drm_device *const dev = file->minor->dev;
int error;
KASSERT(fp == file->filp);
+ KASSERT(len > 0);
+
/* XXX errno Linux->NetBSD */
error = -(*dev->driver->mmap_object)(dev, *offp, len, prot, uobjp,
offp, file->filp);
diff --git a/sys/external/bsd/drm2/drm/drm_gem_vm.c b/sys/external/bsd/drm2/drm/drm_gem_vm.c
index af2c8bf5def8..1d663ea07b66 100644
--- a/sys/external/bsd/drm2/drm/drm_gem_vm.c
+++ b/sys/external/bsd/drm2/drm/drm_gem_vm.c
@@ -71,6 +71,8 @@ drm_gem_or_legacy_mmap_object(struct drm_device *dev, off_t byte_offset,
{
int ret;
+ KASSERT(nbytes > 0);
+
ret = drm_gem_mmap_object(dev, byte_offset, nbytes, prot, uobjp,
uoffsetp, file);
if (ret)
@@ -88,6 +90,8 @@ drm_gem_mmap_object(struct drm_device *dev, off_t byte_offset, size_t nbytes,
{
int ret;
+ KASSERT(nbytes > 0);
+
mutex_lock(&dev->struct_mutex);
ret = drm_gem_mmap_object_locked(dev, byte_offset, nbytes, prot,
uobjp, uoffsetp, file);
diff --git a/sys/external/bsd/drm2/drm/drm_vm.c b/sys/external/bsd/drm2/drm/drm_vm.c
index 3dbe4c5b9b16..a3ddcc44c1c7 100644
--- a/sys/external/bsd/drm2/drm/drm_vm.c
+++ b/sys/external/bsd/drm2/drm/drm_vm.c
@@ -60,6 +60,7 @@ drm_legacy_mmap_object(struct drm_device *dev, off_t offset, size_t size,
struct uvm_object *uobj;
KASSERT(offset == (offset & ~(PAGE_SIZE-1)));
+ KASSERT(size != 0);
/*
* Attach the device. The size and offset are used only for
diff --git a/sys/kern/kern_ksyms.c b/sys/kern/kern_ksyms.c
index 8dc7ca57a3f5..9346d459258b 100644
--- a/sys/kern/kern_ksyms.c
+++ b/sys/kern/kern_ksyms.c
@@ -1426,6 +1426,7 @@ ksymsmmap(struct file *fp, off_t *offp, size_t nbytes, int prot, int *flagsp,
/* uvm_mmap guarantees page-aligned offset and size. */
KASSERT(*offp == round_page(*offp));
KASSERT(nbytes == round_page(nbytes));
+ KASSERT(nbytes > 0);
/* Refuse negative offsets. */
if (*offp < 0)
diff --git a/sys/kern/subr_kcov.c b/sys/kern/subr_kcov.c
index d8f9e42719c0..14324f7472e2 100644
--- a/sys/kern/subr_kcov.c
+++ b/sys/kern/subr_kcov.c
@@ -524,6 +524,8 @@ kcov_fops_mmap(file_t *fp, off_t *offp, size_t size, int prot, int *flagsp,
kcov_t *kd, *kdbuf;
int error = 0;
+ KASSERT(size > 0);
+
if (prot & PROT_EXEC)
return EACCES;
if (off < 0)
diff --git a/sys/kern/vfs_vnops.c b/sys/kern/vfs_vnops.c
index e750b8498ccd..1e7e36207a3a 100644
--- a/sys/kern/vfs_vnops.c
+++ b/sys/kern/vfs_vnops.c
@@ -919,6 +919,8 @@ vn_mmap(struct file *fp, off_t *offp, size_t size, int prot, int *flagsp,
flags = *flagsp;
maxprot = VM_PROT_EXECUTE;
+ KASSERT(size > 0);
+
vp = fp->f_vnode;
if (vp->v_type != VREG && vp->v_type != VCHR &&
vp->v_type != VBLK) {
diff --git a/sys/uvm/uvm_device.c b/sys/uvm/uvm_device.c
index 997e0dba7bde..7aa80f9f8189 100644
--- a/sys/uvm/uvm_device.c
+++ b/sys/uvm/uvm_device.c
@@ -117,6 +117,8 @@ udv_attach(dev_t device, vm_prot_t accessprot,
UVMHIST_FUNC(__func__);
UVMHIST_CALLARGS(maphist, "(device=%#jx)", device,0,0,0);
+ KASSERT(size != 0);
+
/*
* before we do anything, ensure this device supports mmap
*/
diff --git a/sys/uvm/uvm_mmap.c b/sys/uvm/uvm_mmap.c
index 2f6f47062ce0..831c9213b034 100644
--- a/sys/uvm/uvm_mmap.c
+++ b/sys/uvm/uvm_mmap.c
@@ -1011,6 +1011,8 @@ uvm_mmap_dev(struct proc *p, void **addrp, size_t len, dev_t dev,
struct uvm_object *uobj;
int error, flags, prot;
+ KASSERT(len > 0);
+
flags = MAP_SHARED;
prot = VM_PROT_READ | VM_PROT_WRITE;
if (*addrp)
From 110119da8ed597da1d5649aa5ffadfd0f13cee0b Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Tue, 5 Jul 2022 23:07:02 +0000
Subject: [PATCH 4/6] mmap(2): Avoid overflow in overflow check in vn_mmap.
Sprinkle KNF while here.
---
sys/kern/vfs_vnops.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/sys/kern/vfs_vnops.c b/sys/kern/vfs_vnops.c
index 1e7e36207a3a..680fb011de10 100644
--- a/sys/kern/vfs_vnops.c
+++ b/sys/kern/vfs_vnops.c
@@ -111,16 +111,16 @@ int (*vn_union_readdir_hook) (struct vnode **, struct file *, struct lwp *);
#include <sys/verified_exec.h>
static int vn_read(file_t *fp, off_t *offset, struct uio *uio,
- kauth_cred_t cred, int flags);
+ kauth_cred_t cred, int flags);
static int vn_write(file_t *fp, off_t *offset, struct uio *uio,
- kauth_cred_t cred, int flags);
+ kauth_cred_t cred, int flags);
static int vn_closefile(file_t *fp);
static int vn_poll(file_t *fp, int events);
static int vn_fcntl(file_t *fp, u_int com, void *data);
static int vn_statfile(file_t *fp, struct stat *sb);
static int vn_ioctl(file_t *fp, u_long com, void *data);
static int vn_mmap(struct file *, off_t *, size_t, int, int *, int *,
- struct uvm_object **, int *);
+ struct uvm_object **, int *);
static int vn_seek(struct file *, off_t, int, off_t *, int);
const struct fileops vnops = {
@@ -902,7 +902,7 @@ vn_kqfilter(file_t *fp, struct knote *kn)
static int
vn_mmap(struct file *fp, off_t *offp, size_t size, int prot, int *flagsp,
- int *advicep, struct uvm_object **uobjp, int *maxprotp)
+ int *advicep, struct uvm_object **uobjp, int *maxprotp)
{
struct uvm_object *uobj;
struct vnode *vp;
@@ -930,7 +930,8 @@ vn_mmap(struct file *fp, off_t *offp, size_t size, int prot, int *flagsp,
if (vp->v_type != VCHR && off < 0) {
return EINVAL;
}
- if (vp->v_type != VCHR && (off_t)(off + size) < off) {
+ if (vp->v_type != VCHR &&
+ (size > __type_max(off_t) || off > __type_max(off_t) - size)) {
/* no offset wrapping */
return EOVERFLOW;
}
From 000a369c513459cfa4b79519b6ba40ba7c290213 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Tue, 5 Jul 2022 23:09:50 +0000
Subject: [PATCH 5/6] mmap(2): Prohibit overflowing offsets for
non-D_NEGOFFSAFE devices.
---
sys/uvm/uvm_device.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/sys/uvm/uvm_device.c b/sys/uvm/uvm_device.c
index 7aa80f9f8189..4e64c7c318ea 100644
--- a/sys/uvm/uvm_device.c
+++ b/sys/uvm/uvm_device.c
@@ -133,12 +133,17 @@ udv_attach(dev_t device, vm_prot_t accessprot,
}
/*
- * Negative offsets on the object are not allowed.
+ * Negative offsets on the object are not allowed, unless the
+ * device has affirmatively set D_NEGOFFSAFE.
*/
-
- if ((cdev->d_flag & D_NEGOFFSAFE) == 0 &&
- off != UVM_UNKNOWN_OFFSET && off < 0)
- return(NULL);
+ if ((cdev->d_flag & D_NEGOFFSAFE) == 0 && off != UVM_UNKNOWN_OFFSET) {
+ if (off < 0)
+ return NULL;
+ if (size > __type_max(voff_t))
+ return NULL;
+ if (off > __type_max(voff_t) - size)
+ return NULL;
+ }
/*
* Check that the specified range of the device allows the
From 369a240dd0587ee5dc34fa67ad48f3569a9c7785 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Tue, 5 Jul 2022 23:10:24 +0000
Subject: [PATCH 6/6] mmap(2): Guarantee two's-complement wraparound for
D_NEGOFFSAFE.
XXX Not sure this should be allowed at all, but this way we don't
change the semantics of the existing code which was written under
essentially the assumption of -fwrapv.
---
sys/uvm/uvm_device.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/sys/uvm/uvm_device.c b/sys/uvm/uvm_device.c
index 4e64c7c318ea..c2517e5a2b64 100644
--- a/sys/uvm/uvm_device.c
+++ b/sys/uvm/uvm_device.c
@@ -152,13 +152,29 @@ udv_attach(dev_t device, vm_prot_t accessprot,
* XXX assumes VM_PROT_* == PROT_*
* XXX clobbers off and size, but nothing else here needs them.
*/
-
- while (size != 0) {
- if (cdev_mmap(device, off, accessprot) == -1) {
- return (NULL);
+ do {
+ KASSERTMSG((off % PAGE_SIZE) == 0, "off=%jd", (intmax_t)off);
+ KASSERTMSG(size >= PAGE_SIZE, "size=%"PRIuVSIZE, size);
+ if (cdev_mmap(device, off, accessprot) == -1)
+ return NULL;
+ KASSERT(off <= __type_max(voff_t) - PAGE_SIZE ||
+ (cdev->d_flag & D_NEGOFFSAFE) != 0);
+ if (__predict_false(off > __type_max(voff_t) - PAGE_SIZE)) {
+ /*
+ * off += PAGE_SIZE, with two's-complement
+ * wraparound, or
+ *
+ * off += PAGE_SIZE - 2*(VOFF_MAX + 1).
+ */
+ CTASSERT(PAGE_SIZE >= 2);
+ off -= __type_max(voff_t);
+ off += PAGE_SIZE - 2;
+ off -= __type_max(voff_t);
+ } else {
+ off += PAGE_SIZE;
}
- off += PAGE_SIZE; size -= PAGE_SIZE;
- }
+ size -= PAGE_SIZE;
+ } while (size != 0);
/*
* keep looping until we get it