linux-kernel - Re: [PATCHv2 1/4] arm64: dump: Make ptdump debugfs a separate option

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161017105252.GA29095@leverpostej>
Date:   Mon, 17 Oct 2016 11:52:53 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Laura Abbott <labbott@...hat.com>
Cc:     AKASHI Takahiro <takahiro.akashi@...aro.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Will Deacon <will.deacon@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Kees Cook <keescook@...omium.org>,
        kernel-hardening@...ts.openwall.com, matt@...eblueprint.co.uk
Subject: Re: [PATCHv2 1/4] arm64: dump: Make ptdump debugfs a separate option

Hi Laura,

In looking at this, I realised I was confused about ptdump_initialize()
previously, and now see why we can't decouple the debugfs registration
of the kernel page tables from the rest of the ptdump init. Sorry for
the noise on that.

Aside from one issue below, this looks good to me.

On Wed, Oct 12, 2016 at 03:31:59PM -0700, Laura Abbott wrote:
> diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
> index 07b8ed0..7c35689 100644
> --- a/arch/arm64/include/asm/ptdump.h
> +++ b/arch/arm64/include/asm/ptdump.h
> @@ -16,9 +16,10 @@
>  #ifndef __ASM_PTDUMP_H
>  #define __ASM_PTDUMP_H
>  
> -#ifdef CONFIG_ARM64_PTDUMP
> +#ifdef CONFIG_ARM64_PTDUMP_CORE
>  
>  #include <linux/mm_types.h>
> +#include <linux/seq_file.h>
>  
>  struct addr_marker {
>  	unsigned long start_address;
> @@ -32,13 +33,15 @@ struct ptdump_info {
>  	unsigned long			max_addr;
>  };
>  
> -int ptdump_register(struct ptdump_info *info, const char *name);
> -
> +void ptdump_walk_pgd(struct seq_file *s, struct ptdump_info *info);
> +#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS
> +int ptdump_debugfs_register(struct ptdump_info *info, const char *name);
>  #else
> -static inline int ptdump_register(struct ptdump_info *info, const char *name)
> +static inline int ptdump_debugfs_register(struct ptdump_info *info,
> +					const char *name)
>  {
>  	return 0;
>  }
> -#endif /* CONFIG_ARM64_PTDUMP */
> +#endif

I think you didn't mean to remove the existing endif here?

It's still needed to guard the CONFIG_ARM64_PTDUMP_CORE case, and the
new one is needed for the new #ifdef CONFIG_ARM64_PTDUMP_DEBUGFS.
Without it, I get a build error with this patch atop of v4.9-rc1 with
CONFIG_ARM64_PTDUMP_DEBUGFS selected:

[mark@...erpostej:~/src/linux]% uselinaro 15.08 make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- -j10 -s
In file included from arch/arm64/mm/ptdump_debugfs.c:4:0:
./arch/arm64/include/asm/ptdump.h:16:0: error: unterminated #ifndef
 #ifndef __ASM_PTDUMP_H
 ^
make[1]: *** [arch/arm64/mm/ptdump_debugfs.o] Error 1
make[1]: *** Waiting for unfinished jobs....

With that #endif restored, everything works fine. So FWIW, with that:

Reviewed-by: Mark Rutland <mark.rutland@....com>
Tested-by: Mark Rutland <mark.rutland@....com>

[...]

> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
> index 7c75a8d..33d35e8 100644
> --- a/drivers/firmware/efi/arm-runtime.c
> +++ b/drivers/firmware/efi/arm-runtime.c
> @@ -39,7 +39,7 @@ static struct mm_struct efi_mm = {
>  	.mmlist			= LIST_HEAD_INIT(efi_mm.mmlist),
>  };
>  
> -#ifdef CONFIG_ARM64_PTDUMP
> +#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS
>  #include <asm/ptdump.h>
>  
>  static struct ptdump_info efi_ptdump_info = {
> @@ -53,10 +53,9 @@ static struct ptdump_info efi_ptdump_info = {
>  
>  static int __init ptdump_init(void)
>  {
> -	return ptdump_register(&efi_ptdump_info, "efi_page_tables");
> +	return ptdump_debugfs_register(&efi_ptdump_info, "efi_page_tables");
>  }
>  device_initcall(ptdump_init);
> -
>  #endif

For the EFI changes, we'll need an ack from Ard or Matt; this should
probably be Cc'd to the linux-efi list for that.

Thanks,
Mark.