> Date: Mon, 18 Feb 2019 10:33:15 +0100
> From: Hans-Joerg Hoexer <hans-joerg_hoe...@genua.de>
>
> 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.
Hi Hans-Jeorg,
I suspect the direct map has to work for things like GDB's "target
kvm" to work. Otherwise inspecting the contents of some of the memory
pools will fail.
You seem to imply that you can crash the kernel by just reading some
random memory address. That surprises me somewhat, even though I
think I managed to do exactly that not too long ago. In principle the
data is read using kcopy(9), which should recover and return an error
if we get a fault. Makes me wonder if somehow the trap handling is
subtly broken on amd64. Do you have more details on how you triggered
the kernel crash?
Thanks,
Mark
> -----
> 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;
>
>
