Re: Patch [1/1] minor bugfix in 2.6.26/arch/x86/mm/pat.c - caused problems in mmap() of /dev/mem character file

["V.Radhakrishnan" <rk@atr-labs.com>]

> The idea behind this check is to add a second layer of checks to mmap()s 
> - which is absolutely necessary in the case of PAT and not optional. If 
> non-promisc /dev/mem is used then this check is not needed. (because the 
> higher level API already restricts us)
> 
> But this duplication is ugly and confusing - we should have just a 
> single check function, defined once and unconditionally, and utilized by 
> both places (with the devmem check being optional).
> 
> Venki, Arjan, agreed?

In fact, I spent some time thinking that the bug was actually at the
higher level API instead of in the arch specific layer, and I was amazed
that in spite of the config level option being turned off, it was NOT
being reflected in the code !!

This being my first submission to the kernel, I sent it to the mailing
list cc to Linus, as mentioned in the SubmittingPatches file WITHOUT
first subscribing to the mailing list ! Perhaps this is why the thread
did not appear ! ( 'Bug' in the SubmittingPatches file ?? )

Anyway, this is the original patch submitted, after verifying with the
perl script.

--- arch/x86/mm/pat.c.orig	2008-07-17 22:04:18.000000000 +0530
+++ arch/x86/mm/pat.c	2008-07-17 22:43:39.000000000 +0530
@@ -5,7 +5,11 @@
  *          Suresh B Siddha <suresh.b.siddha@intel.com>
  *
  * Loosely based on earlier PAT patchset from Eric Biederman and Andi
Kleen.
- */
+ *
+ * Bug fixed - V. Radhakrishnan <rk@atr-labs.com> on 17-07-2008
+		#ifdef CONFIG_NONPROMISC_DEVMEM changed to
+		#ifndef CONFIG_NONPROMISC_DEVMEM
+*/
 
 #include <linux/mm.h>
 #include <linux/kernel.h>
@@ -471,7 +475,7 @@ pgprot_t phys_mem_access_prot(struct fil
 	return vma_prot;
 }
 
-#ifdef CONFIG_NONPROMISC_DEVMEM
+#ifndef CONFIG_NONPROMISC_DEVMEM
 /* This check is done in drivers/char/mem.c in case of
NONPROMISC_DEVMEM*/
 static inline int range_is_allowed(unsigned long pfn, unsigned long
size)
 {
@@ -586,4 +590,3 @@ void unmap_devmem(unsigned long pfn, uns
 
 	free_memtype(addr, addr + size);
 }
-
Signed-off-by: Radhakrishnan <rk@atr-labs.com>
---




On Fri, 2008-07-18 at 00:39 +0200, Ingo Molnar wrote:
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Thu, 17 Jul 2008, V.Radhakrishnan wrote:
> > > 
> > > The above #ifdef must be actually #ifndef and not #ifdef The bug 
> > > does not allow a valid user (root) from accessing /dev/mem even 
> > > though the CONFIG_PROMISC_DEVMEM is NOT selected.
> > 
> > The real bug is that we shouldn't have "double negatives", and 
> > certainly not negative config options. Making that "promiscuous 
> > /dev/mem" option a negated thing as a config option was bad.
> > 
> > Ingo, over to you..
> 
> (hm, i dont find the original thread anywhere.)
> 
> this change:
> 
> > > --- arch/x86/mm/pat.c.orig	2008-07-17 22:04:18.000000000 +0530
> > > +++ arch/x86/mm/pat.c	2008-07-17 22:43:39.000000000 +0530
> > > @@ -471,7 +475,7 @@ pgprot_t phys_mem_access_prot(struct fil
> > >  	return vma_prot;
> > >  }
> > >  
> > > -#ifdef CONFIG_NONPROMISC_DEVMEM
> > > +#ifndef CONFIG_NONPROMISC_DEVMEM
> > >  /* This check is done in drivers/char/mem.c in case of NONPROMISC_DEVMEM*/
> > >  static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> > >  {
> 
> ... would add a crasher cache aliasing bug in case of 
> CONFIG_NONPROMISC_DEVMEM=n (or CONFIG_PROMISC_DEVMEM).
> 
> The idea behind this check is to add a second layer of checks to mmap()s 
> - which is absolutely necessary in the case of PAT and not optional. If 
> non-promisc /dev/mem is used then this check is not needed. (because the 
> higher level API already restricts us)
> 
> But this duplication is ugly and confusing - we should have just a 
> single check function, defined once and unconditionally, and utilized by 
> both places (with the devmem check being optional).
> 
> Venki, Arjan, agreed?
> 
> Also, Linus's observation about the illogical naming of the config 
> symbol is spot on as well, below is the rename commit i've just added to 
> tip/x86/urgent.
> 
> 	Ingo
> 
> ------------------->
> commit 64d206d896ff70b828138577d5ff39deda5f1c4d
> Author: Ingo Molnar <mingo@elte.hu>
> Date:   Fri Jul 18 00:26:59 2008 +0200
> 
>     x86: rename CONFIG_NONPROMISC_DEVMEM to CONFIG_PROMISC_DEVMEM
>     
>     Linus observed:
>     
>     > The real bug is that we shouldn't have "double negatives", and
>     > certainly not negative config options. Making that "promiscuous
>     > /dev/mem" option a negated thing as a config option was bad.
>     
>     right ... lets rename this option. There should never be a negation
>     in config options.
>     
>     [ that reminds me of CONFIG_SCHED_NO_NO_OMIT_FRAME_POINTER, but that
>       is for another commit ;-) ]
>     
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/Kconfig.debug |    7 ++++---
>  arch/x86/mm/pat.c      |    6 +++---
>  drivers/char/mem.c     |    2 +-
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index ae36bfa..f0cf5d9 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -5,10 +5,11 @@ config TRACE_IRQFLAGS_SUPPORT
>  
>  source "lib/Kconfig.debug"
>  
> -config NONPROMISC_DEVMEM
> -	bool "Filter access to /dev/mem"
> +config PROMISC_DEVMEM
> +	bool "Allow unlimited access to /dev/mem"
> +	default y
>  	help
> -	  If this option is left off, you allow userspace access to all
> +	  If this option is left on, you allow userspace (root) access to all
>  	  of memory, including kernel and userspace memory. Accidental
>  	  access to this is obviously disastrous, but specific access can
>  	  be used by people debugging the kernel.
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index d458507..c34dc48 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -373,8 +373,8 @@ pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>  	return vma_prot;
>  }
>  
> -#ifdef CONFIG_NONPROMISC_DEVMEM
> -/* This check is done in drivers/char/mem.c in case of NONPROMISC_DEVMEM*/
> +#ifndef CONFIG_PROMISC_DEVMEM
> +/* This check is done in drivers/char/mem.c in case of !PROMISC_DEVMEM*/
>  static inline int range_is_allowed(unsigned long pfn, unsigned long size)
>  {
>  	return 1;
> @@ -398,7 +398,7 @@ static inline int range_is_allowed(unsigned long pfn, unsigned long size)
>  	}
>  	return 1;
>  }
> -#endif /* CONFIG_NONPROMISC_DEVMEM */
> +#endif /* CONFIG_PROMISC_DEVMEM */
>  
>  int phys_mem_access_prot_allowed(struct file *file, unsigned long pfn,
>  				unsigned long size, pgprot_t *vma_prot)
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 070e22e..de05775 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -80,7 +80,7 @@ static inline int valid_mmap_phys_addr_range(unsigned long pfn, size_t size)
>  }
>  #endif
>  
> -#ifdef CONFIG_NONPROMISC_DEVMEM
> +#ifndef CONFIG_PROMISC_DEVMEM
>  static inline int range_is_allowed(unsigned long pfn, unsigned long size)
>  {
>  	u64 from = ((u64)pfn) << PAGE_SHIFT;