Patchwork powerpc/mm: add devmem_is_allowed() for STRICT_DEVMEM checking

login
register
mail settings
Submitter Steve Best
Date Jan. 31, 2011, 7:16 p.m.
Message ID <20110131191109.5727.87742.sendpatchset@squad5-lp1.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/81216/
State Rejected
Headers show

Comments

Steve Best - Jan. 31, 2011, 7:16 p.m.
Provide devmem_is_allowed() routine to restrict access to kernel
    memory from userspace.
    Set CONFIG_STRICT_DEVMEM config option to switch on checking.

Signed-off-by: Steve Best <sfbest@us.ibm.com>
Josh Boyer - Jan. 31, 2011, 7:25 p.m.
On Mon, Jan 31, 2011 at 2:16 PM, Steve Best <sfbest@us.ibm.com> wrote:
>    Provide devmem_is_allowed() routine to restrict access to kernel
>    memory from userspace.
>    Set CONFIG_STRICT_DEVMEM config option to switch on checking.
>
> Signed-off-by: Steve Best <sfbest@us.ibm.com>
>
> diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> index 2d38a50..6805d5d 100644
> --- a/arch/powerpc/Kconfig.debug
> +++ b/arch/powerpc/Kconfig.debug
> @@ -299,4 +299,16 @@ config PPC_EARLY_DEBUG_CPM_ADDR
>          platform probing is done, all platforms selected must
>          share the same address.
>
> +config STRICT_DEVMEM
> +        def_bool y
> +        prompt "Filter access to /dev/mem"
> +        ---help---
> +          This option restricts access to /dev/mem.  If this option is
> +          disabled, you allow userspace access to all memory, including
> +          kernel and userspace memory. Accidental memory access is likely
> +          to be disastrous.
> +          Memory access is required for experts who want to debug the kernel.
> +
> +          If you are unsure, say Y.
> +
>  endmenu
> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
> index 53b64be..f225032 100644
> --- a/arch/powerpc/include/asm/page.h
> +++ b/arch/powerpc/include/asm/page.h
> @@ -262,6 +262,11 @@ extern void copy_user_page(void *to, void *from, unsigned long vaddr,
>                struct page *p);
>  extern int page_is_ram(unsigned long pfn);
>
> +static inline int devmem_is_allowed(unsigned long pfn)
> +{
> +        return 0;
> +}
> +

Er, should this be toggled via CONFIG_STRICT_DEVMEM somehow?  Or I
guess I'm missing why the config option had to be added if not.

josh
Josh Boyer - Jan. 31, 2011, 7:32 p.m.
On Mon, Jan 31, 2011 at 2:25 PM, Josh Boyer <jwboyer@gmail.com> wrote:
> On Mon, Jan 31, 2011 at 2:16 PM, Steve Best <sfbest@us.ibm.com> wrote:
>>    Provide devmem_is_allowed() routine to restrict access to kernel
>>    memory from userspace.
>>    Set CONFIG_STRICT_DEVMEM config option to switch on checking.
>>
>> Signed-off-by: Steve Best <sfbest@us.ibm.com>
>>
>> diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
>> index 2d38a50..6805d5d 100644
>> --- a/arch/powerpc/Kconfig.debug
>> +++ b/arch/powerpc/Kconfig.debug
>> @@ -299,4 +299,16 @@ config PPC_EARLY_DEBUG_CPM_ADDR
>>          platform probing is done, all platforms selected must
>>          share the same address.
>>
>> +config STRICT_DEVMEM
>> +        def_bool y
>> +        prompt "Filter access to /dev/mem"
>> +        ---help---
>> +          This option restricts access to /dev/mem.  If this option is
>> +          disabled, you allow userspace access to all memory, including
>> +          kernel and userspace memory. Accidental memory access is likely
>> +          to be disastrous.
>> +          Memory access is required for experts who want to debug the kernel.
>> +
>> +          If you are unsure, say Y.
>> +
>>  endmenu
>> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
>> index 53b64be..f225032 100644
>> --- a/arch/powerpc/include/asm/page.h
>> +++ b/arch/powerpc/include/asm/page.h
>> @@ -262,6 +262,11 @@ extern void copy_user_page(void *to, void *from, unsigned long vaddr,
>>                struct page *p);
>>  extern int page_is_ram(unsigned long pfn);
>>
>> +static inline int devmem_is_allowed(unsigned long pfn)
>> +{
>> +        return 0;
>> +}
>> +
>
> Er, should this be toggled via CONFIG_STRICT_DEVMEM somehow?  Or I
> guess I'm missing why the config option had to be added if not.

Nevermind.  I see that it's done in the drivers/char/mem.c file.
Should have looked more first.

josh
Scott Wood - Jan. 31, 2011, 7:40 p.m.
On Mon, 31 Jan 2011 14:16:00 -0500
Steve Best <sfbest@us.ibm.com> wrote:

>     Provide devmem_is_allowed() routine to restrict access to kernel
>     memory from userspace.
>     Set CONFIG_STRICT_DEVMEM config option to switch on checking.
> 
> Signed-off-by: Steve Best <sfbest@us.ibm.com>
> 
> diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> index 2d38a50..6805d5d 100644
> --- a/arch/powerpc/Kconfig.debug
> +++ b/arch/powerpc/Kconfig.debug
> @@ -299,4 +299,16 @@ config PPC_EARLY_DEBUG_CPM_ADDR
>  	  platform probing is done, all platforms selected must
>  	  share the same address.
>  
> +config STRICT_DEVMEM
> +        def_bool y
> +        prompt "Filter access to /dev/mem"
> +        ---help---
> +          This option restricts access to /dev/mem.  If this option is
> +          disabled, you allow userspace access to all memory, including
> +          kernel and userspace memory. Accidental memory access is likely
> +          to be disastrous.
> +          Memory access is required for experts who want to debug the kernel.
> +
> +          If you are unsure, say Y.
> +
>  endmenu
> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
> index 53b64be..f225032 100644
> --- a/arch/powerpc/include/asm/page.h
> +++ b/arch/powerpc/include/asm/page.h
> @@ -262,6 +262,11 @@ extern void copy_user_page(void *to, void *from, unsigned long vaddr,
>  		struct page *p);
>  extern int page_is_ram(unsigned long pfn);
>  
> +static inline int devmem_is_allowed(unsigned long pfn)
> +{
> +        return 0;
> +}
> +

I don't see how this is a sane thing to turn on by default (you're not
restricting it, BTW -- you're completely disabling it with that
implementation of devmem_is_allowed).  It will break anything that
uses /dev/mem to access I/O, possibly including desktoppy stuff like X
servers, as well as lots of stuff that goes on in embedded setups.

You need to be root to access /dev/mem, and root has plenty of
other options for causing "disastrous" results.  You don't just stumble
onto /dev/mem by accident.

-Scott
Steve Best - Feb. 1, 2011, 5:21 p.m.
On Mon, 2011-01-31 at 13:40 -0600, Scott Wood wrote:
> On Mon, 31 Jan 2011 14:16:00 -0500
> Steve Best <sfbest@us.ibm.com> wrote:
> 
> >     Provide devmem_is_allowed() routine to restrict access to kernel
> >     memory from userspace.
> >     Set CONFIG_STRICT_DEVMEM config option to switch on checking.
> > 
> > Signed-off-by: Steve Best <sfbest@us.ibm.com>
> > 
> > diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> > index 2d38a50..6805d5d 100644
> > --- a/arch/powerpc/Kconfig.debug
> > +++ b/arch/powerpc/Kconfig.debug
> > @@ -299,4 +299,16 @@ config PPC_EARLY_DEBUG_CPM_ADDR
> >  	  platform probing is done, all platforms selected must
> >  	  share the same address.
> >  
> > +config STRICT_DEVMEM
> > +        def_bool y
> > +        prompt "Filter access to /dev/mem"
> > +        ---help---
> > +          This option restricts access to /dev/mem.  If this option is
> > +          disabled, you allow userspace access to all memory, including
> > +          kernel and userspace memory. Accidental memory access is likely
> > +          to be disastrous.
> > +          Memory access is required for experts who want to debug the kernel.
> > +
> > +          If you are unsure, say Y.
> > +
> >  endmenu
> > diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
> > index 53b64be..f225032 100644
> > --- a/arch/powerpc/include/asm/page.h
> > +++ b/arch/powerpc/include/asm/page.h
> > @@ -262,6 +262,11 @@ extern void copy_user_page(void *to, void *from, unsigned long vaddr,
> >  		struct page *p);
> >  extern int page_is_ram(unsigned long pfn);
> >  
> > +static inline int devmem_is_allowed(unsigned long pfn)
> > +{
> > +        return 0;
> > +}
> > +
> 
> I don't see how this is a sane thing to turn on by default (you're not
> restricting it, BTW -- you're completely disabling it with that
> implementation of devmem_is_allowed).  It will break anything that
> uses /dev/mem to access I/O, 

could you expand on what I/O depends on /dev/mem, so I can take
that into account?

> possibly including desktoppy stuff like X
> servers, 

you are right just found out that X needs to access it. will 
take that into account
> as well as lots of stuff that goes on in embedded setups.

could you explain more about what needs access to /dev/mem in 
the embedded setups?

> 
> You need to be root to access /dev/mem, and root has plenty of
> other options for causing "disastrous" results.  You don't just stumble
> onto /dev/mem by accident.
> 
> -Scott

-Steve
Scott Wood - Feb. 1, 2011, 6:35 p.m.
On Tue, 1 Feb 2011 12:21:45 -0500
Steve Best <sfbest@us.ibm.com> wrote:

> 
> On Mon, 2011-01-31 at 13:40 -0600, Scott Wood wrote:
> > I don't see how this is a sane thing to turn on by default (you're not
> > restricting it, BTW -- you're completely disabling it with that
> > implementation of devmem_is_allowed).  It will break anything that
> > uses /dev/mem to access I/O, 
> 
> could you expand on what I/O depends on /dev/mem, so I can take
> that into account?

It could be anything.  You're shutting off, by default, a
longstanding userspace interface, that already has adequate security
protection.

Even x86 doesn't default it to yes (though it does say "if in doubt
say Y"), and when enabled x86 only restricts access to memory, not I/O.

> > possibly including desktoppy stuff like X
> > servers, 
> 
> you are right just found out that X needs to access it. will 
> take that into account
> > as well as lots of stuff that goes on in embedded setups.
> 
> could you explain more about what needs access to /dev/mem in 
> the embedded setups?

All sorts of custom stuff -- userspace drivers, special memory regions
reserved at boot, etc.

If you really want this, I suggest prohibiting access only when it's an
actual RAM page tracked by the kernel (maybe only when PageReserved
is unset as well?), or when iomem_is_exclusive returns true.

-Scott

Patch

diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
index 2d38a50..6805d5d 100644
--- a/arch/powerpc/Kconfig.debug
+++ b/arch/powerpc/Kconfig.debug
@@ -299,4 +299,16 @@  config PPC_EARLY_DEBUG_CPM_ADDR
 	  platform probing is done, all platforms selected must
 	  share the same address.
 
+config STRICT_DEVMEM
+        def_bool y
+        prompt "Filter access to /dev/mem"
+        ---help---
+          This option restricts access to /dev/mem.  If this option is
+          disabled, you allow userspace access to all memory, including
+          kernel and userspace memory. Accidental memory access is likely
+          to be disastrous.
+          Memory access is required for experts who want to debug the kernel.
+
+          If you are unsure, say Y.
+
 endmenu
diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index 53b64be..f225032 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -262,6 +262,11 @@  extern void copy_user_page(void *to, void *from, unsigned long vaddr,
 		struct page *p);
 extern int page_is_ram(unsigned long pfn);
 
+static inline int devmem_is_allowed(unsigned long pfn)
+{
+        return 0;
+}
+
 #ifdef CONFIG_PPC_SMLPAR
 void arch_free_page(struct page *page, int order);
 #define HAVE_ARCH_FREE_PAGE