bootloader/lk: add support for UC20 lk bootloader with V2 lkenv structs #9695

anonymouse64 · 2020-11-24T23:22:41Z

This commit adds support for the UC20 LittleKernel bootloader to snapd.

The big changes for using V2 are that we now use a secure bootloader set kernel
command line parameter to identify what disk to look for bootloader environment
structures, and that we use different lkenv structs for different role
bootloaders.

Eventually we should make V1 also use a secure bootloader set kernel command
line parameter to identify what disk to look for bootloader environment
structures on too, as using the partition label like this is vulnerable to
attack by attaching a USB disk with the same partition label to the system. I omitted
that change from here since it changes a bunch more tests and I'd like to unblock UC20
ASAP and we can go back and fix up UC16/UC18 later.

anonymouse64 · 2020-11-24T23:49:57Z

The failed mac check is fixed here: #9696

anonymouse64 · 2020-11-25T16:13:09Z

Marking this as ready to review since we have a +1 on the pre-requisite pr of #9675 , but just the last commit is relevant here: 6e98320

This commit adds support for the UC20 LittleKernel bootloader to snapd. The big changes for using V2 are that we now use a secure bootloader set kernel command line parameter to identify what disk to look for bootloader environment structures, and that we use different lkenv structs for different role bootloaders. Eventually we should make V1 also use a secure bootloader set kernel command line parameter to identify what disk to look for bootloader environment structures on too, as using the partition label like this is vulnerable to attack by attaching a USB disk with the same partition label to the system. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…NotExist Previously we would never continue in SetBootVars if the file doesn't exist because we used fmt.Errorf which does not preserve the error type such that os.IsNotExist would return true. Now, we can continue to provide a customized error message from Load(), as well as make SetBootVars continue and just write out a new file even if we can't read the lkenv file in Load(). Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…ootloader-5

bootloader/lk.go

bootloader/lkenv/lkenv_test.go

bootloader/lkenv/lkenv.go

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…err types This is to accommodate a future FindMatching...WithPartLabel. Additionally, an internal refactor to support storing additional properties for partitions when we search a disk is implemented to allow for the additional future method. The error type now is also more versatile to allow specifying what part failed, searching for a partition by filesystem label or by partition label. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

This will enable searching for a partition with a given label on the provided disk. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…t fs labels The partitions with the bootloader structures we care about do not have filesystems on them, and as such they will not have filesystem labels. Instead, they will have partition labels, and as such we should search using the partition label instead. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

@bboozzoo

This simplifies numerous functions to be more clear with the simpler prepare-image time implementation happening first, then the complex runtime cases happening afterwards are not indented as much. Thanks to @bboozzoo for the suggestion. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

@bboozzoo

Thanks to @bboozzoo for the suggestion. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…ents We should be using xerrors.Is() everywhere here, since the error returned is wrapped. However, for specifically RemoveKernelAssets, it's not a bug that we would skip over os.ErrNotExist, but it would result in a more confusing error message - we should try to give better error messages, and returning early with os.ErrNotExist is more useful to the user than the existing error around not being able to find the specified kernel blob. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

@bboozzoo

…KernelAssets Thanks to @bboozzoo for the recommendation. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 · 2020-12-01T00:05:04Z

Marking this blocked as we have 2 problems I need to fix for this PR:

the lkenv fallback handling for uc20 is clearly wrong, we will have env files passed to the lkenv package as something like "/dev/disk/by-partuuid/1234-5678", and the fallback path will be something like "/dev/disk/by-partuuid/5634-2435" since we are using uuids and not simple labels. Probably the straight forward fix for that is to just pass in the expected backup file name when we create the lkenv file.
the rootdir of the lk bootloader is being used incorrectly on uc20, for the ubuntu-boot (i.e. role run mode) bootloader, we will pass in a rootdir of "/run/mnt/ubuntu-boot", which currently results in an effective env file path of /run/mnt/ubuntu-boot/dev/disk/by-partlabel/1234-5678. We weren't catching this in tests because we pass the root dir in as whatever the value of dirs.GlobalRootDir is anyways and so it looked the same, but we should add tests for passing in different root dirs to handle this situation such as boot.InitramfsUbuntuBootDir for example

I am actively working on fixing both problems, the lkenv problem will be fixed with a separate PR though I think.

@bboozzoo

…labels Thanks to @bboozzoo for clarifying this for me. Partition labels are optional on GPT partitions. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

@bboozzoo

Thanks to @bboozzoo for catching this. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…implify Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

@bboozzoo

…dev props Thanks to @bboozzoo for the suggestion. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

@bboozzoo

Thanks to @bboozzoo again for spotting all my typos :-) Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

This will enable searching for a partition with a given label on the provided disk. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…ath+"bak" Merge pull request #9729 from anonymouse64/feature/uc20-lk-bootloader-7 To simplify existing code, use "" as the backup file to mean path+"bak". Also add tests that when specifying a non-empty string as the backup file that we don't use the legacy backup file handling logic. This will resolve the issue we have in #9695 around passing in /dev/disk/by-partuuid/1234 as the primary file and then getting /dev/disk/by-partuuid/1234bak as the backup file.

Go 1.9 os.PathError does not implement Unwrap(), so using xerrors.Is(err,os.ErrNotExist) does not work, as Is() calls Unwrap() in a loop, waiting for either the error returned to be equal to the target error or to be nil, and in the case of Go 1.9, where os.PathError does not implement Unwrap(), then nil is returned and Is() returns false. We can work around this by implementing Unwrap() on our own error type and specifically returning os.ErrNotExist on Go 1.9 when we know the error is actually os.ErrNotExist. This will make the unit tests with Go 1.9 pass. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

For UC20 runtime, the backup file will be a different partition, and since we are using partition uuid symlinks instead of partition labels, we can't just append "bak" to the primary env file, and instead need to manually provide the backup file. Also add a test which validates that the backup env file for run mode on uc20 is handled appropriately and updated. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…d/remove Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 · 2020-12-03T04:33:58Z

This is ready again now

bboozzoo

Left some tiny suggestions, otherwise LGTM

bootloader/lk.go

bboozzoo · 2020-12-03T08:44:54Z

bootloader/lk.go

-	if l.inRuntimeMode {
+	if l.prepareImageTime {
+		// we are preparing image, just extract boot image to bootloader directory
+		logger.Debugf("ExtractKernelAssets handling image prepare")


bit undecided about keeping or dropping this log. If we are to keep it, perhaps we should add some useful information:

logger.Debugf("extract kernel assets from %s to %s", env.GetBootImageName(), l.dir())

yes I left most of these Debugf's that were already there, I agree as they currently exist they are not very useful. I will do a pass over all these leftover messages

ah actually forgot to do this in my most recent update, I will maybe look at these in a followup

done in #9756

bootloader/lkenv/lkenv.go

pedronis

thank you, did a pass, bunch of details comments, some questions. some bits feels a bit awkward and could use some cleaning

bootloader/lkenv/lkenv.go

bootloader/lk.go

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…nwrap() Instead of checking the version of Go we were compiled with which is weird, we can instead just check for the exact thing we know doesn't work, which is that os.PathError doesn't implement Unwrap(), so if it doesn't implement Unwrap(), we know we are in a legacy version of Go and thus should do the manual unwrapping to return ErrNotExist directly. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

This refactors some helper methods to be more clear, namely that envFile becomes envBackstore, and it takes an argument for whether or not it is returning the primary or the backup, and eliminating the specific method for getting the backup file, as well as combining some codepaths in the other helpers to be more easily readable if slightly redundant in terms of actual code execution. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

… sake Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…cmdline It might not be there on older gadget bootloaders which don't set that kernel command line value and as such we will have to fallback to our current behavior. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

pedronis

thanks for the changes, couple more small comments

bootloader/lk.go

The only case where we can really proceed without having an initial env file is at prepare image time when the bootloader env file doesn't exist, as we could be writing an empty/new one. All other cases we need the file to exist as a pre-requisite, if only for accurate error reporting. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

@pedronis

Thanks to @pedronis for the suggestion. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…esent() Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…mdline Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…backup Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

pedronis

thank you, one consideration about the new backup fallback code

pedronis · 2020-12-04T19:38:01Z

bootloader/lk.go

+			return true, nil
+		}
+
+		// if the primary backstore doesn't exist, check the backup storage


looking at it implemented, this bit here is probably right only for RoleSole though, at prepareImageTime we don't expect to rely only on the backup I think

this will be done in a follow-up

done in #9756

…owups bootloader/{lk,lkenv}: followups from #9695

anonymouse64 added the UC20 label Nov 24, 2020

anonymouse64 mentioned this pull request Nov 24, 2020

osutil/disks: allow building on mac os #9696

Merged

anonymouse64 force-pushed the feature/uc20-lk-bootloader-5 branch 2 times, most recently from f96842b to 6e98320 Compare November 25, 2020 16:12

anonymouse64 marked this pull request as ready for review November 25, 2020 16:13

anonymouse64 force-pushed the feature/uc20-lk-bootloader-5 branch from 6e98320 to 1ad7561 Compare November 25, 2020 22:14

anonymouse64 and others added 2 commits November 25, 2020 16:59

Merge remote-tracking branch 'upstream/master' into feature/uc20-lk-b…

1c83b92

…ootloader-5

pedronis self-requested a review November 27, 2020 11:06

bboozzoo reviewed Nov 27, 2020

View reviewed changes

bootloader/lk.go Outdated Show resolved Hide resolved

bootloader/lk.go Outdated Show resolved Hide resolved

bootloader/lk.go Outdated Show resolved Hide resolved

bootloader/lk.go Outdated Show resolved Hide resolved

bootloader/lkenv/lkenv_test.go Show resolved Hide resolved

bboozzoo reviewed Nov 27, 2020

View reviewed changes

bootloader/lkenv/lkenv.go Show resolved Hide resolved

anonymouse64 added 8 commits November 30, 2020 10:08

Merge branch 'master' into feature/uc20-lk-bootloader-5

a2438c6

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

osutil/disks: add FindMatchingPartitionUUIDFromPartLabel to Disk iface

f36c6d0

This will enable searching for a partition with a given label on the provided disk. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

bootloader/lk.go: rename var

74dc349

Thanks to @bboozzoo for the suggestion. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

bootloader/lk.go: move prepare-image check earlier in ExtractRecovery…

9987809

…KernelAssets Thanks to @bboozzoo for the recommendation. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 added the ⛔ Blocked label Nov 30, 2020

anonymouse64 added 6 commits December 1, 2020 10:36

osutil/disks/disks_linux.go: adjust comment about optional partition …

b8f86da

…labels Thanks to @bboozzoo for clarifying this for me. Partition labels are optional on GPT partitions. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

osutil/disks/disks.go: fix typo

4fd0d05

Thanks to @bboozzoo for catching this. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

osutil/disks/disks_linux.go: add some TODOs about things to cleanup/s…

a304490

…implify Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

osutil/disks/disks_linux.go: add comments about the encoding of the u…

7797633

…dev props Thanks to @bboozzoo for the suggestion. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

osutil/disks/mockdisk.go: fix typo

261a5ab

Thanks to @bboozzoo again for spotting all my typos :-) Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

osutil/disks: add FindMatchingPartitionUUIDFromPartLabel to Disk iface

26d7069

This will enable searching for a partition with a given label on the provided disk. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 added 4 commits December 2, 2020 10:02

Merge branch 'master' into feature/uc20-lk-bootloader-5

80c92bd

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

bootloader/lk: add test scaffolding and TODO about recovery system ad…

7da10c7

…d/remove Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 removed the ⛔ Blocked label Dec 3, 2020

anonymouse64 requested a review from bboozzoo December 3, 2020 04:34

bboozzoo approved these changes Dec 3, 2020

View reviewed changes

pedronis reviewed Dec 3, 2020

View reviewed changes

anonymouse64 added 5 commits December 3, 2020 11:24

bootloader/lk.go: adjust comments

23fd1f4

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

bootloader/lkenv/lkenv.go: change XXX to TODO

f9daf3a

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

bootloader/lk: move some function definitions around for conventions'…

12a43a6

… sake Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 mentioned this pull request Dec 3, 2020

bootloader: add check for prepare-image time and more tests validating options #9746

Merged

anonymouse64 added 2 commits December 3, 2020 12:06

bootloader/lkenv/lkenv.go: fix for gofmt 1.9

18ddfb8

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

pedronis reviewed Dec 4, 2020

View reviewed changes

bootloader/lk.go Outdated Show resolved Hide resolved

bootloader/lk.go Outdated Show resolved Hide resolved

bootloader/lk.go Outdated Show resolved Hide resolved

anonymouse64 added 6 commits December 4, 2020 10:08

bootloader/lk.go: rename variable, simplify return statement

f136f75

Thanks to @pedronis for the suggestion. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

bootloader/lk.go: implement TODO about checking the backup file in Pr…

b74ceef

…esent() Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

bootloader/lk.go: re-word awkwardly phrased TODO about using kernel c…

d085669

…mdline Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

Merge branch 'master' into feature/uc20-lk-bootloader-5

86a7aeb

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

bootloader/lk_test.go: add unit test for Present() RoleSole checking …

b609f38

…backup Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

pedronis self-requested a review December 4, 2020 18:56

pedronis approved these changes Dec 4, 2020

View reviewed changes

pedronis merged commit 66ae367 into snapcore:master Dec 4, 2020

anonymouse64 deleted the feature/uc20-lk-bootloader-5 branch December 4, 2020 20:18

mvo5 added a commit that referenced this pull request Dec 8, 2020

Merge pull request #9756 from anonymouse64/feature/lk-bootloader-foll…

ce74e7c

…owups bootloader/{lk,lkenv}: followups from #9695

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootloader/lk: add support for UC20 lk bootloader with V2 lkenv structs #9695

bootloader/lk: add support for UC20 lk bootloader with V2 lkenv structs #9695

anonymouse64 commented Nov 24, 2020 •

edited

anonymouse64 commented Nov 24, 2020

anonymouse64 commented Nov 25, 2020

anonymouse64 commented Dec 1, 2020

anonymouse64 commented Dec 3, 2020

bboozzoo left a comment

bboozzoo Dec 3, 2020

anonymouse64 Dec 3, 2020

anonymouse64 Dec 3, 2020

anonymouse64 Dec 4, 2020

pedronis left a comment

pedronis left a comment

pedronis left a comment

pedronis Dec 4, 2020

pedronis Dec 4, 2020

anonymouse64 Dec 4, 2020

bootloader/lk: add support for UC20 lk bootloader with V2 lkenv structs #9695

bootloader/lk: add support for UC20 lk bootloader with V2 lkenv structs #9695

Conversation

anonymouse64 commented Nov 24, 2020 • edited

anonymouse64 commented Nov 24, 2020

anonymouse64 commented Nov 25, 2020

anonymouse64 commented Dec 1, 2020

anonymouse64 commented Dec 3, 2020

bboozzoo left a comment

Choose a reason for hiding this comment

bboozzoo Dec 3, 2020

Choose a reason for hiding this comment

anonymouse64 Dec 3, 2020

Choose a reason for hiding this comment

anonymouse64 Dec 3, 2020

Choose a reason for hiding this comment

anonymouse64 Dec 4, 2020

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis Dec 4, 2020

Choose a reason for hiding this comment

pedronis Dec 4, 2020

Choose a reason for hiding this comment

anonymouse64 Dec 4, 2020

Choose a reason for hiding this comment

anonymouse64 commented Nov 24, 2020 •

edited