linux-kernel - Re: [PATCH v19 057/130] KVM: x86/mmu: Add a new is_private member for union kvm_mmu_page

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]

Date: Thu, 21 Mar 2024 00:18:47 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Yamahata,
 Isaku" <isaku.yamahata@...el.com>
CC: "Zhang, Tina" <tina.zhang@...el.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "Yuan, Hang" <hang.yuan@...el.com>, "Huang, Kai"
	<kai.huang@...el.com>, "Chen, Bo2" <chen.bo@...el.com>, "sagis@...gle.com"
	<sagis@...gle.com>, "isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
	"Aktas, Erdem" <erdemaktas@...gle.com>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>
Subject: Re: [PATCH v19 057/130] KVM: x86/mmu: Add a new is_private member for
 union kvm_mmu_page_role

On Mon, 2024-02-26 at 00:25 -0800, isaku.yamahata@...el.com wrote:
> From: Isaku Yamahata <isaku.yamahata@...el.com>
> 
> Because TDX support introduces private mapping, add a new member in
> union
> kvm_mmu_page_role with access functions to check the member.

I guess we should have a role bit for private like in this patch, but
just barely. AFAICT we have a gfn and struct kvm in every place where
it is checked (assuming my proposal in patch 56 holds water). So we
could have
bool is_private = !(gfn & kvm_gfn_shared_mask(kvm));

But there are extra bits available in the role, so we can skip the
extra step. Can you think of any more reasons? I want to try to write a
log for this one. It's very short.

> 
> +static inline bool is_private_sptep(u64 *sptep)
> +{
> +       if (WARN_ON_ONCE(!sptep))
> +               return false;

This is not supposed to be NULL, from the existence of the warning. It
looks like some previous comments were to not let the NULL pointer
deference happen and bail if it's NULL. I think maybe we should just
drop the check and warning completely. The NULL pointer deference will
be plenty loud if it happens.

> +       return is_private_sp(sptep_to_sp(sptep));
> +}
> +