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

login
register
mail settings
Submitter Steve Best
Date June 14, 2011, 4:58 p.m.
Message ID <20110614165011.23034.66685.sendpatchset@squad5-lp1.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/100375/
State Changes Requested
Headers show

Comments

Steve Best - June 14, 2011, 4:58 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>
Nathan Lynch - June 14, 2011, 5:30 p.m.
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?
Steve Best - June 14, 2011, 6:17 p.m.
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
Scott Wood - June 14, 2011, 7:04 p.m.
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
sukadev@linux.vnet.ibm.com - June 29, 2011, 10:01 p.m.
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

Patch

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