| 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
| ||
|
Message-ID: <7d9ef614838fb5b40305aae7e66003e0bb319b53.camel@intel.com>
Date: Wed, 28 Jan 2026 22:33:01 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "Gao, Chao" <chao.gao@...el.com>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "linux-coco@...ts.linux.dev"
<linux-coco@...ts.linux.dev>, "bp@...en8.de" <bp@...en8.de>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, "kas@...nel.org"
<kas@...nel.org>, "mingo@...hat.com" <mingo@...hat.com>, "Weiny, Ira"
<ira.weiny@...el.com>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "seanjc@...gle.com" <seanjc@...gle.com>,
"Verma, Vishal L" <vishal.l.verma@...el.com>, "nik.borisov@...e.com"
<nik.borisov@...e.com>, "Chatre, Reinette" <reinette.chatre@...el.com>,
"hpa@...or.com" <hpa@...or.com>, "sagis@...gle.com" <sagis@...gle.com>,
"Chen, Farrah" <farrah.chen@...el.com>, "Duan, Zhenzhong"
<zhenzhong.duan@...el.com>, "tglx@...utronix.de" <tglx@...utronix.de>,
"Edgecombe, Rick P" <rick.p.edgecombe@...el.com>, "paulmck@...nel.org"
<paulmck@...nel.org>, "Annapurve, Vishal" <vannapurve@...gle.com>,
"yilun.xu@...ux.intel.com" <yilun.xu@...ux.intel.com>, "x86@...nel.org"
<x86@...nel.org>, "Williams, Dan J" <dan.j.williams@...el.com>
Subject: Re: [PATCH v3 13/26] x86/virt/seamldr: Allocate and populate a module
update request
On Wed, 2026-01-28 at 19:28 +0800, Gao, Chao wrote:
> On Tue, Jan 27, 2026 at 11:21:06AM +0800, Huang, Kai wrote:
> >
> > > +/*
> > > + * Allocate and populate a seamldr_params.
> > > + * Note that both @module and @sig should be vmalloc'd memory.
> > > + */
> > > +static struct seamldr_params *alloc_seamldr_params(const void *module, unsigned int module_size,
> > > + const void *sig, unsigned int sig_size)
> > > +{
> > > + struct seamldr_params *params;
> > > + const u8 *ptr;
> > > + int i;
> > > +
> > > + BUILD_BUG_ON(sizeof(struct seamldr_params) != SZ_4K);
> > > + if (module_size > SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + if (!IS_ALIGNED(module_size, SZ_4K) || sig_size != SZ_4K ||
> > > + !IS_ALIGNED((unsigned long)module, SZ_4K) ||
> > > + !IS_ALIGNED((unsigned long)sig, SZ_4K))
> > > + return ERR_PTR(-EINVAL);
> > > +
> >
> > Based on the the blob format link below, we have
> >
> > struct tdx_blob
> > {
> > ...
> > _u64 sigstruct[256]; // 2KB sigstruct,intel_tdx_module.so.sigstruct
> > _u64 reserved2[256]; // Reserved space
> > ...
> > }
> >
> > So it's clear SIGSTRUCT is just 2KB and the second half 2KB is "reserved
> > space".
> >
> > Why is the "reserved space" treated as part of SIGSTRUCT here?
>
> Good question. Because the space is reserved for sigstruct expansion.
>
> The __current__ SEAMLDR ABI accepts one 4KB page, but all __existing__
> sigstructs are only 2KB.
>
Oh I see.
I think we have two perspectives here: 1) what P-SEAMLDR ABI requires for
module and sigstruct; 2) how does the kernel get them and pass to
alloc_seamldr_params().
IIUC, I now understand alloc_seamldr_params() is expecting the 'module',
'module_size', 'sig' and 'sig_size' to meet P-SEAMCALL's ABI.
Then would it be better to add a comment for the checks of 'module',
'module_size', 'sig' and 'sig_size' in alloc_seamldr_params() (below code)
that it is P-SEAMCALL ABI that has these requirement?
if (!IS_ALIGNED(module_size, SZ_4K) || sig_size != SZ_4K ||
!IS_ALIGNED((unsigned long)module, SZ_4K) ||
!IS_ALIGNED((unsigned long)sig, SZ_4K))
return ERR_PTR(-EINVAL);
Otherwise it's a bit confusing because these 4 arguments are passed to
alloc_seamldr_params() right from the layout of 'struct tdx_blob' which is a
"software-organized" structure which, theoretically, could have nothing to
do P-SEAMLDR ABI.
> so, tdx_blob currently defines a 2KB sigstruct field
> followed by 2KB of reserved space. We anticipate that sigstructs will
> eventually exceed 4KB, so we added reserved3[N*512] to accommodate future
> growth.
>
> You're right. The current tdx_blob definition doesn't clearly indicate that
> reserved2/3 are actually part of the sigstruct.
>
> Does this revised tdx_blob definition make that clearer and better align with
> this patch?
>
Yes it's clearer, from the perspective that how it matches your code to
calculate 'sig_size'.
> The idea is to make tdx_blob generic enough to clearly represent:
> a 4KB header, followed by 4KB-aligned sigstruct, followed by the TDX Module
> binary. Current SEAMLDR ABI details or current sigstruct sizes are irrelevant.
>
> struct tdx_blob
> {
> _u16 version; // Version number
> _u16 checksum; // Checksum of the entire blob should be zero
> _u32 offset_of_module; // Offset of the module binary intel_tdx_module.bin in bytes
> _u8 signature[8]; // Must be "TDX-BLOB"
> _u32 length; // The length in bytes of the entire blob
> _u32 reserved0; // Reserved space
> _u64 reserved1[509]; // Reserved space
> _u64 sigstruct[512 + N*512]; // sigstruct, 4KB aligned
>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> _u8 module[]; // intel_tdx_module.bin, 4KB aligned, to the end of the file
> }
>
>
> >
A side topic:
I checked the SEAMLDR.INSTALL. It appears the only requirement of the
SIGSTRUCT is it is 4K aligned. There's no where in the ABI (certainly not
in SEAMLDR_PARAMS) to tell how does SEAMLDR.INSTALL verifies the size of
SIGSTRUCT.
Is this right?
When we bumping SIGSTRUCT to a larger size, do we have some kinda
enumeration that reports such?
From your patch 24, IIUC I don't see such enumeration or explicit opt-in,
because you just changes the layout of SEAMLDR_PARAM w/o even changing it's
version.
[...]
> > But I think if we add 'sigstruct' to the 'struct tdx_blob', e.g.,
> >
> > struct tdx_blob {
> > u16 version;
> > ...
> > u64 rsvd2[509];
> > u64 sigstruct[256];
> > u64 rsvd3[256];
> > u64 data;
> > } __packed;
> >
> > .. we can just use
> >
> > sig = blob->sigstruct;
> > sig_size = 2K (or 4K I don't quite follow);
> >
> > which is clearer to read IMHO?
>
> The problem is hard-coding the sigstruct size to 2KB/4KB. This will soon no
> longer hold.
>
> But
> sig = blob->data;
> sig_size = blob->offset_of_module - sizeof(struct tdx_blob);
>
> doesn't make that assumption, making it more future-proof.
Sure. I am certainly fine with making it future-proof (albeit arguably you
could also change the way that how sig_size is calculated in the future,
i.e., in your patch 24).
But the real point is the code here needs to reflect the 'struct tdx_blob'
description in the doc. But with the current doc I don't see they match to
each other:
The doc says SIGSTRUCT is 2K but the code says it's 4K.
So I think you need to update the 'struct tdx_blob' description in the doc
to justify such code.
Btw, I think the link
https://github.com/intel/tdx-module-binaries/blob/main/blob_structure.txt
is subject to change, both the link itself and it's content.
Do you think we should just make the layout of 'struct tdx_blob' as a
documentation patch and include that to this series?
Powered by blists - more mailing lists