sh: Stop printing the virtual memory layout
diff mbox series

Message ID 20200305151010.835954-1-nivedita@alum.mit.edu
State New
Headers show
Series
  • sh: Stop printing the virtual memory layout
Related show

Commit Message

Arvind Sankar March 5, 2020, 3:10 p.m. UTC
For security, don't display the kernel's virtual memory layout.

Kees Cook points out:
"These have been entirely removed on other architectures, so let's
just do the same for ia32 and remove it unconditionally."

071929dbdd86 ("arm64: Stop printing the virtual memory layout")
1c31d4e96b8c ("ARM: 8820/1: mm: Stop printing the virtual memory layout")
31833332f798 ("m68k/mm: Stop printing the virtual memory layout")
fd8d0ca25631 ("parisc: Hide virtual kernel memory layout")
adb1fe9ae2ee ("mm/page_alloc: Remove kernel address exposure in free_reserved_area()")

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/sh/mm/init.c | 41 -----------------------------------------
 1 file changed, 41 deletions(-)

Comments

John Paul Adrian Glaubitz March 5, 2020, 3:18 p.m. UTC | #1
On 3/5/20 4:10 PM, Arvind Sankar wrote:
> For security, don't display the kernel's virtual memory layout.
> 
> Kees Cook points out:
> "These have been entirely removed on other architectures, so let's
> just do the same for ia32 and remove it unconditionally."
> 
> 071929dbdd86 ("arm64: Stop printing the virtual memory layout")
> 1c31d4e96b8c ("ARM: 8820/1: mm: Stop printing the virtual memory layout")
> 31833332f798 ("m68k/mm: Stop printing the virtual memory layout")
> fd8d0ca25631 ("parisc: Hide virtual kernel memory layout")
> adb1fe9ae2ee ("mm/page_alloc: Remove kernel address exposure in free_reserved_area()")
Aww, why wasn't this made configurable? I found these memory map printouts
very useful for development.

Adrian
Joe Perches March 5, 2020, 3:38 p.m. UTC | #2
On Thu, 2020-03-05 at 16:18 +0100, John Paul Adrian Glaubitz wrote:
> On 3/5/20 4:10 PM, Arvind Sankar wrote:
> > For security, don't display the kernel's virtual memory layout.
> > 
> > Kees Cook points out:
> > "These have been entirely removed on other architectures, so let's
> > just do the same for ia32 and remove it unconditionally."
> > 
> > 071929dbdd86 ("arm64: Stop printing the virtual memory layout")
> > 1c31d4e96b8c ("ARM: 8820/1: mm: Stop printing the virtual memory layout")
> > 31833332f798 ("m68k/mm: Stop printing the virtual memory layout")
> > fd8d0ca25631 ("parisc: Hide virtual kernel memory layout")
> > adb1fe9ae2ee ("mm/page_alloc: Remove kernel address exposure in free_reserved_area()")
> Aww, why wasn't this made configurable? I found these memory map printouts
> very useful for development.

It could be changed from pr_info to pr_devel.

A #define DEBUG would have to be added to emit it.
John Paul Adrian Glaubitz March 5, 2020, 3:41 p.m. UTC | #3
On 3/5/20 4:38 PM, Joe Perches wrote:
>> Aww, why wasn't this made configurable? I found these memory map printouts
>> very useful for development.
> 
> It could be changed from pr_info to pr_devel.
> 
> A #define DEBUG would have to be added to emit it.

Well, from the discussion it seems the decision to cut it out has already been
made, so I guess it's too late :(.

Adrian
Arvind Sankar March 5, 2020, 3:46 p.m. UTC | #4
On Thu, Mar 05, 2020 at 04:41:05PM +0100, John Paul Adrian Glaubitz wrote:
> On 3/5/20 4:38 PM, Joe Perches wrote:
> >> Aww, why wasn't this made configurable? I found these memory map printouts
> >> very useful for development.
> > 
> > It could be changed from pr_info to pr_devel.
> > 
> > A #define DEBUG would have to be added to emit it.
> 
> Well, from the discussion it seems the decision to cut it out has already been
> made, so I guess it's too late :(.
> 
> Adrian
> 
> -- 
>  .''`.  John Paul Adrian Glaubitz
> : :' :  Debian Developer - glaubitz@debian.org
> `. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
>   `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Not really too late. I can do s/pr_info/pr_devel and resubmit.

parisc for eg actually hides this in #if 0 rather than deleting the
code.

Kees, you fine with that?
John Paul Adrian Glaubitz March 5, 2020, 3:49 p.m. UTC | #5
On 3/5/20 4:46 PM, Arvind Sankar wrote:
> Not really too late. I can do s/pr_info/pr_devel and resubmit.
> 
> parisc for eg actually hides this in #if 0 rather than deleting the
> code.
> 
> Kees, you fine with that?

But wasn't it removed for all the other architectures already? Or are these
changes not in Linus' tree yet?

Adrian
Arvind Sankar March 5, 2020, 3:56 p.m. UTC | #6
On Thu, Mar 05, 2020 at 04:49:22PM +0100, John Paul Adrian Glaubitz wrote:
> On 3/5/20 4:46 PM, Arvind Sankar wrote:
> > Not really too late. I can do s/pr_info/pr_devel and resubmit.
> > 
> > parisc for eg actually hides this in #if 0 rather than deleting the
> > code.
> > 
> > Kees, you fine with that?
> 
> But wasn't it removed for all the other architectures already? Or are these
> changes not in Linus' tree yet?
> 
> Adrian

The ones mentioned in the commit message, yes, those are long gone. But
I don't see any reason why the remaining ones (there are 6 left that I
submitted patches just now for) couldn't switch to pr_devel instead.
Kees Cook March 5, 2020, 5:34 p.m. UTC | #7
On Thu, Mar 05, 2020 at 10:46:58AM -0500, Arvind Sankar wrote:
> On Thu, Mar 05, 2020 at 04:41:05PM +0100, John Paul Adrian Glaubitz wrote:
> > On 3/5/20 4:38 PM, Joe Perches wrote:
> > >> Aww, why wasn't this made configurable? I found these memory map printouts
> > >> very useful for development.
> > > 
> > > It could be changed from pr_info to pr_devel.
> > > 
> > > A #define DEBUG would have to be added to emit it.
> > 
> > Well, from the discussion it seems the decision to cut it out has already been
> > made, so I guess it's too late :(.
> > 
> > Adrian
> > 
> > -- 
> >  .''`.  John Paul Adrian Glaubitz
> > : :' :  Debian Developer - glaubitz@debian.org
> > `. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
> >   `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913
> 
> Not really too late. I can do s/pr_info/pr_devel and resubmit.
> 
> parisc for eg actually hides this in #if 0 rather than deleting the
> code.
> 
> Kees, you fine with that?

I don't mind pr_devel(). ("#if 0" tends to lead to code-rot since it's
not subjected to syntax checking in case the names of things change.)
That said, it's really up to the arch maintainers.
Tycho Andersen March 5, 2020, 8:51 p.m. UTC | #8
On Thu, Mar 05, 2020 at 10:56:29AM -0500, Arvind Sankar wrote:
> On Thu, Mar 05, 2020 at 04:49:22PM +0100, John Paul Adrian Glaubitz wrote:
> > On 3/5/20 4:46 PM, Arvind Sankar wrote:
> > > Not really too late. I can do s/pr_info/pr_devel and resubmit.
> > > 
> > > parisc for eg actually hides this in #if 0 rather than deleting the
> > > code.
> > > 
> > > Kees, you fine with that?
> > 
> > But wasn't it removed for all the other architectures already? Or are these
> > changes not in Linus' tree yet?
> > 
> > Adrian
> 
> The ones mentioned in the commit message, yes, those are long gone. But
> I don't see any reason why the remaining ones (there are 6 left that I
> submitted patches just now for) couldn't switch to pr_devel instead.

If you do happen to re-send with pr_debug() instead, feel free to add
my ack to that series as well. But in any case, this one is also:

Acked-by: Tycho Andersen <tycho@tycho.ws>
John Paul Adrian Glaubitz March 5, 2020, 8:56 p.m. UTC | #9
> On Mar 5, 2020, at 9:52 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> 
> On Thu, Mar 05, 2020 at 10:56:29AM -0500, Arvind Sankar wrote:
>>> On Thu, Mar 05, 2020 at 04:49:22PM +0100, John Paul Adrian Glaubitz wrote:
>>> On 3/5/20 4:46 PM, Arvind Sankar wrote:
>>>> Not really too late. I can do s/pr_info/pr_devel and resubmit.
>>>> 
>>>> parisc for eg actually hides this in #if 0 rather than deleting the
>>>> code.
>>>> 
>>>> Kees, you fine with that?
>>> 
>>> But wasn't it removed for all the other architectures already? Or are these
>>> changes not in Linus' tree yet?
>>> 
>>> Adrian
>> 
>> The ones mentioned in the commit message, yes, those are long gone. But
>> I don't see any reason why the remaining ones (there are 6 left that I
>> submitted patches just now for) couldn't switch to pr_devel instead.
> 
> If you do happen to re-send with pr_debug() instead, feel free to add
> my ack to that series as well.

Since it already got removed for most other architectures, I don’t think it makes much sense to keep it for consistency.

I just didn’t understand why it was made configurable for debugging purposes in the first place.

Also, many distributions disable access to the kernel buffer for unprivileged users anyway.

Adrian
Kees Cook March 5, 2020, 9:17 p.m. UTC | #10
On Thu, Mar 05, 2020 at 01:51:58PM -0700, Tycho Andersen wrote:
> On Thu, Mar 05, 2020 at 10:56:29AM -0500, Arvind Sankar wrote:
> > On Thu, Mar 05, 2020 at 04:49:22PM +0100, John Paul Adrian Glaubitz wrote:
> > > On 3/5/20 4:46 PM, Arvind Sankar wrote:
> > > > Not really too late. I can do s/pr_info/pr_devel and resubmit.
> > > > 
> > > > parisc for eg actually hides this in #if 0 rather than deleting the
> > > > code.
> > > > 
> > > > Kees, you fine with that?
> > > 
> > > But wasn't it removed for all the other architectures already? Or are these
> > > changes not in Linus' tree yet?
> > > 
> > > Adrian
> > 
> > The ones mentioned in the commit message, yes, those are long gone. But
> > I don't see any reason why the remaining ones (there are 6 left that I
> > submitted patches just now for) couldn't switch to pr_devel instead.
> 
> If you do happen to re-send with pr_debug() instead, feel free to add

(FYI, pr_devel() was suggested, not pr_debug() -- the former is
compile-time enabled with DEBUG and the latter can be enabled dynamically
in some cases in the kernel, so pr_debug() should not be used.)

> my ack to that series as well. But in any case, this one is also:
> 
> Acked-by: Tycho Andersen <tycho@tycho.ws>

Same for me. :) Consider the series:

Acked-by: Kees Cook <keescook@chromium.org>
Geert Uytterhoeven March 6, 2020, 8:04 a.m. UTC | #11
Hi Adrian,

On Thu, Mar 5, 2020 at 4:18 PM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On 3/5/20 4:10 PM, Arvind Sankar wrote:
> > For security, don't display the kernel's virtual memory layout.
> >
> > Kees Cook points out:
> > "These have been entirely removed on other architectures, so let's
> > just do the same for ia32 and remove it unconditionally."
> >
> > 071929dbdd86 ("arm64: Stop printing the virtual memory layout")
> > 1c31d4e96b8c ("ARM: 8820/1: mm: Stop printing the virtual memory layout")
> > 31833332f798 ("m68k/mm: Stop printing the virtual memory layout")
> > fd8d0ca25631 ("parisc: Hide virtual kernel memory layout")
> > adb1fe9ae2ee ("mm/page_alloc: Remove kernel address exposure in free_reserved_area()")
> Aww, why wasn't this made configurable? I found these memory map printouts
> very useful for development.

In most of the above (but not in this patch), "%p" was used to print
addresses, which started showing useless hashed addresses since commit
ad67b74d2469d9b8 ("printk: hash addresses printed with %p").

Instead of changing them all to print usable addresses instead, it was
agreed upon to just remove them.

Gr{oetje,eeting}s,

                        Geert
Kaiwan N Billimoria March 8, 2020, 12:17 p.m. UTC | #12
On Thu, Mar 5, 2020 at 8:48 PM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
>
> On 3/5/20 4:10 PM, Arvind Sankar wrote:
> > For security, don't display the kernel's virtual memory layout.
> >
> > Kees Cook points out:
> > "These have been entirely removed on other architectures, so let's
> > just do the same for ia32 and remove it unconditionally."
> >
> > 071929dbdd86 ("arm64: Stop printing the virtual memory layout")
> > 1c31d4e96b8c ("ARM: 8820/1: mm: Stop printing the virtual memory layout")
> > 31833332f798 ("m68k/mm: Stop printing the virtual memory layout")
> > fd8d0ca25631 ("parisc: Hide virtual kernel memory layout")
> > adb1fe9ae2ee ("mm/page_alloc: Remove kernel address exposure in free_reserved_area()")
> Aww, why wasn't this made configurable? I found these memory map printouts
> very useful for development.

Same here! IMO, the kernel segment layout is useful for devs/debug purposes.
Perhaps:
a) all these printk's could be gathered into one function and invoked
only when DEBUG (or equivalent) is defined?
b) else, the s/pr_info/pr_devel approach with %pK should be good?
-Kaiwan.

Patch
diff mbox series

diff --git a/arch/sh/mm/init.c b/arch/sh/mm/init.c
index d1b1ff2be17a..e68a1106e99b 100644
--- a/arch/sh/mm/init.c
+++ b/arch/sh/mm/init.c
@@ -360,47 +360,6 @@  void __init mem_init(void)
 	vsyscall_init();
 
 	mem_init_print_info(NULL);
-	pr_info("virtual kernel memory layout:\n"
-		"    fixmap  : 0x%08lx - 0x%08lx   (%4ld kB)\n"
-#ifdef CONFIG_HIGHMEM
-		"    pkmap   : 0x%08lx - 0x%08lx   (%4ld kB)\n"
-#endif
-		"    vmalloc : 0x%08lx - 0x%08lx   (%4ld MB)\n"
-		"    lowmem  : 0x%08lx - 0x%08lx   (%4ld MB) (cached)\n"
-#ifdef CONFIG_UNCACHED_MAPPING
-		"            : 0x%08lx - 0x%08lx   (%4ld MB) (uncached)\n"
-#endif
-		"      .init : 0x%08lx - 0x%08lx   (%4ld kB)\n"
-		"      .data : 0x%08lx - 0x%08lx   (%4ld kB)\n"
-		"      .text : 0x%08lx - 0x%08lx   (%4ld kB)\n",
-		FIXADDR_START, FIXADDR_TOP,
-		(FIXADDR_TOP - FIXADDR_START) >> 10,
-
-#ifdef CONFIG_HIGHMEM
-		PKMAP_BASE, PKMAP_BASE+LAST_PKMAP*PAGE_SIZE,
-		(LAST_PKMAP*PAGE_SIZE) >> 10,
-#endif
-
-		(unsigned long)VMALLOC_START, VMALLOC_END,
-		(VMALLOC_END - VMALLOC_START) >> 20,
-
-		(unsigned long)memory_start, (unsigned long)high_memory,
-		((unsigned long)high_memory - (unsigned long)memory_start) >> 20,
-
-#ifdef CONFIG_UNCACHED_MAPPING
-		uncached_start, uncached_end, uncached_size >> 20,
-#endif
-
-		(unsigned long)&__init_begin, (unsigned long)&__init_end,
-		((unsigned long)&__init_end -
-		 (unsigned long)&__init_begin) >> 10,
-
-		(unsigned long)&_etext, (unsigned long)&_edata,
-		((unsigned long)&_edata - (unsigned long)&_etext) >> 10,
-
-		(unsigned long)&_text, (unsigned long)&_etext,
-		((unsigned long)&_etext - (unsigned long)&_text) >> 10);
-
 	mem_init_done = 1;
 }