Skip to content

Read and measure CBFS files into initrd#357

Merged
osresearch merged 9 commits into
linuxboot:masterfrom
flammit:cbfs-init
Apr 30, 2018
Merged

Read and measure CBFS files into initrd#357
osresearch merged 9 commits into
linuxboot:masterfrom
flammit:cbfs-init

Conversation

@flammit

@flammit flammit commented Mar 12, 2018

Copy link
Copy Markdown
Collaborator

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

flammit commented Mar 13, 2018

Copy link
Copy Markdown
Collaborator Author

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

Comment thread initrd/bin/network-init-recovery Outdated
# TODO: setup DHCP if available
ifconfig eth0 > /dev/ttyprintk

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

Copy link
Copy Markdown
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.

Comment thread initrd/bin/network-init-recovery Outdated
if [ ! -z "$CONFIG_BOOT_STATIC_IP" ]; then
ifconfig eth0 $CONFIG_BOOT_STATIC_IP
fi
# TODO: setup DHCP if available

Copy link
Copy Markdown
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.

Comment thread initrd/bin/network-init-recovery Outdated
fi

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

Copy link
Copy Markdown
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.

Comment thread initrd/bin/network-init-recovery Outdated

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

flammit commented Mar 20, 2018

Copy link
Copy Markdown
Collaborator Author

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
Copy Markdown
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.

@kakaroto kakaroto left a comment

Copy link
Copy Markdown
Contributor

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!

Comment thread patches/flashtools.patch Outdated
+ if (lseek(fd, off, SEEK_SET) == -1) {
+ fprintf(stderr, "Failed to seek to next CBFS offset %lx\n", off);
+ return EXIT_FAILURE;
+ }

Copy link
Copy Markdown
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

Comment thread patches/flashtools.patch Outdated
+ if (read(fd, &file, sizeof(struct cbfs_file)) < 0) {
+ fprintf(stderr, "Failed to read cbfs file header\n");
+ return EXIT_FAILURE;
+ }

Copy link
Copy Markdown
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.

Comment thread patches/flashtools.patch Outdated
+ printf("File name : '%s'\n", name);
+ }
+
+ if (do_list && file.type == 0x50) {

Copy link
Copy Markdown
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.

Comment thread patches/flashtools.patch Outdated
+ uint32_t align = header.align;
+ // loop through files
+ uint64_t off = end - ((uint64_t) header.romsize) + ((uint64_t) header.offset);
+ while (1) {

Copy link
Copy Markdown
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) {

Comment thread patches/flashtools.patch Outdated
+ }
+
+ char *file_data = malloc(file.len);
+ if (read(fd, file_data, file.len) < 0) {

Copy link
Copy Markdown
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.

Comment thread patches/flashtools.patch Outdated
+ }
+
+ size_t name_size = file.offset - sizeof(struct cbfs_file);
+ char *name = calloc(name_size, sizeof(char));

Copy link
Copy Markdown
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.

Comment thread patches/flashtools.patch Outdated
+ if (verbose) {
+ printf("\n-------- End Data\n");
+ }
+ do_read++;

Copy link
Copy Markdown
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 ?

Comment thread patches/flashtools.patch Outdated
+
+ uint64_t end = 0x100000000;
+ if (verbose) {
+ printf("Seeking to %lx\n", end-4);

Copy link
Copy Markdown
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

flammit commented Mar 22, 2018

Copy link
Copy Markdown
Collaborator Author

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

@flammit

flammit commented Mar 23, 2018

Copy link
Copy Markdown
Collaborator Author

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

flammit commented Apr 20, 2018

Copy link
Copy Markdown
Collaborator Author

@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

flammit commented Apr 20, 2018

Copy link
Copy Markdown
Collaborator Author

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

flammit commented Apr 22, 2018

Copy link
Copy Markdown
Collaborator Author

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

Comment thread modules/linuxboot
BOARD:=$(linuxboot_board) \
KERNEL=$(build)/$(BOARD)/bzImage \
INITRD=$(build)/$(BOARD)/initrd.cpio.xz \
CUSTOM=$(CUSTOMPWD) \

Copy link
Copy Markdown
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
@tlaurion tlaurion added the port new board addition from existing coreboot port label Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

port new board addition from existing coreboot port

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GPG key and config should live in separate cbfs file

5 participants