From 80ebe8c35d7426f38fa2e951679ce2a765a6bcf2 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Wed, 21 Jun 2023 12:00:09 -0700 Subject: [PATCH] snapuserd: Restrict where reads/writes to dm_user_header happen. Only write to dm_user_header in the functions which explicitly need to marshal it. This avoids leakage of dm-user specifics into core logic. This also simplifies the existing control flow by allowing us to set an error anywhere, or nowhere, as any "return false" from ProcessIORequest will automatically set an error header. Bug: 288273605 Test: snapuserd_test Change-Id: I85f67208197d7ecc49e348ab3013827a38e84761 --- .../user-space-merge/snapuserd_core.h | 1 - .../user-space-merge/snapuserd_dm_user.cpp | 84 ++++++++----------- 2 files changed, 34 insertions(+), 51 deletions(-) diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h index 7cffc1c4b..c984a61f8 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h @@ -119,7 +119,6 @@ class Worker { } // Functions interacting with dm-user - bool ReadDmUserHeader(); bool WriteDmUserPayload(size_t size); bool DmuserReadRequest(); diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_dm_user.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_dm_user.cpp index 0eb2a14fe..1b17698a3 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_dm_user.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_dm_user.cpp @@ -290,21 +290,6 @@ bool Worker::RunThread() { return true; } -// Read Header from dm-user misc device. This gives -// us the sector number for which IO is issued by dm-snapshot device -bool Worker::ReadDmUserHeader() { - if (!android::base::ReadFully(ctrl_fd_, bufsink_.GetBufPtr(), sizeof(struct dm_user_header))) { - if (errno != ENOTBLK) { - SNAP_PLOG(ERROR) << "Control-read failed"; - } - - SNAP_PLOG(DEBUG) << "ReadDmUserHeader failed...."; - return false; - } - - return true; -} - // Send the payload/data back to dm-user misc device. bool Worker::WriteDmUserPayload(size_t size) { size_t payload_size = size; @@ -345,19 +330,15 @@ bool Worker::ReadDataFromBaseDevice(sector_t sector, size_t read_size) { } bool Worker::ReadAlignedSector(sector_t sector, size_t sz) { - struct dm_user_header* header = bufsink_.GetHeaderPtr(); size_t remaining_size = sz; std::vector>& chunk_vec = snapuserd_->GetChunkVec(); - bool io_error = false; int ret = 0; do { // Process 1MB payload at a time size_t read_size = std::min(PAYLOAD_BUFFER_SZ, remaining_size); - header->type = DM_USER_RESP_SUCCESS; size_t total_bytes_read = 0; - io_error = false; bufsink_.ResetBufferOffset(); while (read_size) { @@ -375,7 +356,7 @@ bool Worker::ReadAlignedSector(sector_t sector, size_t sz) { // device. if (!ReadDataFromBaseDevice(sector, size)) { SNAP_LOG(ERROR) << "ReadDataFromBaseDevice failed"; - header->type = DM_USER_RESP_ERROR; + return false; } ret = size; @@ -384,35 +365,26 @@ bool Worker::ReadAlignedSector(sector_t sector, size_t sz) { // process it. if (!ProcessCowOp(it->second)) { SNAP_LOG(ERROR) << "ProcessCowOp failed"; - header->type = DM_USER_RESP_ERROR; + return false; } ret = BLOCK_SZ; } - // Just return the header if it is an error - if (header->type == DM_USER_RESP_ERROR) { - RespondIOError(); - io_error = true; - break; - } - read_size -= ret; total_bytes_read += ret; sector += (ret >> SECTOR_SHIFT); bufsink_.UpdateBufferOffset(ret); } - if (!io_error) { - if (!WriteDmUserPayload(total_bytes_read)) { - return false; - } - - SNAP_LOG(DEBUG) << "WriteDmUserPayload success total_bytes_read: " << total_bytes_read - << " remaining_size: " << remaining_size; - remaining_size -= total_bytes_read; + if (!WriteDmUserPayload(total_bytes_read)) { + return false; } - } while (remaining_size > 0 && !io_error); + + SNAP_LOG(DEBUG) << "WriteDmUserPayload success total_bytes_read: " << total_bytes_read + << " remaining_size: " << remaining_size; + remaining_size -= total_bytes_read; + } while (remaining_size > 0); return true; } @@ -453,8 +425,6 @@ int Worker::ReadUnalignedSector( } bool Worker::ReadUnalignedSector(sector_t sector, size_t size) { - struct dm_user_header* header = bufsink_.GetHeaderPtr(); - header->type = DM_USER_RESP_SUCCESS; bufsink_.ResetBufferOffset(); std::vector>& chunk_vec = snapuserd_->GetChunkVec(); @@ -622,9 +592,15 @@ bool Worker::DmuserReadRequest() { } bool Worker::ProcessIORequest() { + // Read Header from dm-user misc device. This gives + // us the sector number for which IO is issued by dm-snapshot device struct dm_user_header* header = bufsink_.GetHeaderPtr(); + if (!android::base::ReadFully(ctrl_fd_, header, sizeof(*header))) { + if (errno != ENOTBLK) { + SNAP_PLOG(ERROR) << "Control-read failed"; + } - if (!ReadDmUserHeader()) { + SNAP_PLOG(DEBUG) << "ReadDmUserHeader failed...."; return false; } @@ -634,26 +610,34 @@ bool Worker::ProcessIORequest() { SNAP_LOG(DEBUG) << "Daemon: msg->type: " << std::dec << header->type; SNAP_LOG(DEBUG) << "Daemon: msg->flags: " << std::dec << header->flags; + // Use the same header buffer as the response header. + int request_type = header->type; + header->type = DM_USER_RESP_SUCCESS; header_response_ = true; - switch (header->type) { - case DM_USER_REQ_MAP_READ: { - if (!DmuserReadRequest()) { - return false; - } + bool ok; + switch (request_type) { + case DM_USER_REQ_MAP_READ: + ok = DmuserReadRequest(); break; - } - case DM_USER_REQ_MAP_WRITE: { + case DM_USER_REQ_MAP_WRITE: // TODO: We should not get any write request // to dm-user as we mount all partitions // as read-only. Need to verify how are TRIM commands // handled during mount. - return false; - } + ok = false; + break; + + default: + ok = false; + break; } - return true; + if (!ok && header->type != DM_USER_RESP_ERROR) { + RespondIOError(); + } + return ok; } } // namespace snapshot