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]>

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.

https://github.com/TrenchBoot/grub/compare/fdf1960e4eb74f88dd1c6d5a9ef375a5805dfec6..805d4f39d323ab39d5794195a75efb1dcdcf170c#diff-32e991c56790cec87f55280083791c5f9af329a33f9b67cc8998ac8da765c5d4

I'm actually not even sure if this makes any difference for measurements, but it seems to take extra time. I'd rather made loading code zero unused memory if it doesn't do it already

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.

https://github.com/TrenchBoot/grub/compare/fdf1960e4eb74f88dd1c6d5a9ef375a5805dfec6..805d4f39d323ab39d5794195a75efb1dcdcf170c#diff-285279252527f5bc911013189ac09a0880271f60dc8f28f28fe68c2f5d517c74R437

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?

https://github.com/TrenchBoot/grub/compare/fdf1960e4eb74f88dd1c6d5a9ef375a5805dfec6..805d4f39d323ab39d5794195a75efb1dcdcf170c#diff-285279252527f5bc911013189ac09a0880271f60dc8f28f28fe68c2f5d517c74L1236

The comment was copied and maybe code here looked somewhat differently at first.

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.

https://github.com/TrenchBoot/grub/compare/fdf1960e4eb74f88dd1c6d5a9ef375a5805dfec6..805d4f39d323ab39d5794195a75efb1dcdcf170c#diff-b580cbe2a24fcd151d3a26dcb2593b66937420a2cb7ee518043202c5b43a52e7

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.

https://github.com/TrenchBoot/grub/compare/fdf1960e4eb74f88dd1c6d5a9ef375a5805dfec6..805d4f39d323ab39d5794195a75efb1dcdcf170c#diff-8838b20910c42d432b4e57e07f9a31391a72274f313952c9cf29124412b460b4

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))

https://github.com/TrenchBoot/grub/compare/fdf1960e4eb74f88dd1c6d5a9ef375a5805dfec6..805d4f39d323ab39d5794195a75efb1dcdcf170c#diff-beefc3492ec6895088ab2da67a2b0f5888410590ef4f0c02e321904463224a55

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

https://github.com/TrenchBoot/grub/compare/fdf1960e4eb74f88dd1c6d5a9ef375a5805dfec6..805d4f39d323ab39d5794195a75efb1dcdcf170c#diff-aaf00909a549e853f17e08ff0c31aac1519ce76c5097d3f64f698f8f71cd8a9c

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]>

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]>

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]>

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]>

SergiiDmytruk

GitHub: You need to leave a comment indicating the requested changes.
Me: Shut up and post my replies to outdated conversations.

P.S. Private browsing suggests that it did post them.

SergiiDmytruk added 2 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]>

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

krystian-hebel requested changes Nov 28, 2024

View reviewed changes

SergiiDmytruk and others added 24 commits November 29, 2024 19:48

grub-core/loader/slaunch/psp.c: fix rounding up after division

682497d

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

loader/slaunch: fix invalid format specifiers

025f421

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

loader/slaunch/txt.c: FIXME cleanup

8b80806

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

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

e69ea65

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

0231aa9

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

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

561f864

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

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

3fbbb79

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

loader/slaunch: fix reading mle_header

81876ee

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

b883adc

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

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

8a69289

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

4d853d2

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

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

d4b61e3

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

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

b64de8b

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

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

5fc2a42

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

loader/slaunch/slrt.c: add NULL checks

54100f5

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

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

d92ba09

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

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

6e77edd

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

slaunch: introduce a hook for filling SLRT policy

8c705ef

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

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

a655a9e

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

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

8b35409

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

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

805d4f3

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

SergiiDmytruk force-pushed the grub-sl-2.12-v11-amd-no-psp branch from fdf1960 to 805d4f3 Compare November 29, 2024 19:06

SergiiDmytruk commented Nov 29, 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

SergiiDmytruk Nov 29, 2024

krystian-hebel Nov 28, 2024

SergiiDmytruk Nov 29, 2024

krystian-hebel Nov 28, 2024

SergiiDmytruk Nov 29, 2024

krystian-hebel Nov 28, 2024

SergiiDmytruk Nov 29, 2024

krystian-hebel Nov 28, 2024

SergiiDmytruk Nov 29, 2024

krystian-hebel Nov 28, 2024

SergiiDmytruk Nov 29, 2024

krystian-hebel Nov 28, 2024

SergiiDmytruk Nov 29, 2024

SergiiDmytruk left a comment •

edited

Loading

	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

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

SergiiDmytruk left a comment • edited Loading

Choose a reason for hiding this comment

SergiiDmytruk left a comment •

edited

Loading