Add support for caculating kernel eventlog digist when using OVMF+QEMU #655

OuyangHang33 · 2024-02-07T10:36:02Z

Fix: #633

Tested with local bzImage binary and the result is matched with the programe output mentioned in #633 .

OuyangHang33 · 2024-02-07T10:37:02Z

@jiazhang0 , would you please review this patch?

jyao1 · 2024-02-19T14:04:39Z

@jiazhang0 do you have time to review this patch?

portersrc · 2024-02-19T16:52:08Z

td-shim-tools/src/bin/td-payload-reference-calculator/main.rs

+
+    // Refer to https://learn.microsoft.com/en-us/windows/win32/debug/pe-format, the NumberOfSections' offset is 2
+    let pe_addr =
+        ((buf[IMAGE_PE_ADDR + 1] as u16) << 8) | (buf[IMAGE_PE_ADDR] as u16 + PE_SIGNATURE_SIZE);


This calculation confused me. This field you're grabbing seems to be e_lfanew. I'm having trouble finding its size. Do you have a link? It may actually be 32 bits (e.g. here or here), but you're taking 16 bits. (Maybe in practice it's only ever 16 bits?) Also, I wasn't sure why you added PE_SIGNATURE_SIZE to this. Don't you just want the value at 0x3c?

Thank you for your correction!

It should be 32 bits, I've modified it.

Here the link for coff-file-header: https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#signature-image-only
I want to get the address of coff-file-header and it starts at pe_addr + PE_SIGNATURE_SIZE. The variables should be renamed, and I've modified it.

Xynnn007

Overall lgtm. btw, if links to the related ovmf and qemu repo are given it would be better

Xynnn007 · 2024-02-20T04:53:23Z

td-shim-tools/src/bin/td-payload-reference-calculator/main.rs

+    Ok(hex::encode(res))
+}
+
+fn get_image_regions(buf: Vec<u8>) -> (usize, Vec<usize>, Vec<usize>) {


Suggested change

fn get_image_regions(buf: Vec<u8>) -> (usize, Vec<usize>, Vec<usize>) {

fn get_image_regions(buf: &[u8]) -> (usize, Vec<usize>, Vec<usize>) {

Use &[u8] than Vec<u8> could help to avoid extra memory copy.

Thank you for your suggestions!
I've added the link and replaced Vec<u8> with &[u8].

I cloned the code from https://github.com/tianocore/edk2-staging/tree/2024-tdvf-ww01.4/OvmfPkg. To build TDVF there are two ways in the document, which one do I need to use?

Also, could you help to give some guides to build the qemu of the intended version? I'd like to do some test.

Also, I am not familiar with boot things. Is the kernel vmlinuz (compressed) specified by qemu, and the TDVF will uncompress and then boot?

To build TDVF there are two ways in the document, which one do I need to use?

Config-B is better, it will extend eventlog to the RTMR or TPM.

could you help to give some guides to build the qemu of the intended version?

I think you can follow the qemu wiki, the latest qemu version should support TDVF. My version is v7.2.0

Is the kernel vmlinuz (compressed) specified by qemu, and the TDVF will uncompress and then boot?

For this question, to be honest, I'm not an expert :(
I think the kernel vmlinuz may be uncompressed by qemu, TDVF just load kernel from qemu and boot. I can do more research and investigation to figure it out :) .

Hi, I use qemu from https://github.com/qemu/qemu.git with tag v7.2.0 and tdvf suggested by you. When I run

qemu-system-x86_64 -machine confidential-guest-support=tdx0 \ -object tdx-guest,id=tdx0 \ -drive if=pflash,format=raw,unit=0,file=./edk2-staging/Build/IntelTdx/DEBUG_GCC5/FV/OVMF_CODE.fd \ -drive if=pflash,format=raw,unit=1,file=./edk2-staging/Build/IntelTdx/DEBUG_GCC5/FV/OVMF_VARS.fd

Error raised

qemu-system-x86_64: -object tdx-guest,id=tdx0: Parameter 'qom-type' does not accept value 'tdx-guest'

What should I do?

Hi @Xynnn007 , I got the message that the qemu in my test env was supplied from internel linux os team and BKC team, and the qemu from https://github.com/qemu/qemu.git don't have tdx-guest support.

Link for qemu-tdx: https://github.com/intel/qemu-tdx/tree/tdx-qemu-upstream
Link for qemu-td-partitioning: https://github.com/intel/qemu-td-partitioning (Note: the qemu-bkc in readme is private repo)

You can get support from above link.

Hi @OuyangHang33 . I tested locally with the qemu and tdvf with the following command

./qemu-system-x86_64 \ -machine confidential-guest-support=tdx0 \ -object tdx-guest,id=tdx0,sept-ve-disable=off \ -drive if=pflash,format=raw,unit=0,file=.../edk2-staging/Build/IntelTdx/DEBUG_GCC5/FV/OVMF_CODE.fd \ -drive if=pflash,format=raw,unit=1,file=.../edk2-staging/Build/IntelTdx/DEBUG_GCC5/FV/OVMF_VARS.fd

and the result is

qemu-system-x86_64: ../system/vl.c:2684: qemu_machine_creation_done: Assertion `machine->cgs->ready' failed. Aborted (core dumped)

Is there anything wrong?

Also, if I use CoCo/Kata's tdx-qemu to run this firmware, I met the error on containerd side

qemu-system-x86_64-tdx-experimental: TDVF metadata doesn't specify TD_HOB location.

Did you test with the TDX supported system? Because I got a training this afternoon, I will try your command and tdx-qemu on my side later and give the result before tomorrow evening.

Yes. It is on tdx machine.

thanks! take your time.

portersrc

Thanks @OuyangHang33 for the previous response. Two other doubts below.

portersrc · 2024-02-20T15:21:51Z

td-shim-tools/src/bin/td-payload-reference-calculator/main.rs

+    // SizeOfRawData       Offset: 16      Size:4
+    // PointerToRawData    Offset: 20      Size:4
+    // SizeOfSection                       Size:40
+    let mut p = IMGAE_HEADERS_ADDR + 8;


Is this the right way to find the start of the Section Table? From your reference link:

the location of the section table is determined by calculating the location of the first byte after the headers. Make sure to use the size of the optional header as specified in the file header.

So I'm assuming you need to grab the SizeOfOptionalHeader described here.

Thanks, I have corrected. It's better to find the start of the Section Table by:
p = coff_file_header_addr + coff_file_header_size + optional_header_size.

portersrc · 2024-02-20T15:25:55Z

td-shim-tools/src/bin/td-payload-reference-calculator/main.rs

+        | ((buf[IMAGE_PE_OFFSET + 2] as u32) << 16)
+        | ((buf[IMAGE_PE_OFFSET + 1] as u32) << 8)
+        | (buf[IMAGE_PE_OFFSET] as u32) + PE_SIGNATURE_SIZE;
+    let number_of_pecoff_entry = buf[coff_file_header_offset as usize + 2];


The spec indicates this field is 2 bytes. I'm referring to NumberOfSections here. That said, it also reads: "Note that the Windows loader limits the number of sections to 96." So, maybe you're fine.

Yes. It's better to get 2 bytes here. Have corrected.

Xynnn007 · 2024-02-21T07:39:59Z

td-shim-tools/src/bin/td-payload-reference-calculator/main.rs

+    buf[0x228] = 0x00;
+    buf[0x229] = 0x00;
+    buf[0x22A] = 0x02;


It's a stl_p() in https://github.com/qemu/qemu/blob/f48c205fb42be48e2e47b7e1cd9a2802e5ca17b0/hw/i386/x86.c#L962C9-L962C14 which means the dst length is 4 bytes (uint32_t) due to underlying definition https://github.com/qemu/qemu/blob/f48c205fb42be48e2e47b7e1cd9a2802e5ca17b0/include/qemu/bswap.h#L325

Thus we might need to set one more byte here? s.t.

buf[0x22B] = 0x00;

Reasonable, I will set one more byte here.

portersrc · 2024-02-23T16:23:01Z

Thanks @OuyangHang33! One final doubt, because I don't have enough context to understand your qemu_patch function:

Should you be predicating any of those byte assignments based on a protocol value? The code you referenced sets a kernel protocol variable here, and this affects whether or not patches get applied, it seems (e.g. compare your buf[0x210] assignment vs. the predicated one).

Hope that makes sense. My intuition for this question is that the digest will be wrong whenever the protocol version affects these cases.

OuyangHang33 · 2024-02-27T12:19:51Z

Hi @portersrc, based on this 1.2. The Real-Mode Kernel Header, we know that if cmdline_size exsit and is used, then protocol version should be 2.06+. And td-shim needs this cmdline_size in SETUPHEADER:

td-shim/td-shim/src/bin/td-shim/linux/kernel_param.rs

Line 53 in 9b4e454

pub cmdline_size: u32,

td-shim/td-shim/src/bin/td-shim/linux/boot.rs

Line 88 in 9b4e454

params.hdr.cmdline_size = kernel_parameter.len() as u32;

The input kernel protocol version should be 2.06+ (over 0x206). I've added the Note in README and check in code.

Signed-off-by: OuyangHang33 <[email protected]>

OuyangHang33 · 2024-02-29T01:54:34Z

Thank you @portersrc @Xynnn007 !
Do you have any more questions for this PR?

Xynnn007 · 2024-02-29T04:21:05Z

Thank you @portersrc @Xynnn007 !
Do you have any more questions for this PR?

Thanks. Let me do a test to check if it is ok now. Once finished I will add an approve to this PR. Sorry for my busy time, I promise it will be ok until the end of the next week

portersrc · 2024-02-29T04:38:55Z

Thanks for fielding all my questions; yeah looks good to me!

Xynnn007

Great. With great help of @arronwy I test it successfully

$ cargo run -p td-shim-tools --bin td-payload-reference-calculator -- kernel -q --kernel bzImage_6.6 
warning: unused imports: `PKCS1`, `PSS`
  --> library/ring/src/rsa/padding.rs:21:13
   |
21 |     pkcs1::{PKCS1, RSA_PKCS1_SHA256, RSA_PKCS1_SHA384, RSA_PKCS1_SHA512},
   |             ^^^^^
22 |     pss::{PSS, RSA_PSS_SHA256, RSA_PSS_SHA384, RSA_PSS_SHA512},
   |           ^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: `ring` (lib) generated 1 warning (run `cargo fix --lib -p ring` to apply 1 suggestion)
    Finished dev [unoptimized + debuginfo] target(s) in 0.04s
     Running `target/debug/td-payload-reference-calculator kernel -q --kernel bzImage_6.6`
ddb0ae763ef79685a2102128325e6a7f765ef3b602ad22c12e13b97d6f1e44d1dbb4e55fef418003ad37125a14a67a65

while the eventlog parser result

Event Entry:
	RTMR: 2
	Event Type: EV_EFI_BOOT_SERVICES_APPLICATION
	Digest Algorithm: TPM_ALG_SHA384
	Digest: ddb0ae763ef79685a2102128325e6a7f765ef3b602ad22c12e13b97d6f1e44d1dbb4e55fef418003ad37125a14a67a65
	Event Desc: 18e0db790000000080f315010000000000000001000000002a000000000000000403140072f728144ab61e44b8c39ebdd7f893c7040412006b00650072006e0065006c0000007fff0400

OuyangHang33 · 2024-03-07T13:54:29Z

Thank you very much @Xynnn007 and @arronwy!

OuyangHang33 force-pushed the i633 branch from 34d252a to 74ac773 Compare February 7, 2024 13:23

OuyangHang33 closed this Feb 19, 2024

OuyangHang33 reopened this Feb 19, 2024

portersrc reviewed Feb 19, 2024

View reviewed changes

OuyangHang33 force-pushed the i633 branch 2 times, most recently from 6532fe5 to 3d709bd Compare February 20, 2024 01:42

Xynnn007 reviewed Feb 20, 2024

View reviewed changes

OuyangHang33 force-pushed the i633 branch from 3d709bd to fd5cbc0 Compare February 20, 2024 06:11

OuyangHang33 closed this Feb 20, 2024

OuyangHang33 reopened this Feb 20, 2024

portersrc reviewed Feb 20, 2024

View reviewed changes

OuyangHang33 force-pushed the i633 branch from fd5cbc0 to fbe4b1a Compare February 21, 2024 02:28

Xynnn007 reviewed Feb 22, 2024

View reviewed changes

OuyangHang33 force-pushed the i633 branch from fbe4b1a to a807299 Compare February 22, 2024 14:14

OuyangHang33 closed this Feb 22, 2024

OuyangHang33 reopened this Feb 22, 2024

OuyangHang33 force-pushed the i633 branch from a807299 to 9b1eadd Compare February 27, 2024 12:18

OuyangHang33 force-pushed the i633 branch 2 times, most recently from e3e688e to 92fa852 Compare February 27, 2024 12:53

Add support for caculating kernel eventlog digist when using OVMF+QEMU

92fa852

Signed-off-by: OuyangHang33 <[email protected]>

jyao1 approved these changes Mar 4, 2024

View reviewed changes

Xynnn007 approved these changes Mar 7, 2024

View reviewed changes

jyao1 merged commit b2c5ffe into confidential-containers:main Mar 8, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for caculating kernel eventlog digist when using OVMF+QEMU #655

Add support for caculating kernel eventlog digist when using OVMF+QEMU #655

OuyangHang33 commented Feb 7, 2024 •

edited

Loading

OuyangHang33 commented Feb 7, 2024

jyao1 commented Feb 19, 2024

portersrc Feb 19, 2024

OuyangHang33 Feb 20, 2024

Xynnn007 left a comment

Xynnn007 Feb 20, 2024

OuyangHang33 Feb 20, 2024

Xynnn007 Feb 21, 2024

Xynnn007 Feb 21, 2024

OuyangHang33 Feb 22, 2024 •

edited

Loading

Xynnn007 Feb 23, 2024

OuyangHang33 Feb 28, 2024 •

edited

Loading

Xynnn007 Mar 6, 2024

OuyangHang33 Mar 6, 2024

Xynnn007 Mar 6, 2024

portersrc left a comment

portersrc Feb 20, 2024

OuyangHang33 Feb 21, 2024

portersrc Feb 20, 2024

OuyangHang33 Feb 21, 2024

Xynnn007 Feb 21, 2024

OuyangHang33 Feb 22, 2024

portersrc commented Feb 23, 2024 •

edited

Loading

OuyangHang33 commented Feb 27, 2024 •

edited

Loading

OuyangHang33 commented Feb 29, 2024

Xynnn007 commented Feb 29, 2024

portersrc commented Feb 29, 2024

Xynnn007 left a comment •

edited

Loading

OuyangHang33 commented Mar 7, 2024 •

edited

Loading

	fn get_image_regions(buf: Vec<u8>) -> (usize, Vec<usize>, Vec<usize>) {
	fn get_image_regions(buf: &[u8]) -> (usize, Vec<usize>, Vec<usize>) {

Add support for caculating kernel eventlog digist when using OVMF+QEMU #655

Add support for caculating kernel eventlog digist when using OVMF+QEMU #655

Conversation

OuyangHang33 commented Feb 7, 2024 • edited Loading

OuyangHang33 commented Feb 7, 2024

jyao1 commented Feb 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xynnn007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OuyangHang33 Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OuyangHang33 Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

portersrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

portersrc commented Feb 23, 2024 • edited Loading

OuyangHang33 commented Feb 27, 2024 • edited Loading

OuyangHang33 commented Feb 29, 2024

Xynnn007 commented Feb 29, 2024

portersrc commented Feb 29, 2024

Xynnn007 left a comment • edited Loading

Choose a reason for hiding this comment

OuyangHang33 commented Mar 7, 2024 • edited Loading

OuyangHang33 commented Feb 7, 2024 •

edited

Loading

OuyangHang33 Feb 22, 2024 •

edited

Loading

OuyangHang33 Feb 28, 2024 •

edited

Loading

portersrc commented Feb 23, 2024 •

edited

Loading

OuyangHang33 commented Feb 27, 2024 •

edited

Loading

Xynnn007 left a comment •

edited

Loading

OuyangHang33 commented Mar 7, 2024 •

edited

Loading