Use gptprio to select boot target #13

csssuf · 2017-08-09T19:52:09Z

This implementation of gptprio is read-only, in that it doesn't update the tries field of gptprio. It also does not pass anything to shim instructing it what to boot; if desired I can add that for this PR.

Before this PR is merged, csssuf/rust-uefi#18 will need to be merged and a commit bumping Cargo.lock will need to be added to this PR.

This silences a warning from cargo.

bgilbert

I think it'd be fine to defer tries and shim handoff to a followup PR.

bgilbert · 2017-08-09T22:38:36Z

src/util/gpt.rs

+        }
+
+        block_io
+            .read_blocks(self.my_lba, 1)


Leaked.

Also, why does this function read the block again rather than validating the data in the struct it was asked to validate?

bgilbert · 2017-08-09T22:38:42Z

src/util/gpt.rs

+                    (self.num_partition_entries * self.sizeof_partition_entry) as usize;
+
+                block_io
+                    .read_bytes(self.partition_entry_lba, partition_entry_table_size)


bgilbert · 2017-08-09T22:46:22Z

src/util/gpt.rs

+pub struct GptDisk<'a> {
+    block_device: &'static BlockIOProtocol,
+    primary_header: &'a GptHeader,
+    alternate_header: &'a GptHeader,


These should be freed on drop.

They are; see lines 43-47. When a struct is dropped, its Drop implementation is run, and then each of its children has their Drop implementation run (see here for details). Since GptDisk itself requires no special logic to drop, it doesn't need a Drop implementation since the Drop for GptHeader will automatically be run for its children.

bgilbert · 2017-08-09T22:50:57Z

src/util/gpt.rs

+                self.block_device
+                    .read_blocks(self.primary_header.alternate_lba, 1)
+                    .and_then(|block| {
+                        self.alternate_header = &*(block.as_ptr() as *const GptHeader);


This should not be happening in a function called validate. If the alternate header needs to be loaded only after the primary has been validated, this function should be renamed or merged into read_from.

bgilbert · 2017-08-09T22:55:07Z

src/util/gpt.rs

+            .read_bytes(
+                self.primary_header.partition_entry_lba,
+                partition_entry_table_size,
+            )


Leaked.

Also, this is duplicated in the validation code. Perhaps read_from should load all of the bits, validate should check them without loading anything itself, and read_from can generate the partition data structures as well. (validate can be split into two parts for this if necessary.)

This actually isn't leaked; read_partitions returns a slice of GptPartitionEntry references, not GptPartitionEntrys, since GPT partition entries are variable length, and therefore cannot be stored directly in a slice that way.

bgilbert · 2017-08-09T23:53:20Z

src/picker.rs

+        .and_then(parent_device_path)
+        .and_then(|parent_path| {
+            util::GptDisk::read_from(parent_path)
+                .and_then(|disk| disk.read_partitions().map(util::gptprio::next))


Why the nesting here?

bgilbert · 2017-08-10T00:04:24Z

src/picker.rs

+    let boot_result = menu::boot_menu(&option_a, &option_b)
+        .and_then(|option| {
+            match option {
+                Some(user_choice) => Ok(user_choice),


Not necessarily a user_choice, since it could have defaulted.

bgilbert · 2017-08-10T00:24:57Z

src/picker.rs

+                Some(user_choice) => Ok(user_choice),
+                None => {
+                    cons.write(
+                        "User chose nothing and no default was set. Can't proceed.\r\n",


Something like no option selected and... would be a bit better.

On boot, determine the partition gptprio specifies, and use it as the default. As part of the boot menu, display the default choice to the user.

csssuf · 2017-08-10T22:34:08Z

@bgilbert Updated. I think the new validate works better and addresses all of your concerns.

bgilbert · 2017-08-11T06:24:29Z

src/util/gpt.rs

+        let alternate_header = unsafe { &mut *(alternate_block.as_mut_ptr() as *mut GptHeader) };
+
+        out.primary_header = primary_header;
+        out.alternate_header = alternate_header;


Since you're not using out before now, it seems that you can defer its creation until after the read_blocks calls and avoid the null_mut initializations.

bgilbert · 2017-08-11T06:36:28Z

src/util/gpt.rs

+        self.alternate_header
+            .validate(self.primary_header.alternate_lba)?;
+
+        let partitions = self.read_partitions_raw()?;


Leaked.

It still seems as though these should be loaded once and stored, rather than loaded in validate and then thrown away.

bgilbert · 2017-08-11T06:56:49Z

src/util/gpt.rs

 }

-impl Drop for GptHeader {
-    fn drop(&mut self) {


This introduces a leak if let alternate_block = ... fails.

Hmm, there's no way for a GptHeader to know how to free itself due to the allocate_pages/allocate_pool split. The best way to get around this is probably to introduce a type wrapping GptHeader that also stores how to free itself.

bgilbert · 2017-08-11T06:58:51Z

src/util/gpt.rs

+        let bs = uefi::get_system_table().boot_services();
+
+        let num_partitions = self.primary_header.num_partition_entries as usize;
+        let partition_entry_table = self.read_partitions_raw()?;


Leaked. On error we can do something about that, and on success... it would be nice if we had a way to clean this up after the return value is dropped.

I believe the caller has an undocumented responsibility to free the returned array.

By splitting up the validation stage into validate_headers() and validate_partitions(), and splitting each of those into a reading and validation stage, leaks are avoided.

bgilbert · 2017-08-12T07:57:27Z

src/util/gpt.rs

+            partitions: &[],
+        };
+
+        unsafe { out.validate_headers()? };


What happens if we fail here and Drop tries to free raw_partitions?

The implementation of free_pages (and free_pool) in rust-uefi ignores failure, so the actual UEFI call will fail, but that failure won't be passed along to picker, so from picker's perspective, everything will be fine.

bgilbert · 2017-08-12T08:04:13Z

src/util/gpt.rs

-            bs.free_pool(self.alternate_header);
-        }
+
+        self.block_device.free_read(self.raw_partitions);


The partitions array is not freed.

bgilbert · 2017-08-12T08:05:43Z

src/util/gpt.rs

+        let primary_block = protocol.read_blocks(1, 1)?;
+        let primary_header = unsafe { &mut *(primary_block.as_mut_ptr() as *mut GptHeader) };
+
+        let alternate_block = protocol.read_blocks(primary_header.alternate_lba, 1)?;


If you spell out the error check here you can avoid the primary_header leak.

csssuf · 2018-01-19T21:23:02Z

Since I have a few more things to change with this PR and there's apparently no way for me to update this one, I'll reopen a new PR once I have things figured out.

csssuf added 4 commits August 8, 2017 12:59

Cargo.toml: specify lib path explicitly

8af1ea2

This silences a warning from cargo.

util/gpt: support reading GPT headers

0b72057

util/gpt: support reading GPT partitions from disk

776d3c8

util/gptprio: add read-only gptprio implementation

6b1e863

csssuf requested a review from bgilbert August 9, 2017 19:52

bgilbert suggested changes Aug 10, 2017

View reviewed changes

csssuf added 4 commits August 10, 2017 11:58

picker: choose default boot option via gptprio

a05bdca

On boot, determine the partition gptprio specifies, and use it as the default. As part of the boot menu, display the default choice to the user.

review: rework header validation design

d1be63b

review: use read_partitions_raw in read_partitions

05b8bfb

review: properly free GptHeader structs

cc53a12

bgilbert reviewed Aug 11, 2017

View reviewed changes

review: split up gpt reading/validation functions

ece55f7

By splitting up the validation stage into validate_headers() and validate_partitions(), and splitting each of those into a reading and validation stage, leaks are avoided.

bgilbert reviewed Aug 12, 2017

View reviewed changes

csssuf closed this Jan 19, 2018

Use gptprio to select boot target #13

Use gptprio to select boot target #13

Uh oh!

Conversation

csssuf commented Aug 9, 2017

Uh oh!

bgilbert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csssuf commented Aug 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csssuf commented Jan 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants