Skip to content

defs: use new kernel info structure#30

Merged
dpsmith merged 3 commits intoTrenchBoot:masterfrom
3mdeb:new_kernel
Apr 21, 2020
Merged

defs: use new kernel info structure#30
dpsmith merged 3 commits intoTrenchBoot:masterfrom
3mdeb:new_kernel

Conversation

@krystian-hebel
Copy link
Member

Linux boot protocol versions 2.15+ use separate section to provide static
information about the kernel, see [1]. MLE header offset, previously found
directly in zero_page, is moved to this new structure. This patch searches
for proper address, depending on boot protocol version.

As the old way of providing MLE header offset didn't make it to upstream
kernel, it should be deprecated at some point. It is left for the time
being, as there are some other components still using it.

[1] https://01.org/linuxgraphics/gfx-docs/drm/x86/boot.html

Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com

@krystian-hebel krystian-hebel force-pushed the new_kernel branch 2 times, most recently from e8f8f18 to db8db97 Compare April 9, 2020 10:12
include/boot.h Outdated
#define MLE_UUID1 0x74a7476f
#define MLE_UUID2 0xa2555c0f
#define MLE_UUID3 0x42b651cb
typedef struct __packed mle_header {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this to be packed

@dpsmith
Copy link
Collaborator

dpsmith commented Apr 14, 2020

By the way, I believe this address issue #41.

@dpsmith dpsmith linked an issue Apr 14, 2020 that may be closed by this pull request
Linux boot protocol versions 2.15+ use separate section to provide static
information about the kernel, see [1]. MLE header offset, previously found
directly in zero_page, is moved to this structure. This patch searches for
proper address, located in new kernel_info structure.

[1] https://01.org/linuxgraphics/gfx-docs/drm/x86/boot.html

Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>
main.c Outdated
Comment on lines +225 to +227
if (bp->version < 0x020f
|| ki->header != KERNEL_INFO_HEADER
|| mle_header->uuid[0] != MLE_UUID0
Copy link
Collaborator

@andyhhp andyhhp Apr 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, Clang can (and will) miscompile this (Clang devs argue that it is legal under the C spec). It can logically turn each || into | to aggressively reduce the number of jumps (/basic blocks).

The most efficient (compact) way to write this is:

if (bp->version < 0x020f ||
    (ki = _p(bp->code32_start +
	     bp->kern_info_offset))->header != KERNEL_INFO_HEADER ||
    (mle_header = _p(bp->code32_start +
		     ki->mle_header_offset))->uuid[0] != MLE_UUID0 ||
    ...

which moves the assignment of the ki and mle_header variables until after the sequence point from the preceding || which forces the safety check to be first.

However, we should also do some sanity checks that all offsets from bp->code32_start fall within the kernel image, or a malformed kernel header can cause us to operate on junk. Perhaps we want some inline helpers such as $FOO *get_$FOO_header(...) which do all the sanity checks, so the outside logic can be:

if (bp->version < 0x020f ||
    (ki = get_kern_info_header(bp)) != NULL ||
    (mle_header = get_mle_header(bp, ki)) != NULL)
    ...

Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>
@andyhhp
Copy link
Collaborator

andyhhp commented Apr 18, 2020

The Clang build fails reliably because it thinks mle_header is used while still uninitialised, but doesn't spot that reboot() is a terminal exit. Does an __attribute__((noreturn)) on die() help?

Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>
@dpsmith dpsmith merged commit 89fc411 into TrenchBoot:master Apr 21, 2020
@miczyg1 miczyg1 deleted the new_kernel branch April 22, 2020 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Align structures and handoff with RFC'ed SecureLaunch patchset

4 participants