| Message ID | 1392750679-6966-1-git-send-email-bpicco@meloft.net |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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
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