* Re: [PATCH 1/2] sparc64 - strict devmem
2014-02-18 19:11 [PATCH 1/2] sparc64 - strict devmem Bob Picco
@ 2014-02-18 21:05 ` Sam Ravnborg
2014-02-18 22:10 ` Bob Picco
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Sam Ravnborg @ 2014-02-18 21:05 UTC (permalink / raw)
To: sparclinux
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] sparc64 - strict devmem
2014-02-18 19:11 [PATCH 1/2] sparc64 - strict devmem Bob Picco
2014-02-18 21:05 ` Sam Ravnborg
@ 2014-02-18 22:10 ` Bob Picco
2014-02-20 0:44 ` David Miller
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Bob Picco @ 2014-02-18 22:10 UTC (permalink / raw)
To: sparclinux
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] sparc64 - strict devmem
2014-02-18 19:11 [PATCH 1/2] sparc64 - strict devmem Bob Picco
2014-02-18 21:05 ` Sam Ravnborg
2014-02-18 22:10 ` Bob Picco
@ 2014-02-20 0:44 ` David Miller
2014-02-26 15:33 ` Bob Picco
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2014-02-20 0:44 UTC (permalink / raw)
To: sparclinux
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'.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] sparc64 - strict devmem
2014-02-18 19:11 [PATCH 1/2] sparc64 - strict devmem Bob Picco
` (2 preceding siblings ...)
2014-02-20 0:44 ` David Miller
@ 2014-02-26 15:33 ` Bob Picco
2014-02-27 11:03 ` Bob Picco
2014-02-27 17:59 ` David Miller
5 siblings, 0 replies; 7+ messages in thread
From: Bob Picco @ 2014-02-26 15:33 UTC (permalink / raw)
To: sparclinux
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] sparc64 - strict devmem
2014-02-18 19:11 [PATCH 1/2] sparc64 - strict devmem Bob Picco
` (3 preceding siblings ...)
2014-02-26 15:33 ` Bob Picco
@ 2014-02-27 11:03 ` Bob Picco
2014-02-27 17:59 ` David Miller
5 siblings, 0 replies; 7+ messages in thread
From: Bob Picco @ 2014-02-27 11:03 UTC (permalink / raw)
To: sparclinux
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'.
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] sparc64 - strict devmem
2014-02-18 19:11 [PATCH 1/2] sparc64 - strict devmem Bob Picco
` (4 preceding siblings ...)
2014-02-27 11:03 ` Bob Picco
@ 2014-02-27 17:59 ` David Miller
5 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2014-02-27 17:59 UTC (permalink / raw)
To: sparclinux
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.
^ permalink raw reply [flat|nested] 7+ messages in thread