diff options
author | Mattias Nissler <mnissler@google.com> | 2016-06-15 09:26:26 +0000 |
---|---|---|
committer | android-build-merger <android-build-merger@google.com> | 2016-06-15 09:26:26 +0000 |
commit | c537226ff1424d83c1c71766ce6ab143110aedc8 (patch) | |
tree | 1749e79d87a9961ba42579760647918625e1f7eb | |
parent | 0ba4d5aea04c8a064ef0ddf541fdff8a3ad74a96 (diff) | |
parent | 84cff3f1a4ab9c0533f127525097302f716eca59 (diff) | |
download | nvram-c537226ff1424d83c1c71766ce6ab143110aedc8.tar.gz |
Add commands for clearing NVRAM on full reset. am: 0b833364a3
am: 84cff3f1a4
Change-Id: I6cefdaf4d3a659d75daca5456ea2e3b6329d174d
-rw-r--r-- | core/Android.mk | 2 | ||||
-rw-r--r-- | core/include/nvram/core/nvram_manager.h | 28 | ||||
-rw-r--r-- | core/nvram_manager.cpp | 79 | ||||
-rw-r--r-- | core/rules.mk | 4 | ||||
-rw-r--r-- | core/tests/nvram_manager_test.cpp | 121 | ||||
-rw-r--r-- | messages/include/nvram/messages/nvram_messages.h | 25 | ||||
-rw-r--r-- | messages/nvram_messages.cpp | 27 |
7 files changed, 280 insertions, 6 deletions
diff --git a/core/Android.mk b/core/Android.mk index 4da770c..6a705ea 100644 --- a/core/Android.mk +++ b/core/Android.mk @@ -35,7 +35,7 @@ LOCAL_SRC_FILES := \ crypto_boringssl.cpp \ nvram_manager.cpp \ persistence.cpp -LOCAL_CFLAGS := -Wall -Werror -Wextra +LOCAL_CFLAGS := -Wall -Werror -Wextra -DNVRAM_WIPE_STORAGE_SUPPORT LOCAL_CLANG := true LOCAL_C_INCLUDES := $(LOCAL_PATH)/include LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH)/include diff --git a/core/include/nvram/core/nvram_manager.h b/core/include/nvram/core/nvram_manager.h index 97ee424..ac03e09 100644 --- a/core/include/nvram/core/nvram_manager.h +++ b/core/include/nvram/core/nvram_manager.h @@ -56,6 +56,33 @@ class NvramManager { nvram_result_t LockSpaceRead(const LockSpaceReadRequest& request, LockSpaceReadResponse* response); + // The wipe functions are meant for use by firmware after determining the + // device's mode of operation. These can be used to clear access-controlled + // NVRAM when a user invokes a full hardware reset. Note that in regular + // operation, the user *MUST BE PREVENTED* from wiping access-controlled + // NVRAM. + // + // If a full hardware reset can conveniently clear the access-controlled NVRAM + // storage area out of band, it's fine to do so. In this case, the + // wiping-related commands should not be exposed at all. Note that this is the + // default behavior - the reference implementation will ignore all wipe + // requests unless compiled with NVRAM_WIPE_STORAGE_SUPPORT=1. + // + // For devices where firmware doesn't have direct control over the storage + // area used by access-controlled NVRAM, the wiping commands are provided to + // facilitate clearing storage: + // 1. Determine boot mode. + // 2. If not in recovery mode, call DisableWipe(). All further wipe requests + // will be rejected. A reboot (or TEE restart for that matter) is + // required before a new decision can be made. + // 3. If operating in recovery mode, forgo calling DisableWipe(). The + // recovery process will then be able to invoke WipeStorage() later as + // needed. + nvram_result_t WipeStorage(const WipeStorageRequest& request, + WipeStorageResponse* response); + nvram_result_t DisableWipe(const DisableWipeRequest& request, + DisableWipeResponse* response); + private: // Holds transient state corresponding to an allocated NVRAM space, i.e. meta // data valid for a single boot. One instance of this struct is kept in memory @@ -116,6 +143,7 @@ class NvramManager { bool initialized_ = false; bool disable_create_ = false; + bool disable_wipe_ = false; // Bookkeeping information for allocated spaces. size_t num_spaces_ = 0; diff --git a/core/nvram_manager.cpp b/core/nvram_manager.cpp index a1eb9c1..9ea3d65 100644 --- a/core/nvram_manager.cpp +++ b/core/nvram_manager.cpp @@ -135,6 +135,14 @@ void NvramManager::Dispatch(const nvram::Request& request, result = LockSpaceRead(*input.get<COMMAND_LOCK_SPACE_READ>(), &output->Activate<COMMAND_LOCK_SPACE_READ>()); break; + case nvram::COMMAND_WIPE_STORAGE: + result = WipeStorage(*input.get<COMMAND_WIPE_STORAGE>(), + &output->Activate<COMMAND_WIPE_STORAGE>()); + break; + case nvram::COMMAND_DISABLE_WIPE: + result = DisableWipe(*input.get<COMMAND_DISABLE_WIPE>(), + &output->Activate<COMMAND_DISABLE_WIPE>()); + break; } response->result = result; @@ -161,6 +169,7 @@ nvram_result_t NvramManager::GetInfo(const GetInfoRequest& /* request */, for (size_t i = 0; i < num_spaces_; ++i) { space_list[i] = spaces_[i].index; } + response->wipe_disabled = disable_wipe_; return NV_RESULT_SUCCESS; } @@ -501,6 +510,76 @@ nvram_result_t NvramManager::LockSpaceRead( return NV_RESULT_INVALID_PARAMETER; } +nvram_result_t NvramManager::WipeStorage( + const WipeStorageRequest& /* request */, + WipeStorageResponse* /* response */) { + if (!Initialize()) + return NV_RESULT_INTERNAL_ERROR; + +#ifdef NVRAM_WIPE_STORAGE_SUPPORT + if (disable_wipe_) { + return NV_RESULT_OPERATION_DISABLED; + } + + // Go through all spaces and wipe the corresponding data. Note that the header + // is only updated once all space data is gone. This will "break" all spaces + // that are left declared but don't have data. This situation can be observed + // if we crash somewhere during the wiping process before clearing the header. + // + // Note that we deliberately choose this wiping sequence so we can never end + // up in a state where the header appears clean but existing space data + // remains. + // + // As a final note, the ideal solution would be to atomically clear the header + // and delete all space data. While more desirable from an operational point + // of view, this would drastically complicate storage layer requirements to + // support cross-object atomicity instead of per-object atomicity. + for (size_t i = 0; i < num_spaces_; ++i) { + const uint32_t index = spaces_[i].index; + switch (persistence::DeleteSpace(index)) { + case storage::Status::kStorageError: + NVRAM_LOG_ERR("Failed to wipe space 0x%" PRIx32 " data.", index); + return NV_RESULT_INTERNAL_ERROR; + case storage::Status::kNotFound: + // The space was missing even if it shouldn't have been. This may occur + // if a previous wiping attempt was aborted half-way. Log an error, but + // return success as we're in the desired state. + NVRAM_LOG_WARN("Space 0x%" PRIx32 " data missing on wipe.", index); + break; + case storage::Status::kSuccess: + break; + } + } + + // All spaces are gone, clear the header. + num_spaces_ = 0; + return WriteHeader(Optional<uint32_t>()); +#else // NVRAM_WIPE_STORAGE_SUPPORT + // We're not accessing the flag member, so prevent a compiler warning. The + // alternative of conditionally including the member in the class declaration + // looks cleaner at first sight, but comes with the risk of + // NVRAM_WIPE_STORAGE_SUPPORT polarity mismatches between compilation units, + // which is more subtly dangerous, so we rather keep the member even for the + // case in which it is not used. + (void)disable_wipe_; + return NV_RESULT_OPERATION_DISABLED; +#endif // NVRAM_WIPE_STORAGE_SUPPORT +} + +nvram_result_t NvramManager::DisableWipe( + const DisableWipeRequest& /* request */, + DisableWipeResponse* /* response */) { + if (!Initialize()) + return NV_RESULT_INTERNAL_ERROR; + +#ifdef NVRAM_WIPE_STORAGE_SUPPORT + disable_wipe_ = true; + return NV_RESULT_SUCCESS; +#else // NVRAM_WIPE_STORAGE_SUPPORT + return NV_RESULT_OPERATION_DISABLED; +#endif // NVRAM_WIPE_STORAGE_SUPPORT +} + nvram_result_t NvramManager::SpaceRecord::CheckWriteAccess( const Blob& authorization_value) { if (persistent.HasControl(NV_CONTROL_PERSISTENT_WRITE_LOCK)) { diff --git a/core/rules.mk b/core/rules.mk index ed2f0a4..2afd570 100644 --- a/core/rules.mk +++ b/core/rules.mk @@ -38,6 +38,10 @@ ifneq ($(NVRAM_LOG_LEVEL),) GLOBAL_DEFINES += NVRAM_LOG_LEVEL=$(NVRAM_LOG_LEVEL) endif +ifneq ($(NVRAM_WIPE_STORAGE_SUPPORT),) +GLOBAL_DEFINES += NVRAM_WIPE_STORAGE_SUPPORT=$(NVRAM_WIPE_STORAGE_SUPPORT) +endif + GLOBAL_INCLUDES += $(LOCAL_DIR)/include/ include make/module.mk diff --git a/core/tests/nvram_manager_test.cpp b/core/tests/nvram_manager_test.cpp index b42f26a..8b00440 100644 --- a/core/tests/nvram_manager_test.cpp +++ b/core/tests/nvram_manager_test.cpp @@ -1106,5 +1106,126 @@ TEST_F(NvramManagerTest, LockSpaceRead_NotLockable) { nvram.LockSpaceRead(lock_space_read_request, &lock_space_read_response)); } +TEST_F(NvramManagerTest, WipeStorage_Success) { + // Set up an NVRAM space. + NvramSpace space; + ASSERT_TRUE(space.contents.Resize(10)); + ASSERT_EQ(storage::Status::kSuccess, persistence::StoreSpace(17, space)); + SetupHeader(NvramHeader::kVersion, 17); + + // Check that the space is visible. + NvramManager nvram; + GetSpaceInfoRequest get_space_info_request; + get_space_info_request.index = 17; + GetSpaceInfoResponse get_space_info_response; + EXPECT_EQ(NV_RESULT_SUCCESS, nvram.GetSpaceInfo(get_space_info_request, + &get_space_info_response)); + EXPECT_EQ(10U, get_space_info_response.size); + + // Request a wipe. + WipeStorageRequest wipe_storage_request; + WipeStorageResponse wipe_storage_response; + EXPECT_EQ(NV_RESULT_SUCCESS, + nvram.WipeStorage(wipe_storage_request, &wipe_storage_response)); + + // The space should no longer be declared. + GetInfoRequest get_info_request; + GetInfoResponse get_info_response; + EXPECT_EQ(NV_RESULT_SUCCESS, + nvram.GetInfo(get_info_request, &get_info_response)); + EXPECT_EQ(0U, get_info_response.space_list.size()); + + // Accessing the space should fail. + EXPECT_EQ( + NV_RESULT_SPACE_DOES_NOT_EXIST, + nvram.GetSpaceInfo(get_space_info_request, &get_space_info_response)); +} + +TEST_F(NvramManagerTest, WipeStorage_Abort) { + // Set up two pre-existing spaces and a matching header. + NvramSpace space; + ASSERT_TRUE(space.contents.Resize(10)); + ASSERT_EQ(storage::Status::kSuccess, persistence::StoreSpace(1, space)); + ASSERT_TRUE(space.contents.Resize(20)); + ASSERT_EQ(storage::Status::kSuccess, persistence::StoreSpace(2, space)); + NvramHeader header; + header.version = NvramHeader::kVersion; + ASSERT_TRUE(header.allocated_indices.Resize(2)); + header.allocated_indices[0] = 1; + header.allocated_indices[1] = 2; + ASSERT_EQ(storage::Status::kSuccess, persistence::StoreHeader(header)); + + // Check that the spaces are visible. + NvramManager nvram; + GetInfoRequest get_info_request; + GetInfoResponse get_info_response; + EXPECT_EQ(NV_RESULT_SUCCESS, + nvram.GetInfo(get_info_request, &get_info_response)); + EXPECT_EQ(2U, get_info_response.space_list.size()); + int space_mask = 0; + for (size_t i = 0; i < get_info_response.space_list.size(); ++i) { + space_mask |= (1 << get_info_response.space_list[i]); + } + EXPECT_EQ(0x6, space_mask); + + // Set things up so the deletion request for the second space fails. + storage::SetSpaceWriteError(2, true); + + // The wipe request should fail now. + WipeStorageRequest wipe_storage_request; + WipeStorageResponse wipe_storage_response; + EXPECT_EQ(NV_RESULT_INTERNAL_ERROR, + nvram.WipeStorage(wipe_storage_request, &wipe_storage_response)); + + // New wipe attempt with a fresh instance after clearing the error. + storage::SetSpaceWriteError(2, false); + NvramManager nvram2; + EXPECT_EQ(NV_RESULT_SUCCESS, + nvram2.WipeStorage(wipe_storage_request, &wipe_storage_response)); + + // No spaces should remain. + EXPECT_EQ(NV_RESULT_SUCCESS, + nvram2.GetInfo(get_info_request, &get_info_response)); + EXPECT_EQ(0U, get_info_response.space_list.size()); +} + +TEST_F(NvramManagerTest, WipeStorage_Disable) { + // Set up an NVRAM space. + NvramSpace space; + ASSERT_TRUE(space.contents.Resize(10)); + ASSERT_EQ(storage::Status::kSuccess, persistence::StoreSpace(17, space)); + SetupHeader(NvramHeader::kVersion, 17); + + NvramManager nvram; + + // Disable wiping. + DisableWipeRequest disable_wipe_request; + DisableWipeResponse disable_wipe_response; + EXPECT_EQ(NV_RESULT_SUCCESS, + nvram.DisableWipe(disable_wipe_request, &disable_wipe_response)); + + // A wipe request should fail. + WipeStorageRequest wipe_storage_request; + WipeStorageResponse wipe_storage_response; + EXPECT_EQ(NV_RESULT_OPERATION_DISABLED, + nvram.WipeStorage(wipe_storage_request, &wipe_storage_response)); + + // The space should remain declared. + GetInfoRequest get_info_request; + GetInfoResponse get_info_response; + EXPECT_EQ(NV_RESULT_SUCCESS, + nvram.GetInfo(get_info_request, &get_info_response)); + ASSERT_EQ(1U, get_info_response.space_list.size()); + EXPECT_EQ(17U, get_info_response.space_list[0]); + + // The space data should remain present. + GetSpaceInfoRequest get_space_info_request; + get_space_info_request.index = 17; + GetSpaceInfoResponse get_space_info_response; + EXPECT_EQ(NV_RESULT_SUCCESS, nvram.GetSpaceInfo(get_space_info_request, + &get_space_info_response)); + EXPECT_EQ(10U, get_space_info_response.size); +} + } // namespace } // namespace nvram diff --git a/messages/include/nvram/messages/nvram_messages.h b/messages/include/nvram/messages/nvram_messages.h index e574707..c2691f7 100644 --- a/messages/include/nvram/messages/nvram_messages.h +++ b/messages/include/nvram/messages/nvram_messages.h @@ -28,6 +28,8 @@ namespace nvram { enum Command { + // Commands corresponding to the API defined in the access-controlled NVRAM + // HAL spec. Note that some commands service multiple HAL API calls. COMMAND_GET_INFO = 1, COMMAND_CREATE_SPACE = 2, COMMAND_GET_SPACE_INFO = 3, @@ -37,6 +39,12 @@ enum Command { COMMAND_READ_SPACE = 7, COMMAND_LOCK_SPACE_WRITE = 8, COMMAND_LOCK_SPACE_READ = 9, + + // The wipe commands are provided as a utility for clearing NVRAM during + // hardware reset. These are not accessible via the HAL API, but may be used + // by implementations to implement NVRAM clearing on full device reset. + COMMAND_WIPE_STORAGE = 10, + COMMAND_DISABLE_WIPE = 11, }; // COMMAND_GET_INFO request/response. @@ -48,6 +56,7 @@ struct GetInfoResponse { uint64_t max_space_size = 0; uint32_t max_spaces = 0; Vector<uint32_t> space_list; + bool wipe_disabled = false; }; // COMMAND_CREATE_SPACE request/response. @@ -120,6 +129,14 @@ struct LockSpaceReadRequest { struct LockSpaceReadResponse {}; +// COMMAND_WIPE request/response. +struct WipeStorageRequest {}; +struct WipeStorageResponse {}; + +// COMMAND_DISABLE_WIPE request/response. +struct DisableWipeRequest {}; +struct DisableWipeResponse {}; + // Generic request message, carrying command-specific payload. The slot set in // the payload determines the requested command. using RequestUnion = TaggedUnion< @@ -132,7 +149,9 @@ using RequestUnion = TaggedUnion< TaggedUnionMember<COMMAND_WRITE_SPACE, WriteSpaceRequest>, TaggedUnionMember<COMMAND_READ_SPACE, ReadSpaceRequest>, TaggedUnionMember<COMMAND_LOCK_SPACE_WRITE, LockSpaceWriteRequest>, - TaggedUnionMember<COMMAND_LOCK_SPACE_READ, LockSpaceReadRequest>>; + TaggedUnionMember<COMMAND_LOCK_SPACE_READ, LockSpaceReadRequest>, + TaggedUnionMember<COMMAND_WIPE_STORAGE, WipeStorageRequest>, + TaggedUnionMember<COMMAND_DISABLE_WIPE, DisableWipeRequest>>; struct Request { RequestUnion payload; }; @@ -149,7 +168,9 @@ using ResponseUnion = TaggedUnion< TaggedUnionMember<COMMAND_WRITE_SPACE, WriteSpaceResponse>, TaggedUnionMember<COMMAND_READ_SPACE, ReadSpaceResponse>, TaggedUnionMember<COMMAND_LOCK_SPACE_WRITE, LockSpaceWriteResponse>, - TaggedUnionMember<COMMAND_LOCK_SPACE_READ, LockSpaceReadResponse>>; + TaggedUnionMember<COMMAND_LOCK_SPACE_READ, LockSpaceReadResponse>, + TaggedUnionMember<COMMAND_WIPE_STORAGE, WipeStorageResponse>, + TaggedUnionMember<COMMAND_DISABLE_WIPE, DisableWipeResponse>>; struct Response { nvram_result_t result = NV_RESULT_SUCCESS; ResponseUnion payload; diff --git a/messages/nvram_messages.cpp b/messages/nvram_messages.cpp index 7b1cafb..37fe7af 100644 --- a/messages/nvram_messages.cpp +++ b/messages/nvram_messages.cpp @@ -37,7 +37,8 @@ template<> struct DescriptorForType<GetInfoResponse> { MakeField(2, &GetInfoResponse::available_size), MakeField(3, &GetInfoResponse::max_spaces), MakeField(4, &GetInfoResponse::space_list), - MakeField(5, &GetInfoResponse::max_space_size)); + MakeField(5, &GetInfoResponse::max_space_size), + MakeField(6, &GetInfoResponse::wipe_disabled)); }; template<> struct DescriptorForType<CreateSpaceRequest> { @@ -125,6 +126,22 @@ template<> struct DescriptorForType<LockSpaceReadResponse> { static constexpr auto kFields = MakeFieldList(); }; +template<> struct DescriptorForType<WipeStorageRequest> { + static constexpr auto kFields = MakeFieldList(); +}; + +template<> struct DescriptorForType<WipeStorageResponse> { + static constexpr auto kFields = MakeFieldList(); +}; + +template<> struct DescriptorForType<DisableWipeRequest> { + static constexpr auto kFields = MakeFieldList(); +}; + +template<> struct DescriptorForType<DisableWipeResponse> { + static constexpr auto kFields = MakeFieldList(); +}; + template<> struct DescriptorForType<Request> { static constexpr auto kFields = MakeFieldList( MakeOneOfField(1, &Request::payload, COMMAND_GET_INFO), @@ -135,7 +152,9 @@ template<> struct DescriptorForType<Request> { MakeOneOfField(6, &Request::payload, COMMAND_WRITE_SPACE), MakeOneOfField(7, &Request::payload, COMMAND_READ_SPACE), MakeOneOfField(8, &Request::payload, COMMAND_LOCK_SPACE_WRITE), - MakeOneOfField(9, &Request::payload, COMMAND_LOCK_SPACE_READ)); + MakeOneOfField(9, &Request::payload, COMMAND_LOCK_SPACE_READ), + MakeOneOfField(10, &Request::payload, COMMAND_WIPE_STORAGE), + MakeOneOfField(11, &Request::payload, COMMAND_DISABLE_WIPE)); }; template<> struct DescriptorForType<Response> { @@ -149,7 +168,9 @@ template<> struct DescriptorForType<Response> { MakeOneOfField(7, &Response::payload, COMMAND_WRITE_SPACE), MakeOneOfField(8, &Response::payload, COMMAND_READ_SPACE), MakeOneOfField(9, &Response::payload, COMMAND_LOCK_SPACE_WRITE), - MakeOneOfField(10, &Response::payload, COMMAND_LOCK_SPACE_READ)); + MakeOneOfField(10, &Response::payload, COMMAND_LOCK_SPACE_READ), + MakeOneOfField(11, &Response::payload, COMMAND_WIPE_STORAGE), + MakeOneOfField(12, &Response::payload, COMMAND_DISABLE_WIPE)); }; template <typename Message> |