FROM: V.Radhakrishnan <[email protected]>
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 <[email protected]>
*
* Loosely based on earlier PAT patchset from Eric Biederman and Andi
Kleen.
- */
+ *
+ * Bug fixed - V. Radhakrishnan <[email protected]> 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 <[email protected]>
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
* Linus Torvalds <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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;
> 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 <[email protected]>
*
* Loosely based on earlier PAT patchset from Eric Biederman and Andi
Kleen.
- */
+ *
+ * Bug fixed - V. Radhakrishnan <[email protected]> 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 <[email protected]>
---
On Fri, 2008-07-18 at 00:39 +0200, Ingo Molnar wrote:
> * Linus Torvalds <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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;
On Fri, 18 Jul 2008 07:34:17 +0530
"V.Radhakrishnan" <[email protected]> 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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Thu, 17 Jul 2008 10:27:03 -0700 (PDT)
Linus Torvalds <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
* Arjan van de Ven <[email protected]> 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 <[email protected]>
Date: Fri Jul 18 00:26:59 2008 +0200
Subject: devmem, x86: fix rename of CONFIG_NONPROMISC_DEVMEM
From: Arjan van de Ven <[email protected]>
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 <[email protected]>
Cc: "V.Radhakrishnan" <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
---
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;
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" <[email protected]> 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
>
>
On Sun, 20 Jul 2008 21:20:46 +0530
"V.Radhakrishnan" <[email protected]> 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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org