[PATCH] powerpc/mm: add devmem_is_allowed() for STRICT_DEVMEM checking

[PATCH] powerpc/mm: add devmem_is_allowed() for STRICT_DEVMEM checking

[Steve Best]

    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;
+}
+
 #ifdef CONFIG_PPC_SMLPAR
 void arch_free_page(struct page *page, int order);
 #define HAVE_ARCH_FREE_PAGE

Re: [PATCH] powerpc/mm: add devmem_is_allowed() for STRICT_DEVMEM checking

[Josh Boyer]

On Mon, Jan 31, 2011 at 2:16 PM, Steve Best <sfbest@us.ibm.com> wrote:
> =A0 =A0Provide devmem_is_allowed() routine to restrict access to kernel
> =A0 =A0memory from userspace.
> =A0 =A0Set 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
> =A0 =A0 =A0 =A0 =A0platform probing is done, all platforms selected must
> =A0 =A0 =A0 =A0 =A0share the same address.
>
> +config STRICT_DEVMEM
> + =A0 =A0 =A0 =A0def_bool y
> + =A0 =A0 =A0 =A0prompt "Filter access to /dev/mem"
> + =A0 =A0 =A0 =A0---help---
> + =A0 =A0 =A0 =A0 =A0This option restricts access to /dev/mem. =A0If this=
 option is
> + =A0 =A0 =A0 =A0 =A0disabled, you allow userspace access to all memory, =
including
> + =A0 =A0 =A0 =A0 =A0kernel and userspace memory. Accidental memory acces=
s is likely
> + =A0 =A0 =A0 =A0 =A0to be disastrous.
> + =A0 =A0 =A0 =A0 =A0Memory access is required for experts who want to de=
bug the kernel.
> +
> + =A0 =A0 =A0 =A0 =A0If you are unsure, say Y.
> +
> =A0endmenu
> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/p=
age.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, uns=
igned long vaddr,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct page *p);
> =A0extern int page_is_ram(unsigned long pfn);
>
> +static inline int devmem_is_allowed(unsigned long pfn)
> +{
> + =A0 =A0 =A0 =A0return 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

Re: [PATCH] powerpc/mm: add devmem_is_allowed() for STRICT_DEVMEM checking

[Josh Boyer]

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:
>> =A0 =A0Provide devmem_is_allowed() routine to restrict access to kernel
>> =A0 =A0memory from userspace.
>> =A0 =A0Set 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
>> =A0 =A0 =A0 =A0 =A0platform probing is done, all platforms selected must
>> =A0 =A0 =A0 =A0 =A0share the same address.
>>
>> +config STRICT_DEVMEM
>> + =A0 =A0 =A0 =A0def_bool y
>> + =A0 =A0 =A0 =A0prompt "Filter access to /dev/mem"
>> + =A0 =A0 =A0 =A0---help---
>> + =A0 =A0 =A0 =A0 =A0This option restricts access to /dev/mem. =A0If thi=
s option is
>> + =A0 =A0 =A0 =A0 =A0disabled, you allow userspace access to all memory,=
 including
>> + =A0 =A0 =A0 =A0 =A0kernel and userspace memory. Accidental memory acce=
ss is likely
>> + =A0 =A0 =A0 =A0 =A0to be disastrous.
>> + =A0 =A0 =A0 =A0 =A0Memory access is required for experts who want to d=
ebug the kernel.
>> +
>> + =A0 =A0 =A0 =A0 =A0If you are unsure, say Y.
>> +
>> =A0endmenu
>> 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, un=
signed long vaddr,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct page *p);
>> =A0extern int page_is_ram(unsigned long pfn);
>>
>> +static inline int devmem_is_allowed(unsigned long pfn)
>> +{
>> + =A0 =A0 =A0 =A0return 0;
>> +}
>> +
>
> Er, should this be toggled via CONFIG_STRICT_DEVMEM somehow? =A0Or 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

Re: [PATCH] powerpc/mm: add devmem_is_allowed() for STRICT_DEVMEM checking

[Scott Wood]

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

Re: [PATCH] powerpc/mm: add devmem_is_allowed() for STRICT_DEVMEM checking

[Steve Best]

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

Re: [PATCH] powerpc/mm: add devmem_is_allowed() for STRICT_DEVMEM checking

[Scott Wood]

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] powerpc/mm: add devmem_is_allowed() for STRICT_DEVMEM checking

[Steve Best]

       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 e72dcf6..e1aab6b 100644
--- a/arch/powerpc/Kconfig.debug
+++ b/arch/powerpc/Kconfig.debug
@@ -283,4 +283,15 @@ 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. 
+          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 2cd664e..dc2ec96 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -261,6 +261,7 @@ extern void clear_user_page(void *page, unsigned long vaddr, struct page *pg);
 extern void copy_user_page(void *to, void *from, unsigned long vaddr,
 		struct page *p);
 extern int page_is_ram(unsigned long pfn);
+extern int devmem_is_allowed(unsigned long pfn);
 
 #ifdef CONFIG_PPC_SMLPAR
 void arch_free_page(struct page *page, int order);
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index 29d4dde..b1a6233 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -520,3 +520,21 @@ void update_mmu_cache(struct vm_area_struct *vma, unsigned long address,
 	hash_preload(vma->vm_mm, address, access, trap);
 #endif /* CONFIG_PPC_STD_MMU */
 }
+
+/*
+ * devmem_is_allowed() checks to see if /dev/mem access to a certain address
+ * is valid. The argument is a physical page number.
+ *
+ * On PowerPC, access has to be given to data regions used by X. We have to
+ * disallow access to device-exclusive MMIO regions and system RAM. 
+ */
+int devmem_is_allowed(unsigned long pfn)
+{
+        if ((pfn >= 57360 || pfn <= 57392))
+                return 1;
+        if (iomem_is_exclusive(pfn << PAGE_SHIFT))
+                return 0;
+        if (!page_is_ram(pfn))
+                return 1;
+        return 0;
+}

Re: [PATCH] powerpc/mm: add devmem_is_allowed() for STRICT_DEVMEM checking

[Nathan Lynch]

Hi Steve,

On Tue, 2011-06-14 at 12:58 -0400, Steve Best wrote:
> diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> index e72dcf6..e1aab6b 100644
> --- a/arch/powerpc/Kconfig.debug
> +++ b/arch/powerpc/Kconfig.debug
> @@ -283,4 +283,15 @@ config PPC_EARLY_DEBUG_CPM_ADDR
>  	  platform probing is done, all platforms selected must
>  	  share the same address.
>  
> +config STRICT_DEVMEM
> +        def_bool y

Default new config items to n, please.


> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -520,3 +520,21 @@ void update_mmu_cache(struct vm_area_struct *vma, unsigned long address,
>  	hash_preload(vma->vm_mm, address, access, trap);
>  #endif /* CONFIG_PPC_STD_MMU */
>  }
> +
> +/*
> + * devmem_is_allowed() checks to see if /dev/mem access to a certain address
> + * is valid. The argument is a physical page number.
> + *
> + * On PowerPC, access has to be given to data regions used by X. We have to
> + * disallow access to device-exclusive MMIO regions and system RAM. 
> + */
> +int devmem_is_allowed(unsigned long pfn)
> +{
> +        if ((pfn >= 57360 || pfn <= 57392))
> +                return 1;

That seems... fragile.  Where do these numbers come from, and are they
appropriate for all platforms and configurations?

Re: [PATCH] powerpc/mm: add devmem_is_allowed() for STRICT_DEVMEM checking

[Steve Best]

On Tue, 2011-06-14 at 12:30 -0500, Nathan Lynch wrote:
> Hi Steve,
> 
> On Tue, 2011-06-14 at 12:58 -0400, Steve Best wrote:
> > diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> > index e72dcf6..e1aab6b 100644
> > --- a/arch/powerpc/Kconfig.debug
> > +++ b/arch/powerpc/Kconfig.debug
> > @@ -283,4 +283,15 @@ config PPC_EARLY_DEBUG_CPM_ADDR
> >  	  platform probing is done, all platforms selected must
> >  	  share the same address.
> >  
> > +config STRICT_DEVMEM
> > +        def_bool y
> 
> Default new config items to n, please.

ok
> 
> 
> > --- a/arch/powerpc/mm/mem.c
> > +++ b/arch/powerpc/mm/mem.c
> > @@ -520,3 +520,21 @@ void update_mmu_cache(struct vm_area_struct *vma, unsigned long address,
> >  	hash_preload(vma->vm_mm, address, access, trap);
> >  #endif /* CONFIG_PPC_STD_MMU */
> >  }
> > +
> > +/*
> > + * devmem_is_allowed() checks to see if /dev/mem access to a certain address
> > + * is valid. The argument is a physical page number.
> > + *
> > + * On PowerPC, access has to be given to data regions used by X. We have to
> > + * disallow access to device-exclusive MMIO regions and system RAM. 
> > + */
> > +int devmem_is_allowed(unsigned long pfn)
> > +{
> > +        if ((pfn >= 57360 || pfn <= 57392))
> > +                return 1;
> 
> That seems... fragile.  Where do these numbers come from, and are they
> appropriate for all platforms and configurations?

This is the range I got from testing pseries blades and servers. maybe
there is a better way to get this range anyone know of a way?
> 
> 
-Steve

Re: [PATCH] powerpc/mm: add devmem_is_allowed() for STRICT_DEVMEM checking

[Scott Wood]

On Tue, 14 Jun 2011 14:17:01 -0400
Steve Best <sfbest@us.ibm.com> wrote:

> On Tue, 2011-06-14 at 12:30 -0500, Nathan Lynch wrote:
> > Hi Steve,
> > 
> > On Tue, 2011-06-14 at 12:58 -0400, Steve Best wrote:
> > > +/*
> > > + * devmem_is_allowed() checks to see if /dev/mem access to a certain address
> > > + * is valid. The argument is a physical page number.
> > > + *
> > > + * On PowerPC, access has to be given to data regions used by X. We have to
> > > + * disallow access to device-exclusive MMIO regions and system RAM. 
> > > + */
> > > +int devmem_is_allowed(unsigned long pfn)
> > > +{
> > > +        if ((pfn >= 57360 || pfn <= 57392))
> > > +                return 1;
> > 
> > That seems... fragile.  Where do these numbers come from, and are they
> > appropriate for all platforms and configurations?
> 
> This is the range I got from testing pseries blades and servers. maybe
> there is a better way to get this range anyone know of a way?

Use iomem_is_exclusive(), as other architectures (e.g. x86, arm) do.

Anything else is both platform-specific, and inappropriate hardcoding of
policy.

-Scott

Re: [PATCH] powerpc/mm: add devmem_is_allowed() for STRICT_DEVMEM checking

[Sukadev Bhattiprolu]

On 06/14/2011 12:04 PM, Scott Wood wrote:
> On Tue, 14 Jun 2011 14:17:01 -0400
> Steve Best<sfbest@us.ibm.com>  wrote:
>
>> On Tue, 2011-06-14 at 12:30 -0500, Nathan Lynch wrote:
>>> Hi Steve,
>>>
>>> On Tue, 2011-06-14 at 12:58 -0400, Steve Best wrote:
>>>> +/*
>>>> + * devmem_is_allowed() checks to see if /dev/mem access to a certain address
>>>> + * is valid. The argument is a physical page number.
>>>> + *
>>>> + * On PowerPC, access has to be given to data regions used by X. We have to
>>>> + * disallow access to device-exclusive MMIO regions and system RAM.
>>>> + */
>>>> +int devmem_is_allowed(unsigned long pfn)
>>>> +{
>>>> +        if ((pfn>= 57360 || pfn<= 57392))
>>>> +                return 1;
>>>
>>> That seems... fragile.  Where do these numbers come from, and are they
>>> appropriate for all platforms and configurations?
>>
>> This is the range I got from testing pseries blades and servers. maybe
>> there is a better way to get this range anyone know of a way?
>
> Use iomem_is_exclusive(), as other architectures (e.g. x86, arm) do.
>
> Anything else is both platform-specific, and inappropriate hardcoding of
> policy.

x86 allows access to the first 256 pages. Are there other regions that 
we should allow in power besides the !iomem_is_exclusive() region ?

>
> -Scott