Changes/fixes on top of v11 AMD patches for non-PSP case #26

SergiiDmytruk · 2024-11-27T13:34:12Z

Significant number commits are meant to be squashed into other commits on the base branch later on.

Just use grub_divmod64() explicitly like all other code does. Signed-off-by: Sergii Dmytruk <[email protected]>

Signed-off-by: Sergii Dmytruk <[email protected]>

The behaviour is implementation specific and in case of GCC 32-bit pointers are **sign-extended** on conversions to a larger integer type, thus producing invalid values. The opposite isn't dangerous, but still generates compiler warnings. Go through `grub_addr_t` in both cases. https://gcc.gnu.org/onlinedocs/gcc/Arrays-and-pointers-implementation.html Signed-off-by: Sergii Dmytruk <[email protected]>

Signed-off-by: Sergii Dmytruk <[email protected]>

Signed-off-by: Michał Żygowski <[email protected]> Signed-off-by: Krystian Hebel <[email protected]> Signed-off-by: Sergii Dmytruk <[email protected]>

Signed-off-by: Sergii Dmytruk <[email protected]>

Only the case of legacy boot has slparams->mle_start and slparams->mle_mem different. Other places are updated for consistency. Signed-off-by: Sergii Dmytruk <[email protected]>

Signed-off-by: Sergii Dmytruk <[email protected]>

Make them more comprehensive and remove the `skl_size < (8*GRUB_PAGE_SIZE)` one which doesn't make much sense. Signed-off-by: Sergii Dmytruk <[email protected]>

GRUB_MULTIBOOT(get_mbi_size) returns an upper bound on MBI's size instead of the actual size. It could also be used for measurements after zeroing unused parts of the buffer, but using an actual size seems like a better option as the same MBI will always have the same hash regardless of the amount of extra memory that follows it. Signed-off-by: Sergii Dmytruk <[email protected]>

Signed-off-by: Sergii Dmytruk <[email protected]>

Not all boot methods are Linux EFI boot methods. Signed-off-by: Sergii Dmytruk <[email protected]>

Signed-off-by: Sergii Dmytruk <[email protected]>

The code makes sure that MBI entry goes first in DRTM, so the payload can measure it first on launch. Then goes SLRT and other typical entries, while MB2 modules are added at the end. Signed-off-by: Michał Żygowski <[email protected]> Signed-off-by: Tomasz Żyjewski <[email protected]> Signed-off-by: Krystian Hebel <[email protected]> Signed-off-by: Sergii Dmytruk <[email protected]>

Just return bool. Signed-off-by: Sergii Dmytruk <[email protected]>

Signed-off-by: Sergii Dmytruk <[email protected]>

…_device() Signed-off-by: Sergii Dmytruk <[email protected]>

krystian-hebel · 2024-11-28T13:53:46Z

grub-core/loader/multiboot_elfxx.c

@@ -148,9 +167,57 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)

      mld->load_base_addr = get_physical_target_address (ch);
      source = get_virtual_current_address (ch);
+
+#ifdef GRUB_USE_MULTIBOOT2
+      grub_memset (get_virtual_current_address (ch), 0, load_size);


Suggested change

grub_memset (get_virtual_current_address (ch), 0, load_size);

grub_memset (source, 0, load_size);

Doing this unconditionally also has performance implication for non-slaunch case, but lets leave it up for upstream to decide whether it should be moved to if below or not.

krystian-hebel · 2024-11-28T14:13:06Z

grub-core/loader/multiboot_mbi2.c

+      /* It's OK to call this for AMD SKINIT because SKL erases the log before use. */
+      if (slparams->platform_type == SLP_INTEL_TXT || slparams->platform_type == SLP_AMD_SKINIT)
+        grub_txt_init_tpm_event_log (get_virtual_current_address (ch),
+                                    slparams->tpm_evt_log_size);


This needs one more space of indentation.

krystian-hebel · 2024-11-28T14:22:00Z

grub-core/loader/multiboot_mbi2.c

+      /*
+       * AMD SKL final setup may relocate the SKL module. It is also what sets the SLRT and DCE
+       * values in slparams so this must be done before final setup and launch below.
+       */
+      err = grub_skl_setup_module (slparams);


But it could be moved to else if (slparams->platform_type == SLP_AMD_SKINIT) below, or am I missing something?

krystian-hebel · 2024-11-28T14:49:49Z

grub-core/loader/slaunch/psp.c

-  tmr_count = tmr_end / drtm_capability.tmr_alignment;
-  if (tmr_end % drtm_capability.tmr_alignment == 1)
+  tmr_count = grub_divmod64 (tmr_end, drtm_capability.tmr_alignment, &rem);
+  if (rem == 1)


I don't understand this part. Shouldn't it be != 0? Having a memory range that ends on 1 byte shouldn't be possible.

krystian-hebel · 2024-11-28T15:20:15Z

grub-core/loader/slaunch/skl.c

-  info = (struct grub_skl_info *) ((grub_uint8_t *) skl_module + skl_module->skl_info_offset);
+  if (module->bootloader_data_offset > max_size)
+    {
+      grub_dprintf ("slaunch", "Possible SKL module has a gap before data: %u > %u\n",


This message doesn't convey the reason for failure. Offset to that data must be <= max_size because this is where SLRT is located. If it isn't, there is not enough space reserved for SLRT, and this is what the message should say.

It would be worth to also check if module->bootloader_data_offset < module->length. Bootloader data shouldn't be measured as part of SL by SKINIT.

krystian-hebel · 2024-11-28T15:39:55Z

grub-core/loader/slaunch/txt.c

+{
+  struct grub_txt_event_log_container *elog;
+
+  if (buf == NULL || size == 0)


Suggested change

if (buf == NULL || size == 0)

if (buf == NULL || size < sizeof (*elog))

krystian-hebel · 2024-11-28T15:45:19Z

include/grub/slaunch.h

@@ -67,6 +71,14 @@ struct grub_slaunch_params
  grub_uint32_t dce_size;
  grub_uint64_t tpm_evt_log_base;
  grub_uint32_t tpm_evt_log_size;
+
+  /*
+   * Can be NULL.  Called before at before starting and after adding standard


Suggested change

* Can be NULL. Called before at before starting and after adding standard

* Can be NULL. Called before starting and after adding standard

SergiiDmytruk and others added 25 commits November 20, 2024 01:22

loader/slaunch/psp.c: don't cause use of __udivmoddi4()

6c173bd

Just use grub_divmod64() explicitly like all other code does. Signed-off-by: Sergii Dmytruk <[email protected]>

grub-core: fix non-EFI build

c9ffe3f

Signed-off-by: Sergii Dmytruk <[email protected]>

loader/slaunch: fix invalid format specifiers

483e488

Signed-off-by: Sergii Dmytruk <[email protected]>

loader/slaunch/txt.c: FIXME cleanup

85da0d8

Signed-off-by: Sergii Dmytruk <[email protected]>

i386/txt: Initialize TPM 1.2 event log in TXT heap

42b2890

Signed-off-by: Michał Żygowski <[email protected]> Signed-off-by: Krystian Hebel <[email protected]> Signed-off-by: Sergii Dmytruk <[email protected]>

loader/slaunch: make SKINIT work on AMD without PSP

b0482bf

Signed-off-by: Sergii Dmytruk <[email protected]>

loader/slaunch: {SKL,SLB}_MIN_A{,L}IGNMENT

8f8d7ca

Signed-off-by: Sergii Dmytruk <[email protected]>

loader/slaunch/skl.c: fix allocating SKL and placing SLRT

9b1f702

Signed-off-by: Sergii Dmytruk <[email protected]>

loader/slaunch: fix reading mle_header

6fd358c

Only the case of legacy boot has slparams->mle_start and slparams->mle_mem different. Other places are updated for consistency. Signed-off-by: Sergii Dmytruk <[email protected]>

loader/slaunch: allow multiple "slaunch_module" commands

3f7d3ff

Signed-off-by: Sergii Dmytruk <[email protected]>

loader/slaunch/skl.c: improve grub_skl_set_module() checks

61df724

Make them more comprehensive and remove the `skl_size < (8*GRUB_PAGE_SIZE)` one which doesn't make much sense. Signed-off-by: Sergii Dmytruk <[email protected]>

loader/slaunch/dlstub.c: better checks/errors

df833ef

Signed-off-by: Sergii Dmytruk <[email protected]>

loader/slaunch/dlstub.c: start with zeroed relocator state

e683437

Signed-off-by: Sergii Dmytruk <[email protected]>

loader/slaunch/skl.c: better errors and checks

20dccfd

Signed-off-by: Sergii Dmytruk <[email protected]>

include/grub/i386/txt.h: add missing #include

8f84783

Signed-off-by: Sergii Dmytruk <[email protected]>

loader/slaunch/slrt.c: add NULL checks

b1afb8a

Not all boot methods are Linux EFI boot methods. Signed-off-by: Sergii Dmytruk <[email protected]>

include/grub/slaunch.h: add missing forward declaration

1fab0b8

Signed-off-by: Sergii Dmytruk <[email protected]>

loader/slaunch/skl.c: fix a comment typo

6c53233

Signed-off-by: Sergii Dmytruk <[email protected]>

slaunch: introduce a hook for filling SLRT policy

72a509a

Signed-off-by: Sergii Dmytruk <[email protected]>

grub-core/loader/slaunch/psp.c: simplify is_drtm_device()

4af101a

Just return bool. Signed-off-by: Sergii Dmytruk <[email protected]>

grub-core/loader/slaunch/psp.c: fix comment capitalization

a714967

Signed-off-by: Sergii Dmytruk <[email protected]>

grub-core/loader/slaunch/psp.c: drop extra assignments from init_drtm…

fdf1960

…_device() Signed-off-by: Sergii Dmytruk <[email protected]>

SergiiDmytruk requested a review from krystian-hebel November 27, 2024 13:34

krystian-hebel requested changes Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes/fixes on top of v11 AMD patches for non-PSP case #26

Changes/fixes on top of v11 AMD patches for non-PSP case #26

SergiiDmytruk commented Nov 27, 2024

krystian-hebel Nov 28, 2024

krystian-hebel Nov 28, 2024

krystian-hebel Nov 28, 2024

krystian-hebel Nov 28, 2024

krystian-hebel Nov 28, 2024

krystian-hebel Nov 28, 2024

krystian-hebel Nov 28, 2024

krystian-hebel Nov 28, 2024

	grub_memset (get_virtual_current_address (ch), 0, load_size);
	grub_memset (source, 0, load_size);

	if (buf == NULL \|\| size == 0)
	if (buf == NULL \|\| size < sizeof (*elog))

	* Can be NULL. Called before at before starting and after adding standard
	* Can be NULL. Called before starting and after adding standard

Changes/fixes on top of v11 AMD patches for non-PSP case #26

Are you sure you want to change the base?

Changes/fixes on top of v11 AMD patches for non-PSP case #26

Conversation

SergiiDmytruk commented Nov 27, 2024

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

Choose a reason for hiding this comment