Re: inconsistency in MD mmmmap() implementations?

YAMAMOTO Takashi wrote:
>> YAMAMOTO Takashi wrote:
>>
>>> there is nothing wrong to be inconsistent between ports if they have
>>> different memory layouts.
>>> eg. "atop(off) >= physmem" can make sense if its memory is
>>> contiguously mapped from physical address 0.
>>> (well, maybe the "suser" part should be consistent.  but it isn't
>>> what you are asking, right?)
>> I'm interested in both knowing if these are not wrong (like you say,
>> amd64 is wrong, maybe others are too?) and I also want to use kauth(9)
>> there. So before I'm writing the code I'm verifying that the current
>> behavior is correct.
> 
> i386 and alpha seems sane.  amd64 seems wrong.
> i don't know others.
> 
>>> whether each versions are correct or not is a different question. :-)
>>> at least amd64 version seems wrong.  it should be basically the same as 
>>> i386.
>> Do you want to fix it?
> 
> i don't think i'll fix it in a timely manner.
> 
>> or should we wait for the amd64 port-master?
> 
> no.  it shouldn't be too hard for anyone interested.

I tried making a patch (adapted copy/paste of the i386 code) for amd64;
does it look okay?

-e.

-- 
Elad Efrat

Index: sys/arch/amd64/amd64/mem.c
===================================================================
RCS file: /cvsroot/src/sys/arch/amd64/amd64/mem.c,v
retrieving revision 1.7
diff -u -p -r1.7 mem.c
--- sys/arch/amd64/amd64/mem.c  23 Jul 2006 22:06:04 -0000      1.7
+++ sys/arch/amd64/amd64/mem.c  29 Oct 2006 23:14:07 -0000
@@ -115,6 +115,8 @@ const struct cdevsw mem_cdevsw = {
        nostop, notty, nopoll, mmmmap, nokqfilter
 };
 
+static int check_pa_acc(paddr_t, vm_prot_t);
+
 /*ARGSUSED*/
 int
 mmrw(dev, uio, flags)
@@ -155,6 +157,9 @@ mmrw(dev, uio, flags)
                        v = uio->uio_offset;
                        prot = uio->uio_rw == UIO_READ ? VM_PROT_READ :
                            VM_PROT_WRITE;
+                       error = check_pa_acc(uio->uio_offset, prot);
+                       if (error)
+                               break;
                        pmap_enter(pmap_kernel(), (vaddr_t)vmmap,
                            trunc_page(v), prot, PMAP_WIRED|prot);
                        o = uio->uio_offset & PGOFSET;
@@ -220,14 +225,40 @@ mmrw(dev, uio, flags)
        return (error);
 }
 
+#include <sys/kcore.h>
+
+/*
+ * check_pa_acc: check if given pa is accessible.
+ */
+static int
+check_pa_acc(paddr_t pa, vm_prot_t prot __unused)
+{
+       extern phys_ram_seg_t mem_clusters[VM_PHYSSEG_MAX];
+       extern int mem_cluster_cnt;
+       int i;
+
+       if (securelevel <= 0) {
+               return 0;
+       }
+
+       for (i = 0; i < mem_cluster_cnt; i++) {
+               const phys_ram_seg_t *seg = &mem_clusters[i];
+               paddr_t lstart = seg->start;
+
+               if (lstart <= pa && pa - lstart <= seg->size) {
+                       return 0;
+               }
+       }
+
+       return EPERM;
+}
+
 paddr_t
 mmmmap(dev, off, prot)
        dev_t dev;
        off_t off;
        int prot;
 {
-       struct lwp *l = curlwp; /* XXX */
-
        /*
         * /dev/mem is the only one that makes sense through this
         * interface.  For /dev/kmem any physaddr we return here
@@ -239,8 +270,8 @@ mmmmap(dev, off, prot)
        if (minor(dev) != DEV_MEM)
                return (-1);
 
-       if (off > ctob(physmem) && kauth_authorize_generic(l->l_cred,
-           KAUTH_GENERIC_ISSUSER, &l->l_acflag) != 0)
+       if (check_pa_acc(off, prot) != 0)
                return (-1);
+
        return (x86_btop(off));
 }