| 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: <aXny+UkkEzU425k6@intel.com>
Date: Wed, 28 Jan 2026 19:28:57 +0800
From: Chao Gao <chao.gao@...el.com>
To: "Huang, Kai" <kai.huang@...el.com>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "linux-coco@...ts.linux.dev"
<linux-coco@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, "kas@...nel.org"
<kas@...nel.org>, "seanjc@...gle.com" <seanjc@...gle.com>, "Chatre, Reinette"
<reinette.chatre@...el.com>, "Weiny, Ira" <ira.weiny@...el.com>,
"tglx@...utronix.de" <tglx@...utronix.de>, "Verma, Vishal L"
<vishal.l.verma@...el.com>, "nik.borisov@...e.com" <nik.borisov@...e.com>,
"mingo@...hat.com" <mingo@...hat.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>,
"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>, "Williams, Dan J"
<dan.j.williams@...el.com>, "bp@...en8.de" <bp@...en8.de>
Subject: Re: [PATCH v3 13/26] x86/virt/seamldr: Allocate and populate a
module update request
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. 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? 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
}
>
>> +
>> +/*
>> + * Intel TDX Module blob. Its format is defined at:
>> + * https://github.com/intel/tdx-module-binaries/blob/main/blob_structure.txt
>> + */
>> +struct tdx_blob {
>> + u16 version;
>> + u16 checksum;
>> + u32 offset_of_module;
>> + u8 signature[8];
>> + u32 len;
>> + u32 resv1;
>> + u64 resv2[509];
>
>Nit: Perhaps s/resv/rsvd ?
>
Sure. Will do.
>"#grep rsvd arch/x86 -Rn" gave me a bunch of results but "#grep resv" gave
>me much less (and part of the results were 'resvd' and 'resv_xx' instead of
>plain 'resv').
>
>> + u8 data[];
>> +} __packed;
>
>For this structure, I need to click the link and open it in a browser to
>understand where is the sigstruct and module, and ...
>
>> +static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
>> +{
>> + const struct tdx_blob *blob = (const void *)data;
>> + int module_size, sig_size;
>> + const void *sig, *module;
>> +
>> + if (blob->version != 0x100) {
>> + pr_err("unsupported blob version: %x\n", blob->version);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + if (blob->resv1 || memchr_inv(blob->resv2, 0, sizeof(blob->resv2))) {
>> + pr_err("non-zero reserved fields\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + /* Split the given blob into a sigstruct and a module */
>> + sig = blob->data;
>> + sig_size = blob->offset_of_module - sizeof(struct tdx_blob);
>> + module = data + blob->offset_of_module;
>> + module_size = size - blob->offset_of_module;
>> +
>
>... to see whether this code makes sense.
>
>I understand the
>
> ...
> u64 rsvd[N*512];
> u8 module[];
>
>is painful to be declared explicitly in 'struct tdx_blob' because IIUC we
>cannot put two flexible array members at the end of the structure.
Yes.
>
>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.
>
>> + return alloc_seamldr_params(module, module_size, sig, sig_size);
>> +}
>> +
>
>
>
Powered by blists - more mailing lists