linux-kernel - Re: [PATCH v4 02/10] KVM: arm64: Add ptdump registration with debugfs for the stage-2 pagetables

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: <ZYSAfORj2-cXo5t_@linux.dev>
Date: Thu, 21 Dec 2023 18:14:20 +0000
From: Oliver Upton <oliver.upton@...ux.dev>
To: Sebastian Ene <sebastianene@...gle.com>
Cc: will@...nel.org, James Morse <james.morse@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Zenghui Yu <yuzenghui@...wei.com>, catalin.marinas@....com,
	mark.rutland@....com, akpm@...ux-foundation.org, maz@...nel.org,
	kvmarm@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, kernel-team@...roid.com,
	vdonnefort@...gle.com, qperret@...gle.com, smostafa@...gle.com
Subject: Re: [PATCH v4 02/10] KVM: arm64: Add ptdump registration with
 debugfs for the stage-2 pagetables

On Mon, Dec 18, 2023 at 01:58:52PM +0000, Sebastian Ene wrote:
> +config PTDUMP_STAGE2_DEBUGFS
> +       bool "Present the stage-2 pagetables to debugfs"
> +       depends on PTDUMP_DEBUGFS && KVM
> +       default n
> +       help
> +         Say Y here if you want to show the stage-2 kernel pagetables
> +         layout in a debugfs file. This information is only useful for kernel developers
> +         who are working in architecture specific areas of the kernel.
> +         It is probably not a good idea to enable this feature in a production
> +         kernel.

It isn't really a good idea to mount debugfs at all in a production
system. There are already plenty worse interfaces lurking in that
filesystem. The pKVM portions already depend on CONFIG_NVHE_EL2_DEBUG,
so I don't see a need for this Kconfig option.

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index e5f75f1f1..ee8d7cb67 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -40,6 +40,7 @@
>  #include <asm/kvm_pkvm.h>
>  #include <asm/kvm_emulate.h>
>  #include <asm/sections.h>
> +#include <kvm_ptdump.h>
>  
>  #include <kvm/arm_hypercalls.h>
>  #include <kvm/arm_pmu.h>
> @@ -2592,6 +2593,7 @@ static __init int kvm_arm_init(void)
>  	if (err)
>  		goto out_subs;
>  
> +	kvm_ptdump_register_host();
>  	kvm_arm_initialised = true;
>  
>  	return 0;
> diff --git a/arch/arm64/kvm/kvm_ptdump.h b/arch/arm64/kvm/kvm_ptdump.h
> new file mode 100644
> index 000000000..98b595ce8
> --- /dev/null
> +++ b/arch/arm64/kvm/kvm_ptdump.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +//
> +// Copyright (C) Google, 2023
> +// Author: Sebastian Ene <sebastianene@...gle.com>

You've got the comment styles backwards for these. The SPDX license uses
the 'C++' style comment (//), whereas your multiline comment should always
use a 'C' style comment (/* */).

> +struct kvm_ptdump_register {
> +	void *(*get_ptdump_info)(struct kvm_ptdump_register *reg);
> +	void (*put_ptdump_info)(void *priv);
> +	int (*show_ptdump_info)(struct seq_file *m, void *v);
> +	void *priv;
> +};

Please thoroughly consider the necessity of this. You're wrapping a
callback structure with yet another callback structure. IMO, it would
make a lot more sense to implement the file ops structure for every
walker variant you need and avoid the indirection, it's hard to
understand.

> +void kvm_ptdump_register_host(void)
> +{
> +	if (!is_protected_kvm_enabled())
> +		return;
> +
> +	kvm_ptdump_debugfs_register(&host_reg, "host_page_tables",
> +				    kvm_debugfs_dir);
> +}
> +
> +static int __init kvm_host_ptdump_init(void)
> +{
> +	host_reg.priv = (void *)host_s2_pgtable_pages();
> +	return 0;
> +}
> +
> +device_initcall(kvm_host_ptdump_init);

Why can't all of this be called from finalize_pkvm()?

> -- 
> 2.43.0.472.g3155946c3a-goog
> 

-- 
Thanks,
Oliver