| Message ID | 20200305151010.835954-1-nivedita@alum.mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sh: Stop printing the virtual memory layout | expand |
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
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.
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
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?
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
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.
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.
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>
> 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
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>
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
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.
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; }
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(-)