secboot: use half the mem for KDF in AddRecoveryKey #10619

mvo5 · 2021-08-16T15:56:53Z

Instead of benchmarking the KDF parameters for the recovery key
(which takes some time to run) we can also use defaults for the
KDF parameters. The defaults suggested by Chris are "4 iterations"
and half the usable memory. This commit implements the suggestions.

This commit renames total TotalSystemMemory to TotalUsableMemory and also changes the code to take the CmaTotal into account. This is the memory reserved by the "Contiguous Memory Allocator" and it is not usable for normal processes. This kind of memory is used e.g. by the framebuffer of the Raspberry Pi or by DSPs on certain boards.

Instead of benchmarking the KDF parameters for the recovery key (which takes some time to run) we can also use defaults for the KDF parameters. The defaults suggested by Chris are "4 iterations" and half the usable memory. This commit implements the suggestions.

anonymouse64

lgtm, one suggestion about how to avoid the pointer usage just because we can avoid it and I think avoiding it makes the code more readable

anonymouse64 · 2021-08-16T16:22:53Z

secboot/encrypt_sb.go

@@ -57,7 +61,17 @@ func FormatEncryptedDevice(key EncryptionKey, label, node string) error {
 // volume created with FormatEncryptedDevice on the block device given by node.
 // The existing key to the encrypted volume is provided in the key argument.
 func AddRecoveryKey(key EncryptionKey, rkey RecoveryKey, node string) error {


The comment on this function should probably explain the new defaults that are used here

I added a small comment, please let me know if this looks sufficient

a bit hand wavy, but good enough 😄

osutil/meminfo.go

anonymouse64 · 2021-08-16T21:00:18Z

osutil/meminfo.go

+		switch {
+		case strings.HasPrefix(l, "MemTotal:"):
+			p = &memTotal
+		case strings.HasPrefix(l, "CmaTotal:"):


you could do this without the pointer math with a helper function or even an anonymous lambda like this:

func TotalUsableMemory() (totalMem uint64, err error) { f, err := os.Open(procMeminfo) if err != nil { return 0, err } defer f.Close() s := bufio.NewScanner(f) var memTotal, cmaTotal uint64 processLine := func(l string) (uint64, error) { fields := strings.Fields(l) if len(fields) != 3 || fields[2] != "kB" { return 0, fmt.Errorf("cannot process unexpected meminfo entry %q", l) } v, err := strconv.ParseUint(fields[1], 10, 64) if err != nil { return 0, fmt.Errorf("cannot convert memory size value: %v", err) } return v * 1024, nil } for s.Scan() { l := strings.TrimSpace(s.Text()) var err error switch { case strings.HasPrefix(l, "MemTotal:"): memTotal, err = processLine(l) case strings.HasPrefix(l, "CmaTotal:"): cmaTotal, err = processLine(l) } if err != nil { return 0, err } } if err := s.Err(); err != nil { return 0, err } if memTotal == 0 { return 0, fmt.Errorf("cannot determine the total amount of memory in the system from %s", procMeminfo) } return memTotal - cmaTotal, nil }

mardy · 2021-08-17T05:17:42Z

osutil/meminfo.go

+	if memTotal == 0 {
+		return 0, fmt.Errorf("cannot determine the total amount of memory in the system from %s", procMeminfo)
+	}
+	return memTotal - cmaTotal, nil


Does it make sense to add a check to ensure that memTotal >= cmaTotal, since the type of the value returned by this function is unsigned?

I doubt that memInfo would return such bogus values, but...

I wonder what happens with this computation on devices with something like android ION (see the PR making an interface for that), I suppose we will have to find out

mvo5 · 2021-08-17T09:15:30Z

This needs to be re-run with "run-nested" before it its merged. But let's wait for feedback from @chrisccoulson first.

After discussing with Chris and Samuele we updated the KDF memory heuristic so that it takes more parameters in mind. It now considers the usable memory and substracts a hardcoded 384MB that is required to have a working system (a bit of a conservative estiamte) and then takes half of this for the KDF memory.

anonymouse64

some nitpicks, mainly about the fact that there are an awful lot of manual numbers which can sometimes be confusing, if possible I would suggest using unit / types instead so it's more obvious what unit each number is for etc.

secboot/encrypt_sb.go

anonymouse64 · 2021-08-17T19:48:59Z

secboot/encrypt_sb.go

+	kdfMem := (int(usableMem) - 384*1024*1024) / 2
+	// max 1 GB
+	if kdfMem > 1024*1024*1024 {
+		kdfMem = (1024 * 1024 * 1024)
+	}
+	if kdfMem < 32*1024 {
+		kdfMem = 32 * 1024
+	}
+	opts := &sb.KDFOptions{
+		MemoryKiB:       kdfMem / 1024,
+		ForceIterations: 4,
+	}


all of this would be more readable IMHO if we used the gadget.Size type here, but not necessary right now given the time pressure to try this out

anonymouse64 · 2021-08-17T19:52:23Z

osutil/meminfo.go

+//
+// CMA memory is taken up by e.g. the framebuffer on the Raspberry Pi or
+// by DSPs on specific boards.
+func TotalUsableMemory() (totalMem uint64, err error) {


also again ideally this would return a quantity.Size instead of a uint64

secboot/encrypt_sb.go

mvo5 · 2021-08-18T16:19:04Z

I got confirmation that this approach helps and the whole boot time decreased from ~187s to ~165s during the install mode on a reference board we use. So this seems to be good to go.

mvo5 · 2021-08-18T16:44:11Z

Thanks for the excellent comments. I'm inclined to start with this minimal version for 2.51 and then do a followup that moves to using the "quantity" pacakge. WDYT? Especially @pedronis and @chrisccoulson ? Your review would be really appreciated before we get this out.

anonymouse64 · 2021-08-18T17:31:38Z

I'm inclined to start with this minimal version for 2.51 and then do a followup that moves to using the "quantity" pacakge

fine with me

pedronis

two questions, one about the 384 reservation factor we can consider tweaking later on master.

pedronis · 2021-08-19T11:05:32Z

secboot/encrypt_sb.go

+	// we want to use for the KDF. Doing it this way avoids the expensive
+	// benchmark from cryptsetup. The recovery key is already 128bit
+	// strong so we don't need to be super precise here.
+	kdfMem := (int(usableMem) - 384*1024*1024) / 2


do we know how much memory we actually have free on the device during snap-bootstrap and during install mode? 384 is kind of for the full user space run mode? just wondering if it's a bit if it's too pessimistic for here.

It is probably on the high side of pessimism - I agree here. Unfortunately I don't have exact data.

I could maybe add one more estimation - there is MemToal and MemAvailable in /proc/meminfo, I could simply subtract those to get the memory needed. Given that at this point we are on an actual device that runs our base and kernel (in a tmpfs) it would probably a reasonable guess. Capped by [128Mb, 512Mb] maybe just to be on the safe side?

One reason I went a bit more pessimistic is that some architectures are less compact code-wise than others and may need more working memory (iirc that is the case for armhf vs arm64). And as this runs everywhere I wanted to be careful.

as I said, I don't think it's a blocker but we might want to explore a bit more what to do on master later about this

pedronis · 2021-08-19T11:07:40Z

secboot/encrypt_sb_test.go

@@ -54,8 +57,8 @@ func (s *encryptSuite) TestFormatEncryptedDevice(c *C) {
 				MetadataKiBSize:     2048,
 				KeyslotsAreaKiBSize: 2560,
 				KDFOptions: &sb.KDFOptions{
-					MemoryKiB: 32768, TargetDuration: 0,
-					ForceIterations: 4, Parallel: 0,
+					MemoryKiB:       32768,


didn't we discuss that this also should become just 32?

Indeed, I was wondering and it seems it's best if I open a separate PR for this PR is about the recovery key and this change is about the main key. So unless you ffeel differently I do a small extra PR with the 32mb->32kb change for the main key.

I did this now in #10645

that's fine, thanks

pedronis

+1 taking into account the answers to the questions

mvo5 added 2 commits August 16, 2021 17:38

mvo5 requested a review from chrisccoulson August 16, 2021 15:56

anonymouse64 approved these changes Aug 17, 2021

View reviewed changes

mardy reviewed Aug 17, 2021

View reviewed changes

mvo5 added the 🚀 bootspeed label Aug 17, 2021

mvo5 requested a review from anonymouse64 August 17, 2021 18:48

anonymouse64 reviewed Aug 17, 2021

View reviewed changes

mvo5 added 2 commits August 18, 2021 18:40

osutil,secboot: fix typos

6b387de

secboot: add comment about minimum mem

a22313f

mvo5 added this to the 2.51 milestone Aug 18, 2021

mvo5 added the Squash-merge Please squash this PR when merging. label Aug 18, 2021

mvo5 marked this pull request as ready for review August 19, 2021 10:20

pedronis self-requested a review August 19, 2021 10:47

pedronis reviewed Aug 19, 2021

View reviewed changes

pedronis approved these changes Aug 19, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kdf-from-usable-mem

ac6c8df

mvo5 added the Run nested The PR also runs tests inluded in nested suite label Aug 19, 2021

mvo5 closed this Aug 19, 2021

mvo5 reopened this Aug 19, 2021

mvo5 merged commit e331222 into snapcore:master Aug 19, 2021

mvo5 added the cherry-picked label Aug 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

secboot: use half the mem for KDF in AddRecoveryKey #10619

secboot: use half the mem for KDF in AddRecoveryKey #10619

mvo5 commented Aug 16, 2021

anonymouse64 left a comment

anonymouse64 Aug 16, 2021

mvo5 Aug 18, 2021

anonymouse64 Aug 18, 2021

anonymouse64 Aug 16, 2021

mardy Aug 17, 2021

pedronis Aug 19, 2021

mvo5 commented Aug 17, 2021

anonymouse64 left a comment

anonymouse64 Aug 17, 2021

anonymouse64 Aug 17, 2021

mvo5 commented Aug 18, 2021

mvo5 commented Aug 18, 2021

anonymouse64 commented Aug 18, 2021

pedronis left a comment •

edited

pedronis Aug 19, 2021

mvo5 Aug 19, 2021

pedronis Aug 19, 2021 •

edited

pedronis Aug 19, 2021

mvo5 Aug 19, 2021

mvo5 Aug 19, 2021

pedronis Aug 19, 2021

pedronis left a comment

secboot: use half the mem for KDF in AddRecoveryKey #10619

secboot: use half the mem for KDF in AddRecoveryKey #10619

Conversation

mvo5 commented Aug 16, 2021

anonymouse64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 commented Aug 17, 2021

anonymouse64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 commented Aug 18, 2021

mvo5 commented Aug 18, 2021

anonymouse64 commented Aug 18, 2021

pedronis left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Aug 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment •

edited

pedronis Aug 19, 2021 •

edited