x86, PAT: honor CONFIG_STRICT_DEVMEM if pat is disabled

since 0124cecf (x86, PAT: disable /dev/mem mmap RAM with PAT) we are disabling access to the system RAM as if STRICT_DEVMEM was enabled by default. If we, however, disable pat by nopat kernel parameter we can access that memory without considering CONFIG_STRICT_DEVMEM.

Let's make the code to honor STRICT_DEVMEM while we still preserve disabled by default behavior. This means that the access to the system memory is granted only if nopat is provided and STRICT_DEVMEM is disabled.

Signed-off-by Michal Hocko <[email protected]>

---
 arch/x86/mm/pat.c |   10 ++--------

 1 files changed, 2 insertions(+), 8 deletions(-)

arch/x86/mm/pat.c +2 -9

index f6ff57b..fbaf3fc 100644
@@ -472,13 +472,6 @@ pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
        return vma_prot;
  }
 
-#ifdef CONFIG_STRICT_DEVMEM
-/* This check is done in drivers/char/mem.c in case of STRICT_DEVMEM*/
-static inline int range_is_allowed(unsigned long pfn, unsigned long size)
-{
-       return 1;
-}
-#else
  /* This check is needed to avoid cache aliasing when PAT is enabled */
  static inline int range_is_allowed(unsigned long pfn, unsigned long size)
  {

@@ -486,8 +479,10 @@ static inline int range_is_allowed(unsigned long pfn, unsigned long size)
        u64 to = from + size;
        u64 cursor = from;
 
+#ifndef CONFIG_STRICT_DEVMEM
        if (!pat_enabled)
                return 1;
+#endif
 
        while (cursor < to) {
                if (!devmem_is_allowed(pfn)) {

@@ -501,7 +496,6 @@ static inline int range_is_allowed(unsigned long pfn, unsigned long size)
        }
        return 1;
  }
-#endif /* CONFIG_STRICT_DEVMEM */
 
  int phys_mem_access_prot_allowed(struct file *file, unsigned long pfn,
                                unsigned long size, pgprot_t *vma_prot)
--

1.7.5.4

Quoted 5 lines · Michal Hocko

since 0124cecf (x86, PAT: disable /dev/mem mmap RAM with PAT) we are disabling access to the system RAM as if STRICT_DEVMEM was enabled by default. If we, however, disable pat by nopat kernel parameter we can access that memory without considering CONFIG_STRICT_DEVMEM.

Looking at the code, I don't think this problem exists. Code in pat.c disables /dev/mem mmap only when !CONFIG_STRICT_DEVMEM.

Quoted 9 lines · Venki Pallipadi

Quoted 5 lines · Michal Hocko

since 0124cecf (x86, PAT: disable /dev/mem mmap RAM with PAT) we are disabling access to the system RAM as if STRICT_DEVMEM was enabled by default. If we, however, disable pat by nopat kernel parameter we can access that memory without considering CONFIG_STRICT_DEVMEM.

Looking at the code, I don't think this problem exists. Code in pat.c disables /dev/mem mmap only when !CONFIG_STRICT_DEVMEM.

Ahh yes, you are right. I got confused by having two separate range_is_allowed functions and didn't realize which one is called from which context.
       mmap_mem
         -> range_is_allowed [drivers/char/mem.c]
              -EPERM
         -> phys_mem_access_prot_allowed
              -> range_is_allowed [arch/x86/mm/pat.c]

So we really get stopped at the first check and do not get to the later one.

While we are at it. {read,write}_mem checks only range_is_allowed (mem.c version) and it doesn't call phys_mem_access_prot_allowed so we do not consider PAT. {read,write}_kmem doesn't check the range at all. Is this correct?
Why do we care _only_ about mmap?

Quoted 20 lines · Michal Hocko

Quoted 9 lines · Venki Pallipadi

Quoted 5 lines · Michal Hocko

since 0124cecf (x86, PAT: disable /dev/mem mmap RAM with PAT) we are disabling access to the system RAM as if STRICT_DEVMEM was enabled by default. If we, however, disable pat by nopat kernel parameter we can access that memory without considering CONFIG_STRICT_DEVMEM.

Looking at the code, I don't think this problem exists. Code in pat.c disables /dev/mem mmap only when !CONFIG_STRICT_DEVMEM.

Ahh yes, you are right. I got confused by having two separate range_is_allowed functions and didn't realize which one is called from which context.
       mmap_mem
         -> range_is_allowed [drivers/char/mem.c]
              -EPERM
         -> phys_mem_access_prot_allowed
              -> range_is_allowed [arch/x86/mm/pat.c]

Quoted 9 lines

So we really get stopped at the first check and do not get to the later one.

While we are at it. {read,write}_mem checks only range_is_allowed (mem.c version) and it doesn't call phys_mem_access_prot_allowed so we do not consider PAT. {read,write}_kmem doesn't check the range at all. Is this correct?
Why do we care _only_ about mmap?

Yes. The problem with PAT and /dev/mem mmap is with untracked mmaps in user address space. Once mapped the mappings can stay in user space and if there is a page attribute change for that physical address later kernel doesn't have handle on all such user space mappings that may exist.
Read/Write on the other hand use direct mappings and will not cause this problem. I have generally been suggesting tools that want to access /dev/mem and are unable to mmap it due to PAT to use read/write instead. /dev/mem on 32 bit has some problems of its own and the recent patchset on mailing list fixes most of the issues there.