From 9dc3ca76c3406568f45fa7f886aaa5cb7bf5d7c9 Mon Sep 17 00:00:00 2001 From: Christian Walter Date: Sun, 1 Nov 2026 23:01:54 +0100 Subject: [PATCH 1/2] provision: Handle images with a second RO CBFS With AMD PSP A/B recovery or Intel Top Swap, coreboot builds the RO CBFS twice (COREBOOT, COREBOOT_B) and keeps the metadata hash anchors in the BOOTBLOCK and BOOTBLOCK_B regions. payload replace changed only COREBOOT, and the edit guard rejected the anchor update in BOOTBLOCK, so these images could not be changed at all. Replace the payload in both CBFS copies, let the CBFS commands change both copies and their BOOTBLOCK regions, and list COREBOOT_B in payload list and show. Tested on a Sapphire RAVE2 image (AMD Krackan, PSP A/B): both slots carry the new payload, both metadata hashes are valid and the board boots the new payload. Signed-off-by: Christian Walter --- cmd/cbprov/apply.go | 2 +- cmd/cbprov/payload.go | 4 +- cmd/cbprov/schema.go | 2 +- cmd/cbprov/show.go | 2 +- pkg/cbfs/verify.go | 2 +- pkg/provision/ops.go | 70 +++++++++++++--------- pkg/provision/provision_test.go | 102 +++++++++++++++++++++++++++++--- 7 files changed, 143 insertions(+), 41 deletions(-) diff --git a/cmd/cbprov/apply.go b/cmd/cbprov/apply.go index 6eaa35f..1b8c015 100644 --- a/cmd/cbprov/apply.go +++ b/cmd/cbprov/apply.go @@ -27,7 +27,7 @@ func applyCmd() *cobra.Command { if err != nil { return err } - allowed := []string{"RO_VPD", "RW_VPD", options.Region, provision.ROCBFS} + allowed := append([]string{"RO_VPD", "RW_VPD", options.Region}, provision.CBFSRegions...) return out.edit(args[0], allowed, func(im *image.Image) (provision.Warnings, error) { return provision.Apply(im, spec, filepath.Dir(file), force) }) diff --git a/cmd/cbprov/payload.go b/cmd/cbprov/payload.go index 79590c7..b0921e3 100644 --- a/cmd/cbprov/payload.go +++ b/cmd/cbprov/payload.go @@ -67,7 +67,7 @@ func payloadCmd() *cobra.Command { return err } spec.Data = data - return out.edit(args[0], []string{provision.ROCBFS}, func(im *image.Image) (provision.Warnings, error) { + return out.edit(args[0], provision.CBFSRegions, func(im *image.Image) (provision.Warnings, error) { return provision.ReplacePayload(im, spec) }) }, @@ -95,7 +95,7 @@ type payloadInfo struct { func payloads(im *image.Image) ([]payloadInfo, error) { var out []payloadInfo - for _, region := range []string{provision.ROCBFS, "FW_MAIN_A", "FW_MAIN_B"} { + for _, region := range []string{provision.ROCBFS, "COREBOOT_B", "FW_MAIN_A", "FW_MAIN_B"} { if !im.HasRegion(region) { continue } diff --git a/cmd/cbprov/schema.go b/cmd/cbprov/schema.go index fa5a504..717d478 100644 --- a/cmd/cbprov/schema.go +++ b/cmd/cbprov/schema.go @@ -59,7 +59,7 @@ func schemaCmd() *cobra.Command { if err != nil { return err } - return embedOut.edit(args[0], []string{provision.ROCBFS}, func(im *image.Image) (provision.Warnings, error) { + return embedOut.edit(args[0], provision.CBFSRegions, func(im *image.Image) (provision.Warnings, error) { return provision.EmbedSchema(im, raw) }) }, diff --git a/cmd/cbprov/show.go b/cmd/cbprov/show.go index 828b287..818d354 100644 --- a/cmd/cbprov/show.go +++ b/cmd/cbprov/show.go @@ -211,7 +211,7 @@ func printReport(r showReport) { var rs []string for _, rg := range r.Regions { switch rg.Name { - case "RO_VPD", "RW_VPD", "SMMSTORE", "COREBOOT", "FW_MAIN_A", "FW_MAIN_B": + case "RO_VPD", "RW_VPD", "SMMSTORE", "COREBOOT", "COREBOOT_B", "FW_MAIN_A", "FW_MAIN_B": s := rg.Name + " " + size(uint64(rg.Size)) if rg.Name == "COREBOOT" { s += " (" + size(uint64(r.Image.CBFSFree)) + " free)" diff --git a/pkg/cbfs/verify.go b/pkg/cbfs/verify.go index 5dfca99..cca6d70 100644 --- a/pkg/cbfs/verify.go +++ b/pkg/cbfs/verify.go @@ -23,7 +23,7 @@ type anchorContainer struct { var anchorContainers = []anchorContainer{ {cbfs: "COREBOOT", bootblock: "BOOTBLOCK"}, - {cbfs: "COREBOOT_B", bootblock: "BOOTBLOCK_B", secondary: true}, // Intel Top Swap + {cbfs: "COREBOOT_B", bootblock: "BOOTBLOCK_B", secondary: true}, // Intel Top Swap, AMD PSP A/B recovery } // anchor is a struct metadata_hash_anchor inside the image data. diff --git a/pkg/provision/ops.go b/pkg/provision/ops.go index 0d6732c..2b17fbc 100644 --- a/pkg/provision/ops.go +++ b/pkg/provision/ops.go @@ -19,6 +19,11 @@ import ( // ROCBFS is the FMAP region of the read-only CBFS. const ROCBFS = "COREBOOT" +// CBFSRegions are the FMAP regions a CBFS change may touch: the RO CBFS, its +// copy COREBOOT_B (Intel Top Swap, AMD PSP A/B recovery) and the BOOTBLOCK +// regions holding their metadata hash anchors. +var CBFSRegions = []string{ROCBFS, "COREBOOT_B", "BOOTBLOCK", "BOOTBLOCK_B"} + // KV is an ordered key/value pair from the command line or a provisioning file. type KV struct { Key string @@ -192,7 +197,7 @@ type PayloadSpec struct { // ErrSigned is returned when a change would break a signature cbprov cannot renew. var ErrSigned = errors.New("image is signed") -// ReplacePayload swaps a payload in the RO CBFS. +// ReplacePayload swaps a payload in the RO CBFS and its COREBOOT_B copy. func ReplacePayload(im *image.Image, p PayloadSpec) (Warnings, error) { var w Warnings if p.Name == "" { @@ -205,53 +210,62 @@ func ReplacePayload(im *image.Image, p PayloadSpec) (Warnings, error) { if ver.Vboot && !p.Force { return nil, fmt.Errorf("%w: vboot RW slots (FW_MAIN_A/B) present; payload changes need re-signing, which cbprov does not do yet (--force changes the RO CBFS only)", ErrSigned) } - fs, err := cbfs.Open(im, ROCBFS) + // Only the ELF magic is checked: cbfstool accepts ELFs with broken section + // headers and so does the SELF converter, which reads program headers only. + if !p.Flat && !bytes.HasPrefix(p.Data, []byte(elf.ELFMAG)) { + return nil, errors.New("payload is not an ELF file (use --flat with --load and --entry for raw binaries)") + } + // coreboot adds the payload to every RO CBFS (CBFS_REGIONS). + regions := []string{ROCBFS} + if im.HasRegion("COREBOOT_B") { + regions = append(regions, "COREBOOT_B") + } + for _, region := range regions { + if err := replacePayloadIn(im, region, p, &w); err != nil { + return nil, err + } + } + if ver.MetadataHash { + if err := cbfs.UpdateMetadataHash(im); err != nil { + return nil, err + } + w.add("CBFS metadata hash in the bootblock updated; if the bootblock is verified by Boot Guard or the AMD PSP, re-sign the image") + } + if ver.Vboot { + w.add("only the RO CBFS was changed; vboot RW slots still boot the old payload") + } + return w, nil +} + +func replacePayloadIn(im *image.Image, region string, p PayloadSpec, w *Warnings) error { + fs, err := cbfs.Open(im, region) if err != nil { - return nil, err + return err } comp := cbfs.CompressLZMA old, found, err := findFile(fs, p.Name) if err != nil { - return nil, err + return err } if found { comp = PayloadCompression(old) } else { - w.add("%s did not exist; adding it", p.Name) + w.add("%s did not exist in %s; adding it", p.Name, region) } if p.Compression != "" { if comp, err = ParseCompression(p.Compression); err != nil { - return nil, err + return err } } - // Only the ELF magic is checked: cbfstool accepts ELFs with broken section - // headers and so does the SELF converter, which reads program headers only. - if !p.Flat && !bytes.HasPrefix(p.Data, []byte(elf.ELFMAG)) { - return nil, errors.New("payload is not an ELF file (use --flat with --load and --entry for raw binaries)") - } if found { if err := fs.Remove(p.Name); err != nil { - return nil, err + return err } } if p.Flat { - err = fs.AddPayloadFlat(p.Name, p.Data, p.LoadAddr, p.Entry, comp) - } else { - err = fs.AddPayloadELF(p.Name, p.Data, comp) - } - if err != nil { - return nil, err - } - if ver.MetadataHash { - if err := cbfs.UpdateMetadataHash(im); err != nil { - return nil, err - } - w.add("CBFS metadata hash in the bootblock updated; if the bootblock is verified by Boot Guard or the AMD PSP, re-sign the image") - } - if ver.Vboot { - w.add("only the RO CBFS was changed; vboot RW slots still boot the old payload") + return fs.AddPayloadFlat(p.Name, p.Data, p.LoadAddr, p.Entry, comp) } - return w, nil + return fs.AddPayloadELF(p.Name, p.Data, comp) } // EmbedSchema stores a CFR schema in the RO CBFS so later commands find it. diff --git a/pkg/provision/provision_test.go b/pkg/provision/provision_test.go index fb94a58..64ebb1c 100644 --- a/pkg/provision/provision_test.go +++ b/pkg/provision/provision_test.go @@ -2,12 +2,15 @@ package provision import ( "bytes" + "encoding/binary" "os" "path/filepath" "testing" "github.com/linuxboot/fiano/pkg/fmap" + "github.com/9elements/cbprov/pkg/buildconfig" + "github.com/9elements/cbprov/pkg/cbfs" "github.com/9elements/cbprov/pkg/efivars" "github.com/9elements/cbprov/pkg/image" "github.com/9elements/cbprov/pkg/options" @@ -27,14 +30,14 @@ func (w *sliceWriter) Write(p []byte) (int, error) { func (w *sliceWriter) Seek(off int64, _ int) (int64, error) { w.off = off; return off, nil } -// testImage has an FMAP, RO_VPD and an erased SMMSTORE but no CBFS. -func testImage(t *testing.T) *image.Image { +type testArea struct { + name string + off, size uint32 +} + +func buildImage(t *testing.T, size int, areas []testArea) *image.Image { t.Helper() - data := bytes.Repeat([]byte{0xff}, 512*1024) - areas := []struct { - name string - off, size uint32 - }{{"FMAP", 0x0, 0x1000}, {"RO_VPD", 0x4000, 0x4000}, {"SMMSTORE", 0x40000, 0x40000}} + data := bytes.Repeat([]byte{0xff}, size) f := &fmap.FMap{Header: fmap.Header{VerMajor: 1, VerMinor: 1, Size: uint32(len(data)), NAreas: uint16(len(areas))}} copy(f.Signature[:], "__FMAP__") for _, a := range areas { @@ -52,6 +55,45 @@ func testImage(t *testing.T) *image.Image { return im } +// testImage has an FMAP, RO_VPD and an erased SMMSTORE but no CBFS. +func testImage(t *testing.T) *image.Image { + return buildImage(t, 512*1024, []testArea{{"FMAP", 0x0, 0x1000}, {"RO_VPD", 0x4000, 0x4000}, {"SMMSTORE", 0x40000, 0x40000}}) +} + +// abImage is laid out like an AMD PSP A/B image: two RO CBFS whose metadata +// hash anchors sit in separate BOOTBLOCK regions. config is the CBFS build config. +func abImage(t *testing.T, config string) *image.Image { + t.Helper() + im := buildImage(t, 1<<20, []testArea{{"FMAP", 0x0, 0x1000}, {"BOOTBLOCK", 0x1000, 0x1000}, + {"BOOTBLOCK_B", 0x2000, 0x1000}, {"SMMSTORE", 0x40000, 0x40000}, + {"COREBOOT", 0x80000, 0x40000}, {"COREBOOT_B", 0xc0000, 0x40000}}) + for _, bb := range []string{"BOOTBLOCK", "BOOTBLOCK_B"} { + b, _ := im.RegionData(bb) + copy(b[0x100:], append([]byte("\xadMdtHsh\x15\x00\x00\x00\x02"), make([]byte, 64)...)) // SHA-256 anchor + } + for _, region := range []string{"COREBOOT", "COREBOOT_B"} { + b, _ := im.RegionData(region) + // Empty CBFS as cbfstool creates it: one null file spanning the region. + copy(b, "LARCHIVE") + binary.BigEndian.PutUint32(b[8:], uint32(len(b)-28)) + binary.BigEndian.PutUint32(b[12:], 0xffffffff) + binary.BigEndian.PutUint32(b[16:], 0) + binary.BigEndian.PutUint32(b[20:], 28) + clear(b[24:28]) + fs, err := cbfs.Open(im, region) + if err != nil { + t.Fatal(err) + } + if err := fs.AddRaw(buildconfig.FileName, []byte(config), cbfs.CompressNone); err != nil { + t.Fatal(err) + } + if err := fs.AddPayloadFlat("fallback/payload", []byte("old payload"), 0x100000, 0x100000, cbfs.CompressNone); err != nil { + t.Fatal(err) + } + } + return im +} + func TestSetVPD(t *testing.T) { im := testImage(t) w, err := SetVPD(im, "RO_VPD", []KV{ @@ -168,3 +210,49 @@ func vpdErased(b []byte) bool { _, err := vpd.Decode(b) return err == nil && bytes.Count(b, []byte{0xff}) == len(b) } + +func TestReplacePayloadAB(t *testing.T) { + im := abImage(t, "CONFIG_PSP_AB_RECOVERY=y\n") + orig := bytes.Clone(im.Data) + payload := bytes.Repeat([]byte("new payload "), 1000) + if _, err := ReplacePayload(im, PayloadSpec{Data: payload, Flat: true, LoadAddr: 0x200000, Entry: 0x200000}); err != nil { + t.Fatal(err) + } + for _, region := range []string{"COREBOOT", "COREBOOT_B"} { + fs, _ := cbfs.Open(im, region) + f, found, err := findFile(fs, "fallback/payload") + if err != nil || !found || len(f.Segments) == 0 || f.Segments[0].LoadAddr != 0x200000 { + t.Errorf("%s: payload not replaced: %+v %v", region, f, err) + } + } + var allowed []image.Region + for _, n := range CBFSRegions { + r, _ := im.Region(n) + allowed = append(allowed, r) + } + for i := range orig { + if orig[i] == im.Data[i] { + continue + } + inside := false + for _, r := range allowed { + inside = inside || (uint32(i) >= r.Offset && uint32(i) < r.Offset+r.Size) + } + if !inside { + t.Fatalf("byte 0x%x outside %v changed", i, CBFSRegions) + } + } + for _, bb := range []string{"BOOTBLOCK", "BOOTBLOCK_B"} { + r, _ := im.Region(bb) + if bytes.Equal(orig[r.Offset:r.Offset+r.Size], im.Data[r.Offset:r.Offset+r.Size]) { + t.Errorf("%s: metadata hash not updated", bb) + } + } + check, err := image.Parse(bytes.Clone(im.Data)) + if err != nil { + t.Fatal(err) + } + if err := cbfs.UpdateMetadataHash(check); err != nil || !bytes.Equal(check.Data, im.Data) { + t.Errorf("metadata hashes stale after replace (%v)", err) + } +} From 9d5b71fae9ceac6daa0a2b665be28897ec6e8e58 Mon Sep 17 00:00:00 2001 From: Christian Walter Date: Sun, 1 Nov 2026 23:02:08 +0100 Subject: [PATCH 2/2] provision: Refuse options the image's coreboot does not read option set and apply wrote an EDK2 variable store into SMMSTORE even when coreboot was built without CONFIG_USE_UEFI_VARIABLE_STORE, exited 0 and printed only a schema warning. Nothing reads that store, so the values had no effect. Fail instead when the image's build config is present and the option backend is not set. Signed-off-by: Christian Walter --- pkg/provision/ops.go | 3 +++ pkg/provision/provision_test.go | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/pkg/provision/ops.go b/pkg/provision/ops.go index 2b17fbc..698f829 100644 --- a/pkg/provision/ops.go +++ b/pkg/provision/ops.go @@ -96,6 +96,9 @@ func OpenOptions(im *image.Image, schemaOverride []byte) (*options.Set, string, // CommitOptions writes the option changes back into the image. func CommitOptions(im *image.Image, set *options.Set) error { + if cfg := BuildConfig(im); len(cfg) > 0 && !cfg.Bool("USE_UEFI_VARIABLE_STORE") { + return errors.New("coreboot in this image does not read options from SMMSTORE (CONFIG_USE_UEFI_VARIABLE_STORE is not set)") + } region, err := im.RegionData(options.Region) if err != nil { return err diff --git a/pkg/provision/provision_test.go b/pkg/provision/provision_test.go index 64ebb1c..5da88c2 100644 --- a/pkg/provision/provision_test.go +++ b/pkg/provision/provision_test.go @@ -256,3 +256,25 @@ func TestReplacePayloadAB(t *testing.T) { t.Errorf("metadata hashes stale after replace (%v)", err) } } + +func TestCommitOptionsNeedsBackend(t *testing.T) { + for _, tc := range []struct { + config string + ok bool + }{ + {"CONFIG_SMMSTORE=y\n", false}, + {"CONFIG_SMMSTORE=y\nCONFIG_DRIVERS_EFI_VARIABLE_STORE=y\nCONFIG_USE_UEFI_VARIABLE_STORE=y\n", true}, + } { + im := abImage(t, tc.config) + set, _, err := OpenOptions(im, nil) + if err != nil { + t.Fatal(err) + } + if err := set.SetRaw("boot_delay", 3); err != nil { + t.Fatal(err) + } + if err := CommitOptions(im, set); (err == nil) != tc.ok { + t.Errorf("config %q: err = %v", tc.config, err) + } + } +}