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

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]

FROM: V.Radhakrishnan <rk@atr-labs.com>

Description:

This is a 1-line bugfix !

The program linux-2.6.26/arch/x86/mm/pat.c has the following code

#ifdef 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)
{
	return 1;
}
#else { ... }

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.

Actual patch for fix follows ...

------

--- 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>

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

[Linus Torvalds]

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..

		Linus

> --- 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)
>  {
> @@ -586,4 +590,3 @@ void unmap_devmem(unsigned long pfn, uns

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

[Ingo Molnar]

* 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;

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]

> 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;

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

[Arjan van de Ven]

On Fri, 18 Jul 2008 07:34:17 +0530
"V.Radhakrishnan" <rk@atr-labs.com> wrote:
> > 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 !!

Hi,

it's great to see new people contribute to the kernel, however your
patch is not correct. In the PAT case, we really cannot allow /dev/mem
mmap access of kernel-used memory. It would create an incorrect cache
alias... which can have really bad effects, including tripple faulting
the cpu (which is an instant reboot) or data corruption, depending on
which model/vendor CPU you have.

What your patch does is to remove the code that prevents this situation
from happening... and that makes it not a very good idea.

Now, /dev/mem mmap access of address space that is not used by the
kernel is still allowed by the code (for example the PCI mmio region)...

Can you describe what you were trying to achieve by mmaping
of /dev/mem ? It sounds like you have a bug, since it's certainly
curious that you would want to deal with kernel memory this way.
(for example until recently, you wouldn't be able to do this at all)

Greetings,
   Arjan van de Ven


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

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]

Hi,

Thanks !

I was teaching a class of 17 students from HP, basics of kernel
debugging. I had already burnt CDs for them with 2.6.25, and I thought I
would try out the latest 2.6.26 ( announced just a day before the start
of the training program ) last week.

I wanted to cover basic concepts of memory management, and used
the /dev/mem purely as a means to show them what the physical memory
contains, how you can use this mechanism to view the contents of RAM
after a DMA to verify data patterns etc.

It so happened that all my 17 students had working code with 2.6.25 and
I, the instructor, had the kernel 2.6.26 crash in front of them !

So I started tracing the call and found the 'bug' and 'fixed it'.

I agree with you 100% that my patch does not solve the problem, but is a
quick fix approach to the next release. The PAT related access issue is
valid. However, I humbly submit that memory access is a policy issue and
perhaps should not be tinkered with thru code. 

If you provide the root, access to /dev/mem, I don't think it should be
a problem, since in any case, all others are denied access. Also,
theoretically, if you access RAM for Read Only, why should there be a
triple fault ? Again, if you access RAM, modify content, and mark those
pages as dirty ( as described in LDD ), isnt that supposed to take care
of cache issue ? Again, I re-iterate, I am NOT suggesting writing
directly to RAM !, But what about Read Access which which is what
several simple debuggers do ? The crash utility from RedHat also reads
memory (both user as well as kernel virtual addresses as input ). Is
reading RAM directly using a physical address deemed 'bad' ?

Thanks, anyway, and really happy that you have taken the time to answer
these issues.

regards

V. Radhakrishnan (alias 'RK')



On Sat, 2008-07-19 at 15:05 -0700, Arjan van de Ven wrote:
> On Fri, 18 Jul 2008 07:34:17 +0530
> "V.Radhakrishnan" <rk@atr-labs.com> wrote:
> > > 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 !!
> 
> Hi,
> 
> it's great to see new people contribute to the kernel, however your
> patch is not correct. In the PAT case, we really cannot allow /dev/mem
> mmap access of kernel-used memory. It would create an incorrect cache
> alias... which can have really bad effects, including tripple faulting
> the cpu (which is an instant reboot) or data corruption, depending on
> which model/vendor CPU you have.
> 
> What your patch does is to remove the code that prevents this situation
> from happening... and that makes it not a very good idea.
> 
> Now, /dev/mem mmap access of address space that is not used by the
> kernel is still allowed by the code (for example the PCI mmio region)...
> 
> Can you describe what you were trying to achieve by mmaping
> of /dev/mem ? It sounds like you have a bug, since it's certainly
> curious that you would want to deal with kernel memory this way.
> (for example until recently, you wouldn't be able to do this at all)
> 
> Greetings,
>    Arjan van de Ven
> 
> 

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

[Arjan van de Ven]

On Sun, 20 Jul 2008 21:20:46 +0530
"V.Radhakrishnan" <rk@atr-labs.com> wrote:
> 
> It so happened that all my 17 students had working code with 2.6.25
> and I, the instructor, had the kernel 2.6.26 crash in front of them !
> 
> So I started tracing the call and found the 'bug' and 'fixed it'.

to get the old behavior, just disable PAT..

> 
> I agree with you 100% that my patch does not solve the problem, but
> is a quick fix approach to the next release. The PAT related access
> issue is valid. However, I humbly submit that memory access is a
> policy issue and perhaps should not be tinkered with thru code. 

it's not policy, it's correctness. We don't allow root to open a file
on a cdrom for write and then write to it.. even though denying root
would equally be policy, right? (it's not)

> 
> If you provide the root, access to /dev/mem, I don't think it should
> be a problem, since in any case, all others are denied access.

this assertion is not correct; the device driver, the pagecache etc etc
will still use the page for whatever they were using it for.
> Also,
> theoretically, if you access RAM for Read Only, why should there be a
> triple fault ? 

yes.
If you access it as cachable from /dev/mem, but the actual owner of the
memory was using it uncached, that can tripple fault the cpu (again
depending on model etc). If you access it uncachable from /dev/mem, but
the owner was using it cached... likewise. (and on some systems, the CPU
is fine but the chipset/memory controller are not)

On x86 you're not supposed to mix cached and uncached. And PAT enforces
this for all cases where there's an owner.. but for /dev/mem there's no
such thing.


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

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

[Arjan van de Ven]

On Thu, 17 Jul 2008 10:27:03 -0700 (PDT)
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..
> 

patch below clears the double-negative and adds the "PAT also causes
this" explenation



From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH] x86: rename CONFIG_NONPROMISC_DEVMEM to not be a double negative

CONFIG_NONPROMISC_DEVMEM is a rather confusing name; this patch
renames it to CONFIG_STRICT_DEVMEM.

(the polarity of the option is still the same; it needs to be for now
to not break architectures that don't have the infastructure yet to support
this feature)

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 arch/x86/Kconfig.debug            |    6 ++++--
 arch/x86/configs/i386_defconfig   |    2 +-
 arch/x86/configs/x86_64_defconfig |    2 +-
 arch/x86/mm/pat.c                 |    6 +++---
 drivers/char/mem.c                |    2 +-
 5 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index ae36bfa..8d6b649 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -5,13 +5,15 @@ config TRACE_IRQFLAGS_SUPPORT
 
 source "lib/Kconfig.debug"
 
-config NONPROMISC_DEVMEM
+config STRICT_DEVMEM
 	bool "Filter access to /dev/mem"
 	help
 	  If this option is left off, you allow userspace 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.
+	  be used by people debugging the kernel. Note that with PAT support
+	  enabled, even in this case there are restrictions on /dev/mem
+	  use due to the cache aliasing requirements.
 
 	  If this option is switched on, the /dev/mem file only allows
 	  userspace access to PCI space and the BIOS code and data regions.
diff --git a/arch/x86/configs/i386_defconfig b/arch/x86/configs/i386_defconfig
index 9bc34e2..4d73f53 100644
--- a/arch/x86/configs/i386_defconfig
+++ b/arch/x86/configs/i386_defconfig
@@ -2047,7 +2047,7 @@ CONFIG_PROVIDE_OHCI1394_DMA_INIT=y
 # CONFIG_SAMPLES is not set
 # CONFIG_KGDB is not set
 CONFIG_HAVE_ARCH_KGDB=y
-# CONFIG_NONPROMISC_DEVMEM is not set
+# CONFIG_STRICT_DEVMEM is not set
 CONFIG_EARLY_PRINTK=y
 CONFIG_DEBUG_STACKOVERFLOW=y
 CONFIG_DEBUG_STACK_USAGE=y
diff --git a/arch/x86/configs/x86_64_defconfig b/arch/x86/configs/x86_64_defconfig
index ae5124e..a404524 100644
--- a/arch/x86/configs/x86_64_defconfig
+++ b/arch/x86/configs/x86_64_defconfig
@@ -2012,7 +2012,7 @@ CONFIG_PROVIDE_OHCI1394_DMA_INIT=y
 # CONFIG_SAMPLES is not set
 # CONFIG_KGDB is not set
 CONFIG_HAVE_ARCH_KGDB=y
-# CONFIG_NONPROMISC_DEVMEM is not set
+# CONFIG_STRICT_DEVMEM is not set
 CONFIG_EARLY_PRINTK=y
 CONFIG_DEBUG_STACKOVERFLOW=y
 CONFIG_DEBUG_STACK_USAGE=y
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index d458507..6bb597f 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*/
+#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;
@@ -398,7 +398,7 @@ static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 	}
 	return 1;
 }
-#endif /* CONFIG_NONPROMISC_DEVMEM */
+#endif /* CONFIG_STRICT_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..b6772d6 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
+#ifdef CONFIG_STRICT_DEVMEM
 static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 {
 	u64 from = ((u64)pfn) << PAGE_SHIFT;
-- 
1.5.5.1



-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

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

[Ingo Molnar]

* Arjan van de Ven <arjan@infradead.org> 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..
> 
> patch below clears the double-negative and adds the "PAT also causes 
> this" explenation

applied to tip/x86/urgent - i made this a delta commit (see below) 
because some other patches were picked up into x86/urgent meanwhile. 
Thanks Arjan,

	Ingo

--------------------->
commit d092633bff3b19faffc480fe9810805e7792a029
Author: Ingo Molnar <mingo@elte.hu>
Date:   Fri Jul 18 00:26:59 2008 +0200

    Subject: devmem, x86: fix rename of CONFIG_NONPROMISC_DEVMEM
    From: Arjan van de Ven <arjan@infradead.org>
    Date: Sat, 19 Jul 2008 15:47:17 -0700
    
    CONFIG_NONPROMISC_DEVMEM was a rather confusing name - but renaming it
    to CONFIG_PROMISC_DEVMEM causes problems on architectures that do not
    support this feature; this patch renames it to CONFIG_STRICT_DEVMEM,
    so that architectures can opt-in into it.
    
    ( the polarity of the option is still the same as it was originally; it
      needs to be for now to not break architectures that don't have the
      infastructure yet to support this feature)
    
    Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
    Cc: "V.Radhakrishnan" <rk@atr-labs.com>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>
    ---
---
 arch/x86/Kconfig.debug            |    9 +++++----
 arch/x86/configs/i386_defconfig   |    2 +-
 arch/x86/configs/x86_64_defconfig |    2 +-
 arch/x86/mm/pat.c                 |    6 +++---
 drivers/char/mem.c                |    2 +-
 5 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index f0cf5d9..51c8214 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -5,14 +5,15 @@ config TRACE_IRQFLAGS_SUPPORT
 
 source "lib/Kconfig.debug"
 
-config PROMISC_DEVMEM
-	bool "Allow unlimited access to /dev/mem"
-	default y
+config STRICT_DEVMEM
+	bool "Filter access to /dev/mem"
 	help
 	  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.
+	  be used by people debugging the kernel. Note that with PAT support
+	  enabled, even in this case there are restrictions on /dev/mem
+	  use due to the cache aliasing requirements.
 
 	  If this option is switched on, the /dev/mem file only allows
 	  userspace access to PCI space and the BIOS code and data regions.
diff --git a/arch/x86/configs/i386_defconfig b/arch/x86/configs/i386_defconfig
index 9bc34e2..4d73f53 100644
--- a/arch/x86/configs/i386_defconfig
+++ b/arch/x86/configs/i386_defconfig
@@ -2047,7 +2047,7 @@ CONFIG_PROVIDE_OHCI1394_DMA_INIT=y
 # CONFIG_SAMPLES is not set
 # CONFIG_KGDB is not set
 CONFIG_HAVE_ARCH_KGDB=y
-# CONFIG_NONPROMISC_DEVMEM is not set
+# CONFIG_STRICT_DEVMEM is not set
 CONFIG_EARLY_PRINTK=y
 CONFIG_DEBUG_STACKOVERFLOW=y
 CONFIG_DEBUG_STACK_USAGE=y
diff --git a/arch/x86/configs/x86_64_defconfig b/arch/x86/configs/x86_64_defconfig
index ae5124e..a404524 100644
--- a/arch/x86/configs/x86_64_defconfig
+++ b/arch/x86/configs/x86_64_defconfig
@@ -2012,7 +2012,7 @@ CONFIG_PROVIDE_OHCI1394_DMA_INIT=y
 # CONFIG_SAMPLES is not set
 # CONFIG_KGDB is not set
 CONFIG_HAVE_ARCH_KGDB=y
-# CONFIG_NONPROMISC_DEVMEM is not set
+# CONFIG_STRICT_DEVMEM is not set
 CONFIG_EARLY_PRINTK=y
 CONFIG_DEBUG_STACKOVERFLOW=y
 CONFIG_DEBUG_STACK_USAGE=y
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index c34dc48..6bb597f 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;
 }
 
-#ifndef CONFIG_PROMISC_DEVMEM
-/* This check is done in drivers/char/mem.c in case of !PROMISC_DEVMEM*/
+#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;
@@ -398,7 +398,7 @@ static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 	}
 	return 1;
 }
-#endif /* CONFIG_PROMISC_DEVMEM */
+#endif /* CONFIG_STRICT_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 de05775..b6772d6 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
 
-#ifndef CONFIG_PROMISC_DEVMEM
+#ifdef CONFIG_STRICT_DEVMEM
 static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 {
 	u64 from = ((u64)pfn) << PAGE_SHIFT;