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

Read and measure CBFS files into initrd #357

Merged
merged 9 commits into from
Apr 30, 2018

Conversation

flammit
Copy link
Collaborator

@flammit flammit commented Mar 12, 2018

Closes #182 and depends on PR #355. Allows the user to add files such as keys to the reproducibly-build coreboot.rom via cbfstool as long as there CBFS name is prefixed as initrd/*:

./build/coreboot-4.7/x230/cbfstool ./build/x230/coreboot.rom add -t raw -f ~/.ssh/id_rsa.pub -n "initrd/.ssh/authorized_keys"
./build/coreboot-4.7/x230/cbfstool ./build/x230/coreboot.rom add -t raw -f ~/.gnupg/pubring.gpg -n "initrd/.gnupg/pubring.gpg"
./build/coreboot-4.7/x230/cbfstool ./build/x230/coreboot.rom add -t raw -f ~/.gnupg/secring.gpg -n "initrd/.gnupg/secring.gpg"
./build/coreboot-4.7/x230/cbfstool ./build/x230/coreboot.rom add -t raw -f ~/.gnupg/trustdb.gpg -n "initrd/.gnupg/trustdb.gpg"

Tested on x230.

@flammit flammit changed the title Read and measure CBFS files into initrd WIP: Read and measure CBFS files into initrd Mar 12, 2018
@flammit
Copy link
Collaborator Author

flammit commented Mar 13, 2018

Also successfully tested measurement by altering CBFS file name and data with flashtool on x230 after TOTP seal

# TODO: setup DHCP if available
ifconfig eth0 > /dev/ttyprintk

# Setup the ssh server, allow root logins and log to stderr
Copy link
Contributor

Choose a reason for hiding this comment

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

The verb is spelled with a space Set up.

if [ ! -z "$CONFIG_BOOT_STATIC_IP" ]; then
ifconfig eth0 $CONFIG_BOOT_STATIC_IP
fi
# TODO: setup DHCP if available
Copy link
Contributor

Choose a reason for hiding this comment

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

The verb is spelled with a space Set up.

fi

if [ -e /sys/class/net/eth0 ]; then
# Setup static IP
Copy link
Contributor

Choose a reason for hiding this comment

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

The verb is spelled with a space Set up.


if [ -f /lib/modules/e1000e.ko ]; then
insmod /lib/modules/e1000e.ko
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

The modules are device specific, right? Should that depend on some configuration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are device specific. I could use the variables in module/linux that control the inclusion of the kernel modules (CONFIG_LINUX_E1000, CONFIG_LINUX_E1000E, CONFIG_LINUX_IGB, CONFIG_LINUX_SFC, CONFIG_LINUX_MLX4) and export them in all the board configs, but that's equivalent to testing for existence in the initrd. @osresearch what do you think?

@osresearch
Copy link
Collaborator

I'm so excited to merge this so that we can move the user config and volatile things out of the initrd.

The cbfs reader patch should be sent as a patch to https://github.com/osresearch/flashtools so that we don't have the maintain it as a patch. I hear the owner of that tree accepts PRs.

In QEMU when I last tried to read the ROM from /dev/mem it fails due to something not being mapped. /proc/iomem does not contain physmap-flash, which is likely why it doesn't show up. As a total hack, if I comment out this check in read_mem() in linux-4.9.80/drivers/char/mem.c, qemu is able to dump the contents of the ROM via /dev/mem:

if (!valid_phys_addr_range(p, count))

So it is something in the way that the memory map is being communicated to qemu that prevents us from being able to see the ROM.

We could patch the Heads kernel to allow this, although it would be good to know why it is not communicated.

@flammit
Copy link
Collaborator Author

flammit commented Mar 20, 2018

Confirmed, in QEMU the /dev/mem reads fail in the current cbfs command, but using the busybox devmem tool and your peek work:

~# devmem 0xfffffffc
0xFF900138
~# devmem 0xff900138
0x4342524F

I'll fix cbfs.c up to work in QEMU by using your map_physical before pushing the patch to https://github.com/osresearch/flashtools.

@kakaroto
Copy link
Contributor

I really like this solution! Great work!
flashtools doesn't work for me (on skylake) but I already reviewed the skylake port of flashrom so I should still know what needs to be done for it to work and I'll look into it soon.

Copy link
Contributor

@kakaroto kakaroto left a comment

Choose a reason for hiding this comment

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

Another review will probably happen on the flashtools repo once you send the PR but here's a preliminary one!

+ if (lseek(fd, off, SEEK_SET) == -1) {
+ fprintf(stderr, "Failed to seek to next CBFS offset %lx\n", off);
+ return EXIT_FAILURE;
+ }
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation is wrong here

+ if (read(fd, &file, sizeof(struct cbfs_file)) < 0) {
+ fprintf(stderr, "Failed to read cbfs file header\n");
+ return EXIT_FAILURE;
+ }
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation of this closing bracket is wrong.

+ printf("File name : '%s'\n", name);
+ }
+
+ if (do_list && file.type == 0x50) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The magic number here should be made into a macro.. I'm assuming 0x50 is FILE_TYPE_RAW?
Also, I don't think it's necessary for the specific task the tool was written for but the file type could/should probably be given as argument to the program.

+ uint32_t align = header.align;
+ // loop through files
+ uint64_t off = end - ((uint64_t) header.romsize) + ((uint64_t) header.offset);
+ while (1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of an infinite loop (which breaks when it's outside the bounds of the cbfs), it would be better to do :
while (off < end) {

+ }
+
+ char *file_data = malloc(file.len);
+ if (read(fd, file_data, file.len) < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Verify file_data was allocated correctly.. A corrupted image could cause you to try and allocate GBs of data which would fail.

+ }
+
+ size_t name_size = file.offset - sizeof(struct cbfs_file);
+ char *name = calloc(name_size, sizeof(char));
Copy link
Contributor

Choose a reason for hiding this comment

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

name is allocated but never freed.

+ if (verbose) {
+ printf("\n-------- End Data\n");
+ }
+ do_read++;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe break once the file is read ?

+
+ uint64_t end = 0x100000000;
+ if (verbose) {
+ printf("Seeking to %lx\n", end-4);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you print the file to stdout, all the verbose printfs should probably go to sdterr.

@flammit
Copy link
Collaborator Author

flammit commented Mar 22, 2018

@kakaroto thanks for the comments! I'll incorporate them into the upcoming https://github.com/osresearch/flashtools PR

@flammit
Copy link
Collaborator Author

flammit commented Mar 23, 2018

The cbfs command PR is now up at osresearch/flashtools#3. Please review there and I'll adjust this PR once that is merged.

Also added convenience call to import keys and removed credentials
@flammit flammit changed the title WIP: Read and measure CBFS files into initrd Read and measure CBFS files into initrd Apr 20, 2018
@flammit
Copy link
Collaborator Author

flammit commented Apr 20, 2018

@osresearch PTAL should be good to review and merge. I've tested on qemu-coreboot.

You should now be able to add keys just as:

./build/coreboot-4.7/qemu-coreboot/cbfstool ./build/qemu-coreboot/coreboot.rom add -t raw -f ~/.ssh/id_rsa.pub -n "initrd/.ssh/authorized_keys"
./build/coreboot-4.7/qemu-coreboot/cbfstool ./build/qemu-coreboot/coreboot.rom add -t raw -f ~/keys/49FFCE9C.key -n "initrd/.gnupg/keys/49FFCE9C.key"

Needed to identify which files should be preserved between upgrades
such as "heads/initrd/*" or "heads/counter"
@flammit
Copy link
Collaborator Author

flammit commented Apr 20, 2018

Minor change to require that cbfs file names be prefixed as heads/initrd/*. The idea is that any CBFS file with the prefix heads/ will attempt to be preserved between ROM updates.

./build/coreboot-4.7/qemu-coreboot/cbfstool ./build/qemu-coreboot/coreboot.rom add -t raw -f ~/.ssh/id_rsa.pub -n "heads/initrd/.ssh/authorized_keys"
./build/coreboot-4.7/qemu-coreboot/cbfstool ./build/qemu-coreboot/coreboot.rom add -t raw -f ~/keys/49FFCE9C.key -n "heads/initrd/.gnupg/keys/49FFCE9C.key"

@flammit
Copy link
Collaborator Author

flammit commented Apr 22, 2018

By default, both flashrom-x230.sh and flashrom-kgpe-d16.sh now use preserve_rom in /etc/function to inject previously loaded, non-conflicting heads/* files into new update ROMs prior to flashing.

Tested on x230 and kgpe-d16

@@ -74,4 +79,5 @@ linuxboot.run: $(build)/$(BOARD)/linuxboot.rom
BOARD:=$(linuxboot_board) \
KERNEL=$(build)/$(BOARD)/bzImage \
INITRD=$(build)/$(BOARD)/initrd.cpio.xz \
CUSTOM=$(CUSTOMPWD) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is $CUSTOMPWD?

@osresearch osresearch merged commit c0f3a4b into linuxboot:master Apr 30, 2018
@flammit flammit deleted the cbfs-init branch May 6, 2018 02:00
@tlaurion tlaurion mentioned this pull request Aug 5, 2018
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.

GPG key and config should live in separate cbfs file
4 participants