From b662530677855b198e724dd94b9eb4ea4a32db2e Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 8 Nov 2021 16:38:52 -0800 Subject: [PATCH 1/8] fs_mgr: stop allowing the FDE fstab options Since Android 10, new devices have been required to use FBE instead of FDE. Therefore, the FDE code is no longer needed. Make fs_mgr reject fstabs where FDE is enabled. Unfortunately, there is a quirk where the "encryptable" flag (which was originally meant just for FDE) was overloaded to identify adoptable storage volumes. It appears that we have to keep supporting this use case. Therefore, don't reject the "encryptable" flag completely. Instead, just reject "encryptable" when it appears without "voldmanaged", or without "userdata" as its argument. Here are some references for how "encryptable=userdata" is being used to identify adoptable storage volumes: * https://source.android.com/devices/storage/config#adoptable_storage * https://cs.android.com/android/platform/superproject/+/f26c7e9b12e05a6737a96b44bada77232e08ed87:system/vold/main.cpp;l=269 * https://cs.android.com/android/platform/superproject/+/f26c7e9b12e05a6737a96b44bada77232e08ed87:device/google/cuttlefish/shared/config/fstab.f2fs;l=17 * https://cs.android.com/android/platform/superproject/+/f26c7e9b12e05a6737a96b44bada77232e08ed87:device/generic/goldfish/fstab.ranchu;l=7 [ebiggers@: modified from a WIP CL by paulcrowley@] Bug: 191796797 Change-Id: I3c4bbbe549cc6e24607f230fad27ea0d4d35ce09 --- fs_mgr/fs_mgr_fstab.cpp | 40 ++++++++++------- fs_mgr/tests/fs_mgr_test.cpp | 87 ++++++++---------------------------- 2 files changed, 44 insertions(+), 83 deletions(-) diff --git a/fs_mgr/fs_mgr_fstab.cpp b/fs_mgr/fs_mgr_fstab.cpp index 609bd11d7..159be6772 100644 --- a/fs_mgr/fs_mgr_fstab.cpp +++ b/fs_mgr/fs_mgr_fstab.cpp @@ -146,7 +146,7 @@ void ParseMountFlags(const std::string& flags, FstabEntry* entry) { entry->fs_options = std::move(fs_options); } -void ParseFsMgrFlags(const std::string& flags, FstabEntry* entry) { +bool ParseFsMgrFlags(const std::string& flags, FstabEntry* entry) { for (const auto& flag : Split(flags, ",")) { if (flag.empty() || flag == "defaults") continue; std::string arg; @@ -188,10 +188,20 @@ void ParseFsMgrFlags(const std::string& flags, FstabEntry* entry) { #undef CheckFlag // Then handle flags that take an argument. - if (StartsWith(flag, "encryptable=")) { - // The encryptable flag is followed by an = and the location of the keys. + if (flag == "encryptable=userdata") { + // The "encryptable" flag identifies adoptable storage volumes. The + // argument to this flag must be "userdata". + // + // Historical note: this flag was originally meant just for /data, + // to indicate that FDE (full disk encryption) can be enabled. + // Unfortunately, it was also overloaded to identify adoptable + // storage volumes. Today, FDE is no longer supported, leaving only + // the adoptable storage volume meaning for this flag. entry->fs_mgr_flags.crypt = true; - entry->key_loc = arg; + } else if (StartsWith(flag, "encryptable=") || StartsWith(flag, "forceencrypt=") || + StartsWith(flag, "forcefdeorfbe=")) { + LERROR << "flag no longer supported: " << flag; + return false; } else if (StartsWith(flag, "voldmanaged=")) { // The voldmanaged flag is followed by an = and the label, a colon and the partition // number or the word "auto", e.g. voldmanaged=sdcard:3 @@ -235,18 +245,8 @@ void ParseFsMgrFlags(const std::string& flags, FstabEntry* entry) { LWARNING << "Warning: zramsize= flag malformed: " << arg; } } - } else if (StartsWith(flag, "forceencrypt=")) { - // The forceencrypt flag is followed by an = and the location of the keys. - entry->fs_mgr_flags.force_crypt = true; - entry->key_loc = arg; } else if (StartsWith(flag, "fileencryption=")) { ParseFileEncryption(arg, entry); - } else if (StartsWith(flag, "forcefdeorfbe=")) { - // The forcefdeorfbe flag is followed by an = and the location of the keys. Get it and - // return it. - entry->fs_mgr_flags.force_fde_or_fbe = true; - entry->key_loc = arg; - entry->encryption_options = "aes-256-xts:aes-256-cts"; } else if (StartsWith(flag, "max_comp_streams=")) { if (!ParseInt(arg, &entry->max_comp_streams)) { LWARNING << "Warning: max_comp_streams= flag malformed: " << arg; @@ -306,6 +306,13 @@ void ParseFsMgrFlags(const std::string& flags, FstabEntry* entry) { LWARNING << "Warning: unknown flag: " << flag; } } + + if (entry->fs_mgr_flags.crypt && !entry->fs_mgr_flags.vold_managed) { + LERROR << "FDE is no longer supported; 'encryptable' can only be used for adoptable " + "storage"; + return false; + } + return true; } std::string InitAndroidDtDir() { @@ -576,7 +583,10 @@ bool ReadFstabFromFp(FILE* fstab_file, bool proc_mounts, Fstab* fstab_out) { goto err; } - ParseFsMgrFlags(p, &entry); + if (!ParseFsMgrFlags(p, &entry)) { + LERROR << "Error parsing fs_mgr_flags"; + goto err; + } if (entry.fs_mgr_flags.logical) { entry.logical_partition_name = entry.blk_device; diff --git a/fs_mgr/tests/fs_mgr_test.cpp b/fs_mgr/tests/fs_mgr_test.cpp index 94e1abb47..c77874a93 100644 --- a/fs_mgr/tests/fs_mgr_test.cpp +++ b/fs_mgr/tests/fs_mgr_test.cpp @@ -488,18 +488,16 @@ TEST(fs_mgr, ReadFstabFromFile_FsMgrOptions_AllBad) { TemporaryFile tf; ASSERT_TRUE(tf.fd != -1); std::string fstab_contents = R"fs( -source none0 swap defaults encryptable,forceencrypt,fileencryption,forcefdeorfbe,keydirectory,length,swapprio,zramsize,max_comp_streams,reservedsize,eraseblk,logicalblk,sysfs_path,zram_backingdev_size +source none0 swap defaults fileencryption,keydirectory,length,swapprio,zramsize,max_comp_streams,reservedsize,eraseblk,logicalblk,sysfs_path,zram_backingdev_size -source none1 swap defaults encryptable=,forceencrypt=,fileencryption=,keydirectory=,length=,swapprio=,zramsize=,max_comp_streams=,avb=,reservedsize=,eraseblk=,logicalblk=,sysfs_path=,zram_backingdev_size= - -source none2 swap defaults forcefdeorfbe= +source none1 swap defaults fileencryption=,keydirectory=,length=,swapprio=,zramsize=,max_comp_streams=,avb=,reservedsize=,eraseblk=,logicalblk=,sysfs_path=,zram_backingdev_size= )fs"; ASSERT_TRUE(android::base::WriteStringToFile(fstab_contents, tf.path)); Fstab fstab; EXPECT_TRUE(ReadFstabFromFile(tf.path, &fstab)); - ASSERT_LE(3U, fstab.size()); + ASSERT_LE(2U, fstab.size()); auto entry = fstab.begin(); EXPECT_EQ("none0", entry->mount_point); @@ -526,8 +524,6 @@ source none2 swap defaults forcefdeorfbe= EXPECT_EQ("none1", entry->mount_point); { FstabEntry::FsMgrFlags flags = {}; - flags.crypt = true; - flags.force_crypt = true; flags.file_encryption = true; flags.avb = true; EXPECT_TRUE(CompareFlags(flags, entry->fs_mgr_flags)); @@ -546,24 +542,26 @@ source none2 swap defaults forcefdeorfbe= EXPECT_EQ(0, entry->logical_blk_size); EXPECT_EQ("", entry->sysfs_path); EXPECT_EQ(0U, entry->zram_backingdev_size); - entry++; - - // forcefdeorfbe has its own encryption_options defaults, so test it separately. - EXPECT_EQ("none2", entry->mount_point); - { - FstabEntry::FsMgrFlags flags = {}; - flags.force_fde_or_fbe = true; - EXPECT_TRUE(CompareFlags(flags, entry->fs_mgr_flags)); - } - EXPECT_EQ("aes-256-xts:aes-256-cts", entry->encryption_options); - EXPECT_EQ("", entry->key_loc); } -TEST(fs_mgr, ReadFstabFromFile_FsMgrOptions_Encryptable) { +// FDE is no longer supported, so an fstab with FDE enabled should be rejected. +TEST(fs_mgr, ReadFstabFromFile_FsMgrOptions_FDE) { TemporaryFile tf; ASSERT_TRUE(tf.fd != -1); std::string fstab_contents = R"fs( -source none0 swap defaults encryptable=/dir/key +source /data ext4 noatime forceencrypt=footer +)fs"; + ASSERT_TRUE(android::base::WriteStringToFile(fstab_contents, tf.path)); + + Fstab fstab; + EXPECT_FALSE(ReadFstabFromFile(tf.path, &fstab)); +} + +TEST(fs_mgr, ReadFstabFromFile_FsMgrOptions_AdoptableStorage) { + TemporaryFile tf; + ASSERT_TRUE(tf.fd != -1); + std::string fstab_contents = R"fs( +source none0 swap defaults encryptable=userdata,voldmanaged=sdcard:auto )fs"; ASSERT_TRUE(android::base::WriteStringToFile(fstab_contents, tf.path)); @@ -573,11 +571,11 @@ source none0 swap defaults encryptable=/dir/key FstabEntry::FsMgrFlags flags = {}; flags.crypt = true; + flags.vold_managed = true; auto entry = fstab.begin(); EXPECT_EQ("none0", entry->mount_point); EXPECT_TRUE(CompareFlags(flags, entry->fs_mgr_flags)); - EXPECT_EQ("/dir/key", entry->key_loc); } TEST(fs_mgr, ReadFstabFromFile_FsMgrOptions_VoldManaged) { @@ -725,53 +723,6 @@ source none5 swap defaults zramsize=% EXPECT_EQ(0, entry->zram_size); } -TEST(fs_mgr, ReadFstabFromFile_FsMgrOptions_ForceEncrypt) { - TemporaryFile tf; - ASSERT_TRUE(tf.fd != -1); - std::string fstab_contents = R"fs( -source none0 swap defaults forceencrypt=/dir/key -)fs"; - - ASSERT_TRUE(android::base::WriteStringToFile(fstab_contents, tf.path)); - - Fstab fstab; - EXPECT_TRUE(ReadFstabFromFile(tf.path, &fstab)); - ASSERT_LE(1U, fstab.size()); - - auto entry = fstab.begin(); - EXPECT_EQ("none0", entry->mount_point); - - FstabEntry::FsMgrFlags flags = {}; - flags.force_crypt = true; - EXPECT_TRUE(CompareFlags(flags, entry->fs_mgr_flags)); - - EXPECT_EQ("/dir/key", entry->key_loc); -} - -TEST(fs_mgr, ReadFstabFromFile_FsMgrOptions_ForceFdeOrFbe) { - TemporaryFile tf; - ASSERT_TRUE(tf.fd != -1); - std::string fstab_contents = R"fs( -source none0 swap defaults forcefdeorfbe=/dir/key -)fs"; - - ASSERT_TRUE(android::base::WriteStringToFile(fstab_contents, tf.path)); - - Fstab fstab; - EXPECT_TRUE(ReadFstabFromFile(tf.path, &fstab)); - ASSERT_LE(1U, fstab.size()); - - auto entry = fstab.begin(); - EXPECT_EQ("none0", entry->mount_point); - - FstabEntry::FsMgrFlags flags = {}; - flags.force_fde_or_fbe = true; - EXPECT_TRUE(CompareFlags(flags, entry->fs_mgr_flags)); - - EXPECT_EQ("/dir/key", entry->key_loc); - EXPECT_EQ("aes-256-xts:aes-256-cts", entry->encryption_options); -} - TEST(fs_mgr, ReadFstabFromFile_FsMgrOptions_FileEncryption) { TemporaryFile tf; ASSERT_TRUE(tf.fd != -1); From 63fb19532ce726ac7887f071988459088f709e12 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 8 Nov 2021 16:38:53 -0800 Subject: [PATCH 2/8] fs_mgr: remove code that handles FDE Since Android 10, new devices have been required to use FBE instead of FDE. Therefore, the FDE code is no longer needed. Bug: 191796797 Change-Id: I2f29ce5fa61c67325d6eb6cf6693787f8fa8a011 --- fs_mgr/fs_mgr.cpp | 81 ++++------------------------------------- fs_mgr/fs_mgr_roots.cpp | 3 +- 2 files changed, 8 insertions(+), 76 deletions(-) diff --git a/fs_mgr/fs_mgr.cpp b/fs_mgr/fs_mgr.cpp index 07e1e6b08..5350ee036 100644 --- a/fs_mgr/fs_mgr.cpp +++ b/fs_mgr/fs_mgr.cpp @@ -75,9 +75,6 @@ #include "blockdev.h" #include "fs_mgr_priv.h" -#define KEY_LOC_PROP "ro.crypto.keyfile.userdata" -#define KEY_IN_FOOTER "footer" - #define E2FSCK_BIN "/system/bin/e2fsck" #define F2FS_FSCK_BIN "/system/bin/fsck.f2fs" #define MKSWAP_BIN "/system/bin/mkswap" @@ -907,7 +904,7 @@ static bool mount_with_alternatives(const Fstab& fstab, int start_idx, int* end_ << "(): skipping mount due to invalid magic, mountpoint=" << fstab[i].mount_point << " blk_dev=" << realpath(fstab[i].blk_device) << " rec[" << i << "].fs_type=" << fstab[i].fs_type; - mount_errno = EINVAL; // continue bootup for FDE + mount_errno = EINVAL; // continue bootup for metadata encryption continue; } @@ -1005,50 +1002,22 @@ static bool TranslateExtLabels(FstabEntry* entry) { return false; } -static bool needs_block_encryption(const FstabEntry& entry) { - if (android::base::GetBoolProperty("ro.vold.forceencryption", false) && entry.is_encryptable()) - return true; - if (entry.fs_mgr_flags.force_crypt) return true; - if (entry.fs_mgr_flags.crypt) { - // Check for existence of convert_fde breadcrumb file. - auto convert_fde_name = entry.mount_point + "/misc/vold/convert_fde"; - if (access(convert_fde_name.c_str(), F_OK) == 0) return true; - } - if (entry.fs_mgr_flags.force_fde_or_fbe) { - // Check for absence of convert_fbe breadcrumb file. - auto convert_fbe_name = entry.mount_point + "/convert_fbe"; - if (access(convert_fbe_name.c_str(), F_OK) != 0) return true; - } - return false; -} - static bool should_use_metadata_encryption(const FstabEntry& entry) { - return !entry.metadata_key_dir.empty() && - (entry.fs_mgr_flags.file_encryption || entry.fs_mgr_flags.force_fde_or_fbe); + return !entry.metadata_key_dir.empty() && entry.fs_mgr_flags.file_encryption; } // Check to see if a mountable volume has encryption requirements static int handle_encryptable(const FstabEntry& entry) { - // If this is block encryptable, need to trigger encryption. - if (needs_block_encryption(entry)) { - if (umount(entry.mount_point.c_str()) == 0) { - return FS_MGR_MNTALL_DEV_NEEDS_ENCRYPTION; - } else { - PWARNING << "Could not umount " << entry.mount_point << " - allow continue unencrypted"; - return FS_MGR_MNTALL_DEV_NOT_ENCRYPTED; - } - } else if (should_use_metadata_encryption(entry)) { + if (should_use_metadata_encryption(entry)) { if (umount(entry.mount_point.c_str()) == 0) { return FS_MGR_MNTALL_DEV_NEEDS_METADATA_ENCRYPTION; } else { PERROR << "Could not umount " << entry.mount_point << " - fail since can't encrypt"; return FS_MGR_MNTALL_FAIL; } - } else if (entry.fs_mgr_flags.file_encryption || entry.fs_mgr_flags.force_fde_or_fbe) { + } else if (entry.fs_mgr_flags.file_encryption) { LINFO << entry.mount_point << " is file encrypted"; return FS_MGR_MNTALL_DEV_FILE_ENCRYPTED; - } else if (entry.is_encryptable()) { - return FS_MGR_MNTALL_DEV_NOT_ENCRYPTED; } else { return FS_MGR_MNTALL_DEV_NOT_ENCRYPTABLE; } @@ -1056,9 +1025,6 @@ static int handle_encryptable(const FstabEntry& entry) { static void set_type_property(int status) { switch (status) { - case FS_MGR_MNTALL_DEV_MIGHT_BE_ENCRYPTED: - SetProperty("ro.crypto.type", "block"); - break; case FS_MGR_MNTALL_DEV_FILE_ENCRYPTED: case FS_MGR_MNTALL_DEV_IS_METADATA_ENCRYPTED: case FS_MGR_MNTALL_DEV_NEEDS_METADATA_ENCRYPTION: @@ -1532,7 +1498,6 @@ MountAllResult fs_mgr_mount_all(Fstab* fstab, int mount_mode) { // Mounting failed, understand why and retry. wiped = partition_wiped(current_entry.blk_device.c_str()); - bool crypt_footer = false; if (mount_errno != EBUSY && mount_errno != EACCES && current_entry.fs_mgr_flags.formattable && wiped) { // current_entry and attempted_entry point at the same partition, but sometimes @@ -1544,19 +1509,6 @@ MountAllResult fs_mgr_mount_all(Fstab* fstab, int mount_mode) { checkpoint_manager.Revert(¤t_entry); - if (current_entry.is_encryptable() && current_entry.key_loc != KEY_IN_FOOTER) { - unique_fd fd(TEMP_FAILURE_RETRY( - open(current_entry.key_loc.c_str(), O_WRONLY | O_CLOEXEC))); - if (fd >= 0) { - LINFO << __FUNCTION__ << "(): also wipe " << current_entry.key_loc; - wipe_block_device(fd, get_file_size(fd)); - } else { - PERROR << __FUNCTION__ << "(): " << current_entry.key_loc << " wouldn't open"; - } - } else if (current_entry.is_encryptable() && current_entry.key_loc == KEY_IN_FOOTER) { - crypt_footer = true; - } - // EncryptInplace will be used when vdc gives an error or needs to format partitions // other than /data if (should_use_metadata_encryption(current_entry) && @@ -1577,7 +1529,7 @@ MountAllResult fs_mgr_mount_all(Fstab* fstab, int mount_mode) { } } - if (fs_mgr_do_format(current_entry, crypt_footer) == 0) { + if (fs_mgr_do_format(current_entry, false) == 0) { // Let's replay the mount actions. i = top_idx - 1; continue; @@ -1590,27 +1542,8 @@ MountAllResult fs_mgr_mount_all(Fstab* fstab, int mount_mode) { } // mount(2) returned an error, handle the encryptable/formattable case. - if (mount_errno != EBUSY && mount_errno != EACCES && attempted_entry.is_encryptable()) { - if (wiped) { - LERROR << __FUNCTION__ << "(): " << attempted_entry.blk_device << " is wiped and " - << attempted_entry.mount_point << " " << attempted_entry.fs_type - << " is encryptable. Suggest recovery..."; - encryptable = FS_MGR_MNTALL_DEV_NEEDS_RECOVERY; - continue; - } else { - // Need to mount a tmpfs at this mountpoint for now, and set - // properties that vold will query later for decrypting - LERROR << __FUNCTION__ << "(): possibly an encryptable blkdev " - << attempted_entry.blk_device << " for mount " << attempted_entry.mount_point - << " type " << attempted_entry.fs_type; - if (fs_mgr_do_tmpfs_mount(attempted_entry.mount_point.c_str()) < 0) { - ++error_count; - continue; - } - } - encryptable = FS_MGR_MNTALL_DEV_MIGHT_BE_ENCRYPTED; - } else if (mount_errno != EBUSY && mount_errno != EACCES && - should_use_metadata_encryption(attempted_entry)) { + if (mount_errno != EBUSY && mount_errno != EACCES && + should_use_metadata_encryption(attempted_entry)) { if (!call_vdc({"cryptfs", "mountFstab", attempted_entry.blk_device, attempted_entry.mount_point}, nullptr)) { diff --git a/fs_mgr/fs_mgr_roots.cpp b/fs_mgr/fs_mgr_roots.cpp index d2753208b..3e5619bcd 100644 --- a/fs_mgr/fs_mgr_roots.cpp +++ b/fs_mgr/fs_mgr_roots.cpp @@ -125,8 +125,7 @@ bool TryPathMount(FstabEntry* rec, const std::string& mount_pt) { int result = fs_mgr_do_mount_one(*rec, mount_point); if (result == -1 && rec->fs_mgr_flags.formattable) { PERROR << "Failed to mount " << mount_point << "; formatting"; - bool crypt_footer = rec->is_encryptable() && rec->key_loc == "footer"; - if (fs_mgr_do_format(*rec, crypt_footer) != 0) { + if (fs_mgr_do_format(*rec, false) != 0) { PERROR << "Failed to format " << mount_point; return false; } From c953d6eb5f0e80a39642e442d7dff06746910715 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 8 Nov 2021 16:38:53 -0800 Subject: [PATCH 3/8] fs_mgr: remove FDE fields from FstabEntry Remove the now-unused FDE fields from struct FstabEntry. Bug: 191796797 Change-Id: Iab11a1fe86ac9d06beef68dc7e3c543f48ce0ac6 --- fs_mgr/include_fstab/fstab/fstab.h | 9 ++------- fs_mgr/tests/fs_mgr_test.cpp | 4 ---- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/fs_mgr/include_fstab/fstab/fstab.h b/fs_mgr/include_fstab/fstab/fstab.h index d0f32a364..d9c326d42 100644 --- a/fs_mgr/include_fstab/fstab/fstab.h +++ b/fs_mgr/include_fstab/fstab/fstab.h @@ -37,7 +37,6 @@ struct FstabEntry { unsigned long flags = 0; std::string fs_options; std::string fs_checkpoint_opts; - std::string key_loc; std::string metadata_key_dir; std::string metadata_encryption; off64_t length = 0; @@ -60,19 +59,17 @@ struct FstabEntry { struct FsMgrFlags { bool wait : 1; bool check : 1; - bool crypt : 1; + bool crypt : 1; // Now only used to identify adoptable storage volumes bool nonremovable : 1; bool vold_managed : 1; bool recovery_only : 1; bool verify : 1; - bool force_crypt : 1; bool no_emulated_sd : 1; // No emulated sdcard daemon; sd card is the only external // storage. bool no_trim : 1; bool file_encryption : 1; bool formattable : 1; bool slot_select : 1; - bool force_fde_or_fbe : 1; bool late_mount : 1; bool no_fail : 1; bool verify_at_boot : 1; @@ -89,9 +86,7 @@ struct FstabEntry { bool overlayfs_remove_missing_lowerdir : 1; } fs_mgr_flags = {}; - bool is_encryptable() const { - return fs_mgr_flags.crypt || fs_mgr_flags.force_crypt || fs_mgr_flags.force_fde_or_fbe; - } + bool is_encryptable() const { return fs_mgr_flags.crypt; } }; // An Fstab is a collection of FstabEntry structs. diff --git a/fs_mgr/tests/fs_mgr_test.cpp b/fs_mgr/tests/fs_mgr_test.cpp index c77874a93..d631d7a16 100644 --- a/fs_mgr/tests/fs_mgr_test.cpp +++ b/fs_mgr/tests/fs_mgr_test.cpp @@ -193,13 +193,11 @@ bool CompareFlags(FstabEntry::FsMgrFlags& lhs, FstabEntry::FsMgrFlags& rhs) { lhs.vold_managed == rhs.vold_managed && lhs.recovery_only == rhs.recovery_only && lhs.verify == rhs.verify && - lhs.force_crypt == rhs.force_crypt && lhs.no_emulated_sd == rhs.no_emulated_sd && lhs.no_trim == rhs.no_trim && lhs.file_encryption == rhs.file_encryption && lhs.formattable == rhs.formattable && lhs.slot_select == rhs.slot_select && - lhs.force_fde_or_fbe == rhs.force_fde_or_fbe && lhs.late_mount == rhs.late_mount && lhs.no_fail == rhs.no_fail && lhs.verify_at_boot == rhs.verify_at_boot && @@ -505,7 +503,6 @@ source none1 swap defaults fileencryption=,keydirectory=,length=,sw FstabEntry::FsMgrFlags flags = {}; EXPECT_TRUE(CompareFlags(flags, entry->fs_mgr_flags)); } - EXPECT_EQ("", entry->key_loc); EXPECT_EQ("", entry->metadata_key_dir); EXPECT_EQ(0, entry->length); EXPECT_EQ("", entry->label); @@ -528,7 +525,6 @@ source none1 swap defaults fileencryption=,keydirectory=,length=,sw flags.avb = true; EXPECT_TRUE(CompareFlags(flags, entry->fs_mgr_flags)); } - EXPECT_EQ("", entry->key_loc); EXPECT_EQ("", entry->metadata_key_dir); EXPECT_EQ(0, entry->length); EXPECT_EQ("", entry->label); From 4d0c5efac98903fac5be884cc28e15b6c0edd99d Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 8 Nov 2021 16:38:54 -0800 Subject: [PATCH 4/8] fs_mgr: remove crypt_footer argument from fs_mgr_do_format() FDE is no longer supported, so there's no longer any need to ever reserve a crypto footer. Bug: 191796797 Change-Id: I79121188b0bcb7b00c16fda03b68b20c40c1e240 --- fs_mgr/fs_mgr.cpp | 2 +- fs_mgr/fs_mgr_format.cpp | 21 +++++++-------------- fs_mgr/fs_mgr_roots.cpp | 2 +- fs_mgr/include/fs_mgr.h | 2 +- fs_mgr/libsnapshot/snapshot_fuzz_utils.cpp | 2 +- 5 files changed, 11 insertions(+), 18 deletions(-) diff --git a/fs_mgr/fs_mgr.cpp b/fs_mgr/fs_mgr.cpp index 5350ee036..a320c0edb 100644 --- a/fs_mgr/fs_mgr.cpp +++ b/fs_mgr/fs_mgr.cpp @@ -1529,7 +1529,7 @@ MountAllResult fs_mgr_mount_all(Fstab* fstab, int mount_mode) { } } - if (fs_mgr_do_format(current_entry, false) == 0) { + if (fs_mgr_do_format(current_entry) == 0) { // Let's replay the mount actions. i = top_idx - 1; continue; diff --git a/fs_mgr/fs_mgr_format.cpp b/fs_mgr/fs_mgr_format.cpp index 301c90755..bb49873d0 100644 --- a/fs_mgr/fs_mgr_format.cpp +++ b/fs_mgr/fs_mgr_format.cpp @@ -34,7 +34,6 @@ #include #include "fs_mgr_priv.h" -#include "cryptfs.h" using android::base::unique_fd; @@ -58,7 +57,7 @@ static int get_dev_sz(const std::string& fs_blkdev, uint64_t* dev_sz) { } static int format_ext4(const std::string& fs_blkdev, const std::string& fs_mnt_point, - bool crypt_footer, bool needs_projid, bool needs_metadata_csum) { + bool needs_projid, bool needs_metadata_csum) { uint64_t dev_sz; int rc = 0; @@ -68,9 +67,6 @@ static int format_ext4(const std::string& fs_blkdev, const std::string& fs_mnt_p } /* Format the partition using the calculated length */ - if (crypt_footer) { - dev_sz -= CRYPT_FOOTER_OFFSET; - } std::string size_str = std::to_string(dev_sz / 4096); @@ -120,8 +116,8 @@ static int format_ext4(const std::string& fs_blkdev, const std::string& fs_mnt_p return rc; } -static int format_f2fs(const std::string& fs_blkdev, uint64_t dev_sz, bool crypt_footer, - bool needs_projid, bool needs_casefold, bool fs_compress) { +static int format_f2fs(const std::string& fs_blkdev, uint64_t dev_sz, bool needs_projid, + bool needs_casefold, bool fs_compress) { if (!dev_sz) { int rc = get_dev_sz(fs_blkdev, &dev_sz); if (rc) { @@ -130,9 +126,6 @@ static int format_f2fs(const std::string& fs_blkdev, uint64_t dev_sz, bool crypt } /* Format the partition using the calculated length */ - if (crypt_footer) { - dev_sz -= CRYPT_FOOTER_OFFSET; - } std::string size_str = std::to_string(dev_sz / 4096); @@ -159,7 +152,7 @@ static int format_f2fs(const std::string& fs_blkdev, uint64_t dev_sz, bool crypt return logwrap_fork_execvp(args.size(), args.data(), nullptr, false, LOG_KLOG, false, nullptr); } -int fs_mgr_do_format(const FstabEntry& entry, bool crypt_footer) { +int fs_mgr_do_format(const FstabEntry& entry) { LERROR << __FUNCTION__ << ": Format " << entry.blk_device << " as '" << entry.fs_type << "'"; bool needs_casefold = false; @@ -171,10 +164,10 @@ int fs_mgr_do_format(const FstabEntry& entry, bool crypt_footer) { } if (entry.fs_type == "f2fs") { - return format_f2fs(entry.blk_device, entry.length, crypt_footer, needs_projid, - needs_casefold, entry.fs_mgr_flags.fs_compress); + return format_f2fs(entry.blk_device, entry.length, needs_projid, needs_casefold, + entry.fs_mgr_flags.fs_compress); } else if (entry.fs_type == "ext4") { - return format_ext4(entry.blk_device, entry.mount_point, crypt_footer, needs_projid, + return format_ext4(entry.blk_device, entry.mount_point, needs_projid, entry.fs_mgr_flags.ext_meta_csum); } else { LERROR << "File system type '" << entry.fs_type << "' is not supported"; diff --git a/fs_mgr/fs_mgr_roots.cpp b/fs_mgr/fs_mgr_roots.cpp index 3e5619bcd..2ad8125e7 100644 --- a/fs_mgr/fs_mgr_roots.cpp +++ b/fs_mgr/fs_mgr_roots.cpp @@ -125,7 +125,7 @@ bool TryPathMount(FstabEntry* rec, const std::string& mount_pt) { int result = fs_mgr_do_mount_one(*rec, mount_point); if (result == -1 && rec->fs_mgr_flags.formattable) { PERROR << "Failed to mount " << mount_point << "; formatting"; - if (fs_mgr_do_format(*rec, false) != 0) { + if (fs_mgr_do_format(*rec) != 0) { PERROR << "Failed to format " << mount_point; return false; } diff --git a/fs_mgr/include/fs_mgr.h b/fs_mgr/include/fs_mgr.h index 21c998946..5ae4422bd 100644 --- a/fs_mgr/include/fs_mgr.h +++ b/fs_mgr/include/fs_mgr.h @@ -107,7 +107,7 @@ bool fs_mgr_update_logical_partition(android::fs_mgr::FstabEntry* entry); // device is in "check_at_most_once" mode. bool fs_mgr_verity_is_check_at_most_once(const android::fs_mgr::FstabEntry& entry); -int fs_mgr_do_format(const android::fs_mgr::FstabEntry& entry, bool reserve_footer); +int fs_mgr_do_format(const android::fs_mgr::FstabEntry& entry); #define FS_MGR_SETUP_VERITY_SKIPPED (-3) #define FS_MGR_SETUP_VERITY_DISABLED (-2) diff --git a/fs_mgr/libsnapshot/snapshot_fuzz_utils.cpp b/fs_mgr/libsnapshot/snapshot_fuzz_utils.cpp index acee2f4f7..54c6a0053 100644 --- a/fs_mgr/libsnapshot/snapshot_fuzz_utils.cpp +++ b/fs_mgr/libsnapshot/snapshot_fuzz_utils.cpp @@ -488,7 +488,7 @@ std::unique_ptr SnapshotFuzzEnv::CheckMountFormatData(const std::str .fs_type = "ext4", .mount_point = mount_point, }; - CHECK(0 == fs_mgr_do_format(entry, false /* crypt_footer */)); + CHECK(0 == fs_mgr_do_format(entry)); CHECK(0 == fs_mgr_do_mount_one(entry)); return std::make_unique(mount_point); } From e5b5e376f3d22ed5c4465f6964a3059e45c9c1e0 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 8 Nov 2021 16:38:54 -0800 Subject: [PATCH 5/8] init: stop handling FDE-specific fs_mgr return codes These codes can't be returned anymore, so stop handling them. Bug: 191796797 Change-Id: I9bffd43db7c2f43e5f749e04e84154165dec279e --- init/builtins.cpp | 27 +-------------------------- 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/init/builtins.cpp b/init/builtins.cpp index 763a147f3..364744e0f 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -586,32 +586,7 @@ static void import_late(const std::vector& rc_paths) { * return code is processed based on input code */ static Result queue_fs_event(int code, bool userdata_remount) { - if (code == FS_MGR_MNTALL_DEV_NEEDS_ENCRYPTION) { - if (userdata_remount) { - // FS_MGR_MNTALL_DEV_NEEDS_ENCRYPTION should only happen on FDE devices. Since we don't - // support userdata remount on FDE devices, this should never been triggered. Time to - // panic! - LOG(ERROR) << "Userdata remount is not supported on FDE devices. How did you get here?"; - trigger_shutdown("reboot,requested-userdata-remount-on-fde-device"); - } - ActionManager::GetInstance().QueueEventTrigger("encrypt"); - return {}; - } else if (code == FS_MGR_MNTALL_DEV_MIGHT_BE_ENCRYPTED) { - if (userdata_remount) { - // FS_MGR_MNTALL_DEV_MIGHT_BE_ENCRYPTED should only happen on FDE devices. Since we - // don't support userdata remount on FDE devices, this should never been triggered. - // Time to panic! - LOG(ERROR) << "Userdata remount is not supported on FDE devices. How did you get here?"; - trigger_shutdown("reboot,requested-userdata-remount-on-fde-device"); - } - SetProperty("ro.crypto.state", "encrypted"); - ActionManager::GetInstance().QueueEventTrigger("defaultcrypto"); - return {}; - } else if (code == FS_MGR_MNTALL_DEV_NOT_ENCRYPTED) { - SetProperty("ro.crypto.state", "unencrypted"); - ActionManager::GetInstance().QueueEventTrigger("nonencrypted"); - return {}; - } else if (code == FS_MGR_MNTALL_DEV_NOT_ENCRYPTABLE) { + if (code == FS_MGR_MNTALL_DEV_NOT_ENCRYPTABLE) { SetProperty("ro.crypto.state", "unsupported"); ActionManager::GetInstance().QueueEventTrigger("nonencrypted"); return {}; From 4aa4231a8e416d020e7e645f66f9649a9c8f7f4c Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 8 Nov 2021 16:38:54 -0800 Subject: [PATCH 6/8] init: remove FDE workaround from load_persist_props FDE is no longer supported, so this workaround is no longer needed. Bug: 191796797 Change-Id: I059b07035b2158fe84e19544f03aab48de787e62 --- init/builtins.cpp | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/init/builtins.cpp b/init/builtins.cpp index 364744e0f..e9c717a37 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -1094,17 +1094,6 @@ static Result do_loglevel(const BuiltinArguments& args) { } static Result do_load_persist_props(const BuiltinArguments& args) { - // Devices with FDE have load_persist_props called twice; the first time when the temporary - // /data partition is mounted and then again once /data is truly mounted. We do not want to - // read persistent properties from the temporary /data partition or mark persistent properties - // as having been loaded during the first call, so we return in that case. - std::string crypto_state = android::base::GetProperty("ro.crypto.state", ""); - std::string crypto_type = android::base::GetProperty("ro.crypto.type", ""); - if (crypto_state == "encrypted" && crypto_type == "block") { - static size_t num_calls = 0; - if (++num_calls == 1) return {}; - } - SendLoadPersistentPropertiesMessage(); start_waiting_for_property("ro.persistent_properties.ready", "true"); From 89ba7775af00cf9032dc0c8542dfcb7c33194eb2 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 8 Nov 2021 16:38:55 -0800 Subject: [PATCH 7/8] fs_mgr: remove FDE-specific FS_MGR_MNTALL codes Remove these codes, now that neither fs_mgr nor init uses them anymore. Bug: 191796797 Change-Id: I97451ed8b83043a4035fc8cf8bfbb95ee60afd83 --- fs_mgr/include/fs_mgr.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/fs_mgr/include/fs_mgr.h b/fs_mgr/include/fs_mgr.h index 5ae4422bd..29a5e60e6 100644 --- a/fs_mgr/include/fs_mgr.h +++ b/fs_mgr/include/fs_mgr.h @@ -56,9 +56,6 @@ enum mount_mode { #define FS_MGR_MNTALL_DEV_NEEDS_METADATA_ENCRYPTION 6 #define FS_MGR_MNTALL_DEV_FILE_ENCRYPTED 5 #define FS_MGR_MNTALL_DEV_NEEDS_RECOVERY 4 -#define FS_MGR_MNTALL_DEV_NEEDS_ENCRYPTION 3 -#define FS_MGR_MNTALL_DEV_MIGHT_BE_ENCRYPTED 2 -#define FS_MGR_MNTALL_DEV_NOT_ENCRYPTED 1 #define FS_MGR_MNTALL_DEV_NOT_ENCRYPTABLE 0 #define FS_MGR_MNTALL_FAIL (-1) From 335cd1f4a3a34531a37e193a1aae928f5aa817e8 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 8 Nov 2021 16:38:55 -0800 Subject: [PATCH 8/8] init.rc: remove handling of vold.decrypt property changes These triggers were specific to FDE, which is no longer supported, so remove them. Bug: 191796797 Change-Id: Iab4f6bd3d0fa70ff959be2c27986c101c42e29d7 --- rootdir/init.rc | 31 ------------------------------- 1 file changed, 31 deletions(-) diff --git a/rootdir/init.rc b/rootdir/init.rc index 27fa05951..939b92d2a 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -1119,37 +1119,6 @@ on property:sys.init_log_level=* on charger class_start charger -on property:vold.decrypt=trigger_load_persist_props - load_persist_props - start logd - start logd-reinit - -on property:vold.decrypt=trigger_post_fs_data - trigger post-fs-data - trigger zygote-start - -on property:vold.decrypt=trigger_restart_min_framework - # A/B update verifier that marks a successful boot. - exec_start update_verifier - class_start main - -on property:vold.decrypt=trigger_restart_framework - # A/B update verifier that marks a successful boot. - exec_start update_verifier - class_start_post_data hal - class_start_post_data core - class_start main - class_start late_start - setprop service.bootanim.exit 0 - setprop service.bootanim.progress 0 - start bootanim - -on property:vold.decrypt=trigger_shutdown_framework - class_reset late_start - class_reset main - class_reset_post_data core - class_reset_post_data hal - on property:sys.boot_completed=1 bootchart stop # Setup per_boot directory so other .rc could start to use it on boot_completed