Patchwork [1/2] sparc64 - strict devmem

login
register
mail settings
Submitter Bob Picco
Date Feb. 18, 2014, 7:11 p.m.
Message ID <1392750679-6966-1-git-send-email-bpicco@meloft.net>
Download mbox | patch
Permalink /patch/321624/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Bob Picco - Feb. 18, 2014, 7:11 p.m.
From: bob picco <bpicco@meloft.net>

This is just the first part of restricting /dev/mem access on sparc.
This patch does nothing to change the current behaviour. The patch is
in preparation for a subsequent patch.

Signed-off-by: Bob Picco <bob.picco@oracle.com>
---
 arch/sparc/Kconfig.debug         |    8 ++++++++
 arch/sparc/include/asm/page_64.h |    1 +
 arch/sparc/mm/init_64.c          |   11 +++++++++++
 3 files changed, 20 insertions(+), 0 deletions(-)
Sam Ravnborg - Feb. 18, 2014, 9:05 p.m.
Hi Bob.

On Tue, Feb 18, 2014 at 02:11:18PM -0500, Bob Picco wrote:
> From: bob picco <bpicco@meloft.net>
> 
> This is just the first part of restricting /dev/mem access on sparc.
> This patch does nothing to change the current behaviour. The patch is
> in preparation for a subsequent patch.
> 
> Signed-off-by: Bob Picco <bob.picco@oracle.com>
> ---
>  arch/sparc/Kconfig.debug         |    8 ++++++++
>  arch/sparc/include/asm/page_64.h |    1 +
>  arch/sparc/mm/init_64.c          |   11 +++++++++++
>  3 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/sparc/Kconfig.debug b/arch/sparc/Kconfig.debug
> index 6db35fb..92f2388 100644
> --- a/arch/sparc/Kconfig.debug
> +++ b/arch/sparc/Kconfig.debug
> @@ -6,6 +6,14 @@ config TRACE_IRQFLAGS_SUPPORT
>  
>  source "lib/Kconfig.debug"
>  
> +config STRICT_DEVMEM
> +	bool "Filter access to /dev/mem"
> +	---help---
> +	  If this option is disabled, 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.
> +

Grumble - this really belongs in an arch neutral file,
and then archs that support it should select HAVE_STRICT_DEVMEM


> diff --git a/arch/sparc/include/asm/page_64.h b/arch/sparc/include/asm/page_64.h
> index aac53fc..e54ee50 100644
> --- a/arch/sparc/include/asm/page_64.h
> +++ b/arch/sparc/include/asm/page_64.h
> @@ -36,6 +36,7 @@ extern void hugetlb_setup(struct pt_regs *regs);
>  
>  #define WANT_PAGE_VIRTUAL
>  
> +extern int devmem_is_allowed(unsigned long pagenr);
>  extern void _clear_page(void *page);
>  #define clear_page(X)	_clear_page((void *)(X))
>  struct page;
Grumble - and this prototype should not be required to be repeated by all archs either.

My grumbling are general comments - and you may ignore these...
But, see below.

> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
> index eafbc65..9c4a820 100644
> --- a/arch/sparc/mm/init_64.c
> +++ b/arch/sparc/mm/init_64.c
> @@ -2694,3 +2694,14 @@ void hugetlb_setup(struct pt_regs *regs)
>  	}
>  }
>  #endif
> +
> +#ifdef CONFIG_STRICT_DEVMEM
> +/* devmem_is_allowed for sparc.
> + */
> +int devmem_is_allowed(unsigned long pagenr)
> +{
> +	int  rc = page_is_ram(pagenr);
> +
> +	return rc;
> +}
> +#endif

Any specific reason why this is sparc64 specific?
In the Kconfig.debug file you allow this to be sleected also for sparc32.

And I see no reason to restrict this to sparc64 as this is anyway optional.

	Sam
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bob Picco - Feb. 18, 2014, 10:10 p.m.
Hi Sam,
Sam Ravnborg wrote:	[Tue Feb 18 2014, 04:05:15PM EST]
> Hi Bob.
> 
> On Tue, Feb 18, 2014 at 02:11:18PM -0500, Bob Picco wrote:
> > From: bob picco <bpicco@meloft.net>
> > 
> > This is just the first part of restricting /dev/mem access on sparc.
> > This patch does nothing to change the current behaviour. The patch is
> > in preparation for a subsequent patch.
> > 
> > Signed-off-by: Bob Picco <bob.picco@oracle.com>
> > ---
> >  arch/sparc/Kconfig.debug         |    8 ++++++++
> >  arch/sparc/include/asm/page_64.h |    1 +
> >  arch/sparc/mm/init_64.c          |   11 +++++++++++
> >  3 files changed, 20 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/sparc/Kconfig.debug b/arch/sparc/Kconfig.debug
> > index 6db35fb..92f2388 100644
> > --- a/arch/sparc/Kconfig.debug
> > +++ b/arch/sparc/Kconfig.debug
> > @@ -6,6 +6,14 @@ config TRACE_IRQFLAGS_SUPPORT
> >  
> >  source "lib/Kconfig.debug"
> >  
> > +config STRICT_DEVMEM
> > +	bool "Filter access to /dev/mem"
> > +	---help---
> > +	  If this option is disabled, 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.
> > +
> 
> Grumble - this really belongs in an arch neutral file,
> and then archs that support it should select HAVE_STRICT_DEVMEM
Yes, I saw this too.
> 
> 
> > diff --git a/arch/sparc/include/asm/page_64.h b/arch/sparc/include/asm/page_64.h
> > index aac53fc..e54ee50 100644
> > --- a/arch/sparc/include/asm/page_64.h
> > +++ b/arch/sparc/include/asm/page_64.h
> > @@ -36,6 +36,7 @@ extern void hugetlb_setup(struct pt_regs *regs);
> >  
> >  #define WANT_PAGE_VIRTUAL
> >  
> > +extern int devmem_is_allowed(unsigned long pagenr);
> >  extern void _clear_page(void *page);
> >  #define clear_page(X)	_clear_page((void *)(X))
> >  struct page;
> Grumble - and this prototype should not be required to be repeated by all archs either.
And yes, I saw this too.
> 
> My grumbling are general comments - and you may ignore these...
> But, see below.
> 
> > diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
> > index eafbc65..9c4a820 100644
> > --- a/arch/sparc/mm/init_64.c
> > +++ b/arch/sparc/mm/init_64.c
> > @@ -2694,3 +2694,14 @@ void hugetlb_setup(struct pt_regs *regs)
> >  	}
> >  }
> >  #endif
> > +
> > +#ifdef CONFIG_STRICT_DEVMEM
> > +/* devmem_is_allowed for sparc.
> > + */
> > +int devmem_is_allowed(unsigned long pagenr)
> > +{
> > +	int  rc = page_is_ram(pagenr);
> > +
> > +	return rc;
> > +}
> > +#endif
> 
> Any specific reason why this is sparc64 specific?
> In the Kconfig.debug file you allow this to be sleected also for sparc32.
To be honest it didn't even enter my thought process. The last time I had
a sparc32 was during a journey to/from Rome to the Ministry of Finance. Hm,
that was almost twenty years ago - memory fading.
> 
> And I see no reason to restrict this to sparc64 as this is anyway optional.
I'll take a peek but know little of sparc32 intersections with sparc64.

thanx for the feedback,

bob
> 
> 	Sam
> --
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 20, 2014, 12:44 a.m.
From: Bob Picco <bpicco@meloft.net>
Date: Tue, 18 Feb 2014 14:11:18 -0500

> +#ifdef CONFIG_STRICT_DEVMEM
> +/* devmem_is_allowed for sparc.
> + */
> +int devmem_is_allowed(unsigned long pagenr)
> +{
> +	int  rc = page_is_ram(pagenr);
> +
> +	return rc;
> +}
> +#endif

We already go through all of the effort to build
sparc64_valid_addr_bitmap, please use it via kern_addr_valid().

Using resources is over-engineering.  We never provided that stuff
in /proc/iomem on sparc so no need to add it unnecessarily.

Furthermore, the /dev/kmem device probably needs similar restrictions,
it just blindly copies things as long as the address is lower than
high memory.

All of this stuff can potentially trigger bus errors, and really the
right thing to do is:

1) Add the necessary address validations as being discussed here

2) Put the accessed behind something like the PCI config space poking
   framework, it is able to recover from BUS errors cleanly.  Take
   a look at the code mentioning 'pci_poke_in_progress'.

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bob Picco - Feb. 26, 2014, 3:33 p.m.
Hi Sam,

Sorry for delay but required a brief computer disconnect. Though it wasn't
relaxation like I thought but FLU related.
Sam Ravnborg wrote:	[Tue Feb 18 2014, 04:05:15PM EST]
> Hi Bob.
> 
> On Tue, Feb 18, 2014 at 02:11:18PM -0500, Bob Picco wrote:
> > From: bob picco <bpicco@meloft.net>
> > 
> > This is just the first part of restricting /dev/mem access on sparc.
> > This patch does nothing to change the current behaviour. The patch is
> > in preparation for a subsequent patch.
> > 
> > Signed-off-by: Bob Picco <bob.picco@oracle.com>
> > ---
> >  arch/sparc/Kconfig.debug         |    8 ++++++++
> >  arch/sparc/include/asm/page_64.h |    1 +
> >  arch/sparc/mm/init_64.c          |   11 +++++++++++
> >  3 files changed, 20 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/sparc/Kconfig.debug b/arch/sparc/Kconfig.debug
> > index 6db35fb..92f2388 100644
> > --- a/arch/sparc/Kconfig.debug
> > +++ b/arch/sparc/Kconfig.debug
> > @@ -6,6 +6,14 @@ config TRACE_IRQFLAGS_SUPPORT
> >  
> >  source "lib/Kconfig.debug"
> >  
> > +config STRICT_DEVMEM
> > +	bool "Filter access to /dev/mem"
> > +	---help---
> > +	  If this option is disabled, 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.
> > +
> 
> Grumble - this really belongs in an arch neutral file,
> and then archs that support it should select HAVE_STRICT_DEVMEM
> 
> 
> > diff --git a/arch/sparc/include/asm/page_64.h b/arch/sparc/include/asm/page_64.h
> > index aac53fc..e54ee50 100644
> > --- a/arch/sparc/include/asm/page_64.h
> > +++ b/arch/sparc/include/asm/page_64.h
> > @@ -36,6 +36,7 @@ extern void hugetlb_setup(struct pt_regs *regs);
> >  
> >  #define WANT_PAGE_VIRTUAL
> >  
> > +extern int devmem_is_allowed(unsigned long pagenr);
> >  extern void _clear_page(void *page);
> >  #define clear_page(X)	_clear_page((void *)(X))
> >  struct page;
> Grumble - and this prototype should not be required to be repeated by all archs either.
> 
> My grumbling are general comments - and you may ignore these...
> But, see below.
> 
> > diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
> > index eafbc65..9c4a820 100644
> > --- a/arch/sparc/mm/init_64.c
> > +++ b/arch/sparc/mm/init_64.c
> > @@ -2694,3 +2694,14 @@ void hugetlb_setup(struct pt_regs *regs)
> >  	}
> >  }
> >  #endif
> > +
> > +#ifdef CONFIG_STRICT_DEVMEM
> > +/* devmem_is_allowed for sparc.
> > + */
> > +int devmem_is_allowed(unsigned long pagenr)
> > +{
> > +	int  rc = page_is_ram(pagenr);
> > +
> > +	return rc;
> > +}
> > +#endif
> 
> Any specific reason why this is sparc64 specific?
> In the Kconfig.debug file you allow this to be sleected also for sparc32.
> 
> And I see no reason to restrict this to sparc64 as this is anyway optional.
I examined sparc32 last week. I agree and this could be supported on sparc32
and sparc64. I believe it would require common sparc mm module and sparc{32|64}
interfaces.

I didn't pursue any actual code.

bob
> 
> 	Sam
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bob Picco - Feb. 27, 2014, 11:03 a.m.
Hi,
David Miller wrote:	[Wed Feb 19 2014, 07:44:01PM EST]
> From: Bob Picco <bpicco@meloft.net>
> Date: Tue, 18 Feb 2014 14:11:18 -0500
> 
> > +#ifdef CONFIG_STRICT_DEVMEM
> > +/* devmem_is_allowed for sparc.
> > + */
> > +int devmem_is_allowed(unsigned long pagenr)
> > +{
> > +	int  rc = page_is_ram(pagenr);
> > +
> > +	return rc;
> > +}
> > +#endif
> 
> We already go through all of the effort to build
> sparc64_valid_addr_bitmap, please use it via kern_addr_valid().
Sure we certainly can accomplish it this way and probably should.
> 
> Using resources is over-engineering.  We never provided that stuff
> in /proc/iomem on sparc so no need to add it unnecessarily.
Well over-engineering might be a strong word for a path that isn't hot.
/proc/iomem would be nice with it similar to x86_64.

It seems required with kexec-tools/kexec which is functional for T4/T5 but
not ready. Though I suppose you could have only "Crash kernel".

Also I can't think of a way to make "crash" operational without
CONFIG_DEVKMEM. Though I've had little time to ponder.

For example on T5-8 the head of /proc/iomem is:
30400000-3fdde47fff : System RAM
3fdde4e000-3fdde4ffff : System RAM
80000000000-83fffffffff : System RAM
100000000000-103fffffffff : System RAM
180000000000-183fffffffff : System RAM
200000000000-203fffffffff : System RAM
280000000000-283fffffffff : System RAM
300000000000-303fffffffff : System RAM
380000000000-383fffccbfff : System RAM
  380000004000-38000051661f : Kernel code
  380000516620-380000864e3f : Kernel data
  380000900000-380001180838 : Kernel bss
  383f5f400000-383f7f3fffff : Crash kernel
383fffcec000-383fffcfbfff : System RAM
383fffd0e000-383fffd0ffff : System RAM
800000000000-80007effffff : /pci@300
...

How about a combination of your suggestion and /proc/iomem?
> 
> Furthermore, the /dev/kmem device probably needs similar restrictions,
> it just blindly copies things as long as the address is lower than
> high memory.
Ah, the holes. I'll attempt and find time to engage.

thanx,

bob
> 
> All of this stuff can potentially trigger bus errors, and really the
> right thing to do is:
> 
> 1) Add the necessary address validations as being discussed here
> 
> 2) Put the accessed behind something like the PCI config space poking
>    framework, it is able to recover from BUS errors cleanly.  Take
>    a look at the code mentioning 'pci_poke_in_progress'.
> 
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 27, 2014, 5:59 p.m.
From: Bob Picco <bpicco@meloft.net>
Date: Thu, 27 Feb 2014 06:03:15 -0500

> How about a combination of your suggestion and /proc/iomem?

Sounds good.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/sparc/Kconfig.debug b/arch/sparc/Kconfig.debug
index 6db35fb..92f2388 100644
--- a/arch/sparc/Kconfig.debug
+++ b/arch/sparc/Kconfig.debug
@@ -6,6 +6,14 @@  config TRACE_IRQFLAGS_SUPPORT
 
 source "lib/Kconfig.debug"
 
+config STRICT_DEVMEM
+	bool "Filter access to /dev/mem"
+	---help---
+	  If this option is disabled, 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.
+
 config DEBUG_DCFLUSH
 	bool "D-cache flush debugging"
 	depends on SPARC64 && DEBUG_KERNEL
diff --git a/arch/sparc/include/asm/page_64.h b/arch/sparc/include/asm/page_64.h
index aac53fc..e54ee50 100644
--- a/arch/sparc/include/asm/page_64.h
+++ b/arch/sparc/include/asm/page_64.h
@@ -36,6 +36,7 @@  extern void hugetlb_setup(struct pt_regs *regs);
 
 #define WANT_PAGE_VIRTUAL
 
+extern int devmem_is_allowed(unsigned long pagenr);
 extern void _clear_page(void *page);
 #define clear_page(X)	_clear_page((void *)(X))
 struct page;
diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
index eafbc65..9c4a820 100644
--- a/arch/sparc/mm/init_64.c
+++ b/arch/sparc/mm/init_64.c
@@ -2694,3 +2694,14 @@  void hugetlb_setup(struct pt_regs *regs)
 	}
 }
 #endif
+
+#ifdef CONFIG_STRICT_DEVMEM
+/* devmem_is_allowed for sparc.
+ */
+int devmem_is_allowed(unsigned long pagenr)
+{
+	int  rc = page_is_ram(pagenr);
+
+	return rc;
+}
+#endif