linux-kernel - Re: [PATCH 07/13] x86/virt/tdx: Add SEAMCALL wrapper tdh_mem_sept

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: <f92121f411209152f2ab22b5c8dfa9ec74831499.camel@intel.com>
Date: Wed, 8 Jan 2025 01:43:37 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Hansen, Dave" <dave.hansen@...el.com>, "Zhao, Yan Y"
	<yan.y.zhao@...el.com>
CC: "Yamahata, Isaku" <isaku.yamahata@...el.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "pbonzini@...hat.com" <pbonzini@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"sean.j.christopherson@...el.com" <sean.j.christopherson@...el.com>, "Huang,
 Kai" <kai.huang@...el.com>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>
Subject: Re: [PATCH 07/13] x86/virt/tdx: Add SEAMCALL wrapper
 tdh_mem_sept_add() to add SEPT pages

On Tue, 2025-01-07 at 17:20 -0800, Dave Hansen wrote:
> On 1/7/25 17:12, Yan Zhao wrote:
> > So, why does this bitfields definition make things worse?
> 
> Look at the kernel page table management. Why don't we use bitfields for
> _that_? Look at the link I sent. Bitfields can cause some really goofy
> unexpected behavior if you pass them around like they were a full type.

Huh, so this enum is unsafe for reading out the individual fields because if
shifting them, it will perform the shift with the size of the source bit field
size. It is safe in the way it is being used in these patches, which is to
encode a u64. But if we ever started to use tdx_sept_gpa_mapping_info to process
output from a SEAMCALL, or something, we could set ourselves up for the same
problem as the SEV bug.

Let's not open code the encoding in each SEAMCALL though. What about replacing
it with just a helper that encodes the u64 gpa from two args: gfn and tdx_level.
We could add some specific over-size behavior for the fields, but I'd think it
would be ok to keep it simple. Maybe something like this:

static u64 encode_gpa_mapping_info(gfn_t gfn, unsigned int tdx_level)
{
	u64 val = 0;

	val |= level;
	val |= gfn << TDX_MAPPING_INFO_GFN_SHIFT;

	return val;
}