Hi,
when running with insecure securelevel one can crash the amd64 kernel
by accessing memory that is not available to the kernel as indicated
by uvm_kernacc() (see amd64/mem.c):
int
mmrw(dev_t dev, struct uio *uio, int flags)
{
...
/* minor device 1 is kernel memory */
case 1:
v = uio->uio_offset;
c = ulmin(iov->iov_len, MAXPHYS);
if (v >= (vaddr_t)&start && v < kern_end) {
if (v < (vaddr_t)&etext &&
uio->uio_rw == UIO_WRITE)
return EFAULT;
} else if ((!uvm_kernacc((caddr_t)v, c,
uio->uio_rw == UIO_READ ? B_READ : B_WRITE)) &&
XXX (v < PMAP_DIRECT_BASE && v > PMAP_DIRECT_END))
return (EFAULT);
error = uiomove((caddr_t)v, c, uio);
continue;
...
The marked line looks bogus: To the knowledge of UVM v is not accessable
and the condition (v < PMAP_DIRECT_BASE && v > PMAP_DIRECT_END) is
always false. Thus we do not return EFAULT but go on to uiomove()
which will result in an exception.
That line was added with revision 1.3 of amd64/mem.c. I think the intetion
was to allow v to be in the PMAP_DIRECT area. That area is IMHO sort
of unknown to/out of scope UVM, thus uvm_kernacc() returning false.
The obvious fix would be s/&&/||/. However, I wonder if it actually makes
sense to allow access to that area. As far as I see other PMAP_DIRECT
architecture just have the uvm_kernacc() check. So maybe revision 1.3
should just be backed out?
I've included diffs for both reasonings.
The impact of this issues is low, as proper securelevel will prevent
such an access anyway.
Take care,
HJ.
-----
diff --git a/sys/arch/amd64/amd64/mem.c b/sys/arch/amd64/amd64/mem.c
index da186df1f09..c5cb22e4c4a 100644
--- a/sys/arch/amd64/amd64/mem.c
+++ b/sys/arch/amd64/amd64/mem.c
@@ -152,9 +152,8 @@ mmrw(dev_t dev, struct uio *uio, int flags)
if (v < (vaddr_t)&etext &&
uio->uio_rw == UIO_WRITE)
return EFAULT;
- } else if ((!uvm_kernacc((caddr_t)v, c,
- uio->uio_rw == UIO_READ ? B_READ : B_WRITE)) &&
- (v < PMAP_DIRECT_BASE && v > PMAP_DIRECT_END))
+ } else if (!uvm_kernacc((caddr_t)v, c,
+ uio->uio_rw == UIO_READ ? B_READ : B_WRITE))
return (EFAULT);
error = uiomove((caddr_t)v, c, uio);
continue;
-----
diff --git a/sys/arch/amd64/amd64/mem.c b/sys/arch/amd64/amd64/mem.c
index da186df1f09..67636f08977 100644
--- a/sys/arch/amd64/amd64/mem.c
+++ b/sys/arch/amd64/amd64/mem.c
@@ -154,7 +154,7 @@ mmrw(dev_t dev, struct uio *uio, int flags)
return EFAULT;
} else if ((!uvm_kernacc((caddr_t)v, c,
uio->uio_rw == UIO_READ ? B_READ : B_WRITE)) &&
- (v < PMAP_DIRECT_BASE && v > PMAP_DIRECT_END))
+ (v < PMAP_DIRECT_BASE || v > PMAP_DIRECT_END))
return (EFAULT);
error = uiomove((caddr_t)v, c, uio);
continue;
