Re: [PATCH v4 1/2] Build verity images in rootfs Nix derivation

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 7402 bytes --]

Demi Marie Obenour <demiobenour@gmail.com> writes:

> diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile
> index 27a7c689c39bf9bc93b5ba33ce661be7e47b67f1..055185064d84d9450c2076fdeb410b21d00f1d40 100644
> --- a/host/rootfs/Makefile
> +++ b/host/rootfs/Makefile
> @@ -1,12 +1,12 @@
>  # SPDX-License-Identifier: EUPL-1.2+
>  # SPDX-FileCopyrightText: 2021-2024 Alyssa Ross <hi@alyssa.is>
> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
>  
>  .POSIX:
>  
>  include ../../lib/common.mk
>  include file-list.mk
> -
> -dest = build/rootfs.erofs
> +ROOT_FS_DIR = build
>  
>  DIRS = \
>  	dev \
> @@ -46,15 +46,27 @@ FIFOS = etc/s6-linux-init/run-image/service/s6-svscan-log/fifo
>  
>  BUILD_FILES = build/etc/s6-rc
>  
> -$(dest): ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(FILES) $(BUILD_FILES) build/empty build/fifo file-list.mk
> -	set -euo pipefail; \
> +build/verity-timestamp: $(ROOT_FS)
> +	$(VERITYSETUP) format \
> +		--root-hash-file $(ROOT_FS_VERITY_ROOTHASH) \
> +		-- $(ROOT_FS) $(ROOT_FS_VERITY)
> +	# Add trailing newline
> +	echo >> $(ROOT_FS_VERITY_ROOTHASH)

Why do we need to do this?

(Emacs would also rather your comments were not indented, so they're
interpreted by Make as comments rather than being passed on to the
shell.)

> +	touch -- $(ROOT_FS_DIR)/verity-timestamp

This should be build/verity-timestamp (like the rule), or even better $@.

> +
> +# This rule produces three files but Make only (portably)
> +# supports one output per rule.  Instead of resorting to temporary
> +# files, a timestamp file is created as the last step.  The actual
> +# outputs are produced as side-effects.

Is this comment supposed to be on the previous rule?

> +$(ROOT_FS): ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(FILES) $(BUILD_FILES) build/empty build/fifo file-list.mk
> +	mkdir -p $(ROOT_FS_DIR) && \
>  	{ \
>  	    cat $(PACKAGES_FILE) ;\
>  	    for file in $(FILES) $(LINKS); do printf '%s\n%s\n' $$file "$${file#image/}"; done ;\
>  	    for file in $(BUILD_FILES); do printf '%s\n%s\n' $$file $${file#build/}; done ;\
>  	    printf 'build/empty\n%s\n' $(DIRS) ;\
>  	    printf 'build/fifo\n%s\n' $(FIFOS) ;\
> -	} | ../../scripts/make-erofs.sh $@
> +	} | ../../scripts/make-erofs.sh $(ROOT_FS)

Why change this?

>  
>  build/fifo:
>  	mkdir -p build
> @@ -83,25 +95,10 @@ clean:
>  	rm -rf build
>  .PHONY: clean
>  
> -# veritysetup format produces two files, but Make only (portably)
> -# supports one output per rule, so we combine the two outputs then
> -# define two more rules to separate them again.
> -build/rootfs.verity: $(dest)
> -	$(VERITYSETUP) format $(dest) build/rootfs.verity.superblock.tmp \
> -	    | awk -F ':[[:blank:]]*' '$$1 == "Root hash" {print $$2; exit}' \
> -	    > build/rootfs.verity.roothash.tmp
> -	cat build/rootfs.verity.roothash.tmp build/rootfs.verity.superblock.tmp \
> -	    > $@
> -	rm build/rootfs.verity.roothash.tmp build/rootfs.verity.superblock.tmp
> -build/rootfs.verity.roothash: build/rootfs.verity
> -	head -n 1 build/rootfs.verity > $@
> -build/rootfs.verity.superblock: build/rootfs.verity
> -	tail -n +2 build/rootfs.verity > $@
> -
> -build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk build/rootfs.verity.superblock build/rootfs.verity.roothash $(dest)
> +build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk $(ROOT_FS_DIR)/verity-timestamp $(ROOT_FS)

Here you're also still referring to $(ROOT_FS_DIR)/verity-timestamp
rather than build/verity-timestamp.

>  	../../scripts/make-gpt.sh $@.tmp \
> -	    build/rootfs.verity.superblock:verity:$$(../../scripts/format-uuid.sh "$$(dd if=build/rootfs.verity.roothash bs=32 skip=1 count=1 status=none)") \
> -	    $(dest):root:$$(../../scripts/format-uuid.sh "$$(head -c 32 build/rootfs.verity.roothash)")
> +	    $(ROOT_FS)/rootfs.verity.superblock:verity:$$(../../scripts/format-uuid.sh "$$(dd if=$(ROOT_FS_VERITY_ROOTHASH) bs=32 skip=1 count=1 status=none)") \
> +	    $(ROOT_FS)/rootfs:root:$$(../../scripts/format-uuid.sh "$$(head -c 32 $(ROOT_FS_VERITY_ROOTHASH)")

This can't be right, can it?  $(ROOT_FS) is a file.

>  	mv $@.tmp $@
>  
>  debug:
> @@ -111,7 +108,7 @@ debug:
>  	    $(VMLINUX)
>  .PHONY: debug
>  
> -run: build/live.img build/rootfs.verity.roothash
> +run: build/live.img

I'd still prefer we kept the explicit dependency, even though we will
get it via build/live.img as well.

>  	@set -x && \
>  	ext="$$(mktemp build/spectrum-rootfs-extfs.XXXXXXXXXX.img)" && \
>  	truncate -s 10G "$$ext" && \
> @@ -132,7 +129,7 @@ run: build/live.img build/rootfs.verity.roothash
>  	    -device virtconsole,chardev=virtiocon0 \
>  	    -drive file=build/live.img,if=virtio,format=raw,readonly=on \
>  	    -drive file=/proc/self/fd/3,if=virtio,format=raw \
> -	    -append "earlycon console=hvc0 roothash=$$(< build/rootfs.verity.roothash) intel_iommu=on nokaslr" \
> +	    -append "earlycon console=hvc0 roothash=$$(< $(ROOT_FS_VERITY_ROOTHASH)) intel_iommu=on nokaslr" \
>  	    -device virtio-keyboard \
>  	    -device virtio-mouse \
>  	    -device virtio-gpu \
> diff --git a/host/rootfs/default.nix b/host/rootfs/default.nix
> index 0ac70c7c077c0656c5820a5d8b3c7ce0e7c78e54..1578155fa0fb9a4df3fb4884e21ed7d8d8f821dc 100644
> --- a/host/rootfs/default.nix
> +++ b/host/rootfs/default.nix
> @@ -138,7 +138,7 @@ stdenvNoCC.mkDerivation {
>    };
>    sourceRoot = "source/host/rootfs";
>  
> -  nativeBuildInputs = [ erofs-utils spectrum-build-tools s6-rc ];
> +  nativeBuildInputs = [ cryptsetup erofs-utils spectrum-build-tools s6-rc ];
>  
>    env = {
>      PACKAGES = runCommand "packages" {} ''
> @@ -147,7 +147,9 @@ stdenvNoCC.mkDerivation {
>      '';
>    };
>  
> -  makeFlags = [ "dest=$(out)" ];
> +  # The Makefile uses $(ROOT_FS_DIR), not $(dest), so it can share code
> +  # with other Makefiles that also use this variable.
> +  makeFlags = [ "ROOT_FS_DIR=$(out)" ];
>  
>    dontInstall = true;
>  
> diff --git a/host/rootfs/shell.nix b/host/rootfs/shell.nix
> index 1bf61bebf418333624e799cc8ca231f5783206f4..6df2f575fdfc7cdf8067ccfdb5fecaad9f6ea5e6 100644
> --- a/host/rootfs/shell.nix
> +++ b/host/rootfs/shell.nix
> @@ -12,7 +12,7 @@ rootfs.overrideAttrs (
>  
>  {
>    nativeBuildInputs = nativeBuildInputs ++ [
> -    btrfs-progs cryptsetup jq netcat qemu_kvm reuse util-linux
> +    btrfs-progs jq netcat qemu_kvm reuse util-linux
>    ];
>  
>    env = env // {
> diff --git a/lib/common.mk b/lib/common.mk
> index 277c3544036d9a9057f8ba4ad37fe2207548cc59..d1cc4d0514070cc3f418c4d1b7e929abd40d985c 100644
> --- a/lib/common.mk
> +++ b/lib/common.mk
> @@ -11,6 +11,10 @@ GDB = gdb
>  MCOPY = mcopy
>  MKFS_FAT = mkfs.fat
>  MMD = mmd
> +ROOT_FS = $(ROOT_FS_DIR)/rootfs

Would be nice for this to keep its file extension.

> +ROOT_FS_IMAGES = $(ROOT_FS) $(ROOT_FS_VERITY_ROOTHASH) $(ROOT_FS_VERITY)

I'm not sure "IMAGES" makes sense as a name for this.  A verity roothash
is not an image.  ROOT_FS_FILES?

Alternative naming scheme idea, that avoids mistaking ROOT_FS for the
directory like has happened above:

ROOT_FS (for the directory), ROOT_FS_IMAGE, ROOT_FS_VERITY,
ROOT_FS_VERITY_ROOTHASH.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]