Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change grub hdd numbering #3256

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

mikem-zed
Copy link
Contributor

@mikem-zed mikem-zed commented May 31, 2023

when grub queries for available disks it doesn't take into account that
the disk can be removable e.g. USB stick. The disk can appear in front of regular
HDDs and the numbering will be different e.g. hd0 become hd1 when the
USB stick is plugged in. It is not a problem for GRUB to find a correct
partition in this case and the system can be booted just fine. However
every command from grub.cfg is measured into PCR8 while being executed
and HDD names appear in those commands e.g. 'set root=(hd2,gpt5)'. If
any key is sealed into TPM using PCR8 then that key cannot be unsealed when a
random USB stick is inserted (or removed if it was inserted when the key
was sealed)

the original issue should not affect PC BIOS case because USB devices
are usually emulated as either CD or floppy drives and have their unique
numbering

Risk assessment:

  1. 1st and 2nd stages of grub are independent applications. No information is passed between these stages e.g. root drive name. The only purpose of stage 1 is to find and chainload stage 2
  2. Stage 2 will find EVE partitions witout knowing how it was loaded by stage 1
  3. If 2 stages have different HDD numbering it will not affect eve booting process, however it will affect PCR contents

so there is no ricks of bricking a device with this fix.

@mikem-zed mikem-zed requested a review from rene May 31, 2023 19:20
@mikem-zed
Copy link
Contributor Author

I'm currently wating for test results. I already have a version where all formating/printfs are cleaned up just did not wanted to push it in case I still need it. But please look at this now and consider all possible consequences of this fix

fi

WORKDIR /grub

RUN (./bootstrap --gnulib-srcdir=/gnulib || ./autogen.sh)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you don't need to spawn a sub shell in a single RUN command, in other words, you can remove those ()

#include <grub/efi/efi.h>
#include <grub/efi/disk.h>

+#define DEBUG_NAMES 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you forgot to remove this enforcement (for debug purposes) or it was intentional? Also, this could not be replaced by some GRUB's debug mechanism, like grub_dprintf()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is why it is marked WIP. I have fixed all these small issues with formating/extra printf but have not pushed. I'm waiting fo test results from CS

@eriknordmark
Copy link
Contributor

WARNING: eve cannot be just updated remotely because the numbering of HDDs will be different in 1st and 2nd stages of GRUB

Do they have to be identical? Then I don't understand how we can deploy this fix. We don't have a robust way to update stage1.

@mikem-zed mikem-zed force-pushed the mikem/grub-hdd-numbering branch from 3a669eb to 8cbd089 Compare June 27, 2023 10:02
@mikem-zed mikem-zed changed the title [WIP] Change grub hdd numbering Change grub hdd numbering Jun 27, 2023
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

I see a comment from @rene which hasn't been addressed/responded to.

From the description is it clear that we can do this in stage2 independently of what stage1 does?

With an old stage1 don't we still have an issue that some PCRs change whether or not a USB drive is connected due to measurements from stage1 grub?

@mikem-zed mikem-zed requested review from eriknordmark and rene June 28, 2023 18:46
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Kick off tests

when grub queries for available disks it doesn't take into account that
the disk can be removable e.g. USB stick. The disk can appear in front of regular
HDDs and the numbering will be different e.g. hd0 become hd1 when the
USB stick is plugged in. It is not a problem for GRUB to find a correct
partition in this case and the system can be booted just fine. However
every command from grub.cfg is measured into PCR8 while being executed
and HDD names appear in those commands e.g. 'set root=(hd2,gpt5)'. If
any key is sealed into TPM using PCR8 then that key cannot be unsealed when a
random USB stick is inserted (or removed if it was inserted when the key
was sealed)

the original issue should not affect PC BIOS case because USB devices
are usually emulated as either CD or floppy drives and have their unique
numbering

Signed-off-by: Mikhail Malyshev <mikem@zededa.com>
When a cloned ./grub repository exists in the pkg/grub
it is taken instead of tar file. autogen.sh must be run in this case
and gawk util is required

Signed-off-by: Mikhail Malyshev <mikem@zededa.com>
@mikem-zed mikem-zed force-pushed the mikem/grub-hdd-numbering branch from 8cbd089 to 79f3e5f Compare June 29, 2023 14:42
@eriknordmark eriknordmark merged commit 12b392c into lf-edge:master Jun 29, 2023
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.

3 participants