Support umount in helper_process
Currently mount points cleanup is called directly in imageloader_main
without sandboxing. In order for cleanup to work in non-root user in
sandboxed environment (dbus call), I provide a method in helper_process
to perform umount in sandbox as root.
It also fixes a bug in message deserialization where message could be
cut shorter accidentally due to encoding with string delimeter in the
middle.
BUG=chromium:784031,chromium:782334
TEST=unittest, mount/umount images on DuT
Change-Id: Ib4c7522c96165c6ffae4cb0342137f8a0bdcc07b
Reviewed-on: https://chromium-review.googlesource.com/767024
Commit-Ready: Xiaochu Liu <xiaochu@chromium.org>
Tested-by: Xiaochu Liu <xiaochu@chromium.org>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
diff --git a/helper_process.cc b/helper_process.cc
index 583d606..de107fd 100644
--- a/helper_process.cc
+++ b/helper_process.cc
@@ -46,31 +46,11 @@
pid_ = p.Pid();
}
-bool HelperProcess::SendMountCommand(int fd, const std::string& path,
- FileSystem fs_type,
- const std::string& table) {
- struct msghdr msg = {0};
- char fds[CMSG_SPACE(sizeof(fd))];
- memset(fds, '\0', sizeof(fds));
-
- MountImage msg_proto;
- msg_proto.set_mount_path(path);
- msg_proto.set_table(table);
-
- // Convert the internal enum to the protobuf enum.
- switch (fs_type) {
- case FileSystem::kExt4:
- msg_proto.set_fs_type(MountImage::EXT4);
- break;
- case FileSystem::kSquashFS:
- msg_proto.set_fs_type(MountImage::SQUASH);
- break;
- default:
- LOG(FATAL) << "Unknown file system type passed to helper process.";
- }
-
+std::unique_ptr<CommandResponse> HelperProcess::SendCommand(
+ const ImageCommand& image_command, struct msghdr* msg) {
+ // Serialize message object into string.
std::string msg_str;
- if (!msg_proto.SerializeToString(&msg_str))
+ if (!image_command.SerializeToString(&msg_str))
LOG(FATAL) << "error serializing protobuf";
// iov takes a non-const pointer.
@@ -81,9 +61,39 @@
iov[0].iov_base = buffer;
iov[0].iov_len = sizeof(buffer);
- msg.msg_iov = iov;
- msg.msg_iovlen = sizeof(iov) / sizeof(iov[0]);
+ msg->msg_iov = iov;
+ msg->msg_iovlen = sizeof(iov) / sizeof(iov[0]);
+ if (sendmsg(control_fd_.get(), msg, 0) < 0) PLOG(FATAL) << "sendmsg failed";
+
+ return WaitForResponse();
+}
+
+bool HelperProcess::SendMountCommand(int fd, const std::string& path,
+ FileSystem fs_type,
+ const std::string& table) {
+ struct msghdr msg = {0};
+ char fds[CMSG_SPACE(sizeof(fd))];
+ memset(fds, '\0', sizeof(fds));
+
+ // 1. Construct message object.
+ ImageCommand image_command;
+ image_command.mutable_mount_command()->set_mount_path(path);
+ image_command.mutable_mount_command()->set_table(table);
+
+ // Convert the internal enum to the protobuf enum.
+ switch (fs_type) {
+ case FileSystem::kExt4:
+ image_command.mutable_mount_command()->set_fs_type(MountCommand::EXT4);
+ break;
+ case FileSystem::kSquashFS:
+ image_command.mutable_mount_command()->set_fs_type(MountCommand::SQUASH);
+ break;
+ default:
+ LOG(FATAL) << "Unknown file system type passed to helper process.";
+ }
+
+ // 2. Encode the fd into message.
msg.msg_control = fds;
msg.msg_controllen = sizeof(fds);
@@ -96,12 +106,43 @@
memmove(CMSG_DATA(cmsg), &fd, sizeof(fd));
msg.msg_controllen = cmsg->cmsg_len;
- if (sendmsg(control_fd_.get(), &msg, 0) < 0) PLOG(FATAL) << "sendmsg failed";
-
- return WaitForResponse();
+ // 3. Send the command.
+ return SendCommand(image_command, &msg)->success();
}
-bool HelperProcess::WaitForResponse() {
+bool HelperProcess::SendUnmountAllCommand(bool dry_run,
+ const std::string& rootpath,
+ std::vector<std::string>* paths) {
+ struct msghdr msg = {0};
+
+ // 1. Construct message object.
+ ImageCommand image_command;
+ image_command.mutable_unmount_all_command()->set_dry_run(dry_run);
+ image_command.mutable_unmount_all_command()->set_unmount_rootpath(rootpath);
+
+ // 2. Send the command.
+ std::unique_ptr<CommandResponse> response = SendCommand(image_command, &msg);
+
+ // 3. Process return value.
+ for (int i = 0; i < response->paths_size(); i++) {
+ std::string path(response->paths(i));
+ paths->push_back(path);
+ }
+ return response->success();
+}
+
+bool HelperProcess::SendUnmountCommand(const std::string& path) {
+ struct msghdr msg = {0};
+
+ // 1. Construct message object.
+ ImageCommand image_command;
+ image_command.mutable_unmount_command()->set_unmount_path(path);
+
+ // 2. Send the command.
+ return SendCommand(image_command, &msg)->success();
+}
+
+std::unique_ptr<CommandResponse> HelperProcess::WaitForResponse() {
struct pollfd pfd;
pfd.fd = control_fd_.get();
pfd.events = POLLIN;
@@ -109,20 +150,20 @@
int rc = poll(&pfd, 1, /*timeout=*/ 2000);
PCHECK(rc >= 0 || errno == EINTR);
+ std::unique_ptr<CommandResponse> response =
+ std::make_unique<CommandResponse>();
if (pfd.revents & POLLIN) {
char buffer[4096];
memset(buffer, '\0', sizeof(buffer));
ssize_t bytes = read(control_fd_.get(), buffer, sizeof(buffer));
PCHECK(bytes != -1);
- MountResponse response;
- if (!response.ParseFromArray(buffer, bytes)) {
+ if (!response->ParseFromArray(buffer, bytes)) {
LOG(FATAL) << "could not deserialize protobuf: " << buffer;
}
- return response.success();
}
- return false;
+ return response;
}
} // namespace imageloader
diff --git a/helper_process.h b/helper_process.h
index a6a7da4..e78a01d 100644
--- a/helper_process.h
+++ b/helper_process.h
@@ -10,11 +10,18 @@
#include <base/files/scoped_file.h>
#include <base/macros.h>
+// Forward declare classes in sys/socket.h
+struct msghdr;
+
namespace imageloader {
// Forward declare the FileSystem enum.
enum class FileSystem;
+// Forward declare classes in ipc.pb.h
+class ImageCommand;
+class CommandResponse;
+
// Tracks a helper subprocess. Handles forking, cleaning up on termination, and
// IPC.
class HelperProcess {
@@ -33,17 +40,33 @@
FileSystem fs_type,
const std::string& table);
+ // Sends a message telling the helper process to enumerate all mount point
+ // paths with prefix of |rootpath| and returns them with |paths|. If
+ // |dry_run| is true, no mount points are unmounted. If |dry_run| is false,
+ // all mount points returned in |paths| are unmounted.
+ virtual bool SendUnmountAllCommand(bool dry_run,
+ const std::string& rootpath,
+ std::vector<std::string>* paths);
+
+ // Sends a message telling the helper process to umount mount point at
+ // |path|.
+ virtual bool SendUnmountCommand(const std::string& path);
+
const pid_t pid() { return pid_; }
protected:
// Waits for a reply from the helper process indicating if the mount succeeded
// or failed.
- virtual bool WaitForResponse();
+ virtual std::unique_ptr<CommandResponse> WaitForResponse();
pid_t pid_{0};
base::ScopedFD control_fd_;
private:
+ // Constructs msghdr and sends it.
+ virtual std::unique_ptr<CommandResponse> SendCommand(
+ const ImageCommand& msg_proto, struct msghdr* msg);
+
DISALLOW_COPY_AND_ASSIGN(HelperProcess);
};
diff --git a/imageloader_impl.cc b/imageloader_impl.cc
index 28081c2..ff85c10 100644
--- a/imageloader_impl.cc
+++ b/imageloader_impl.cc
@@ -105,6 +105,18 @@
return RemoveComponentAtPath(name, component_root, component_path);
}
+bool ImageLoaderImpl::CleanupAll(bool dry_run,
+ const base::FilePath& parent_dir,
+ std::vector<std::string>* paths,
+ HelperProcess* process) {
+ return process->SendUnmountAllCommand(dry_run, parent_dir.value(), paths);
+}
+
+bool ImageLoaderImpl::Cleanup(const base::FilePath& path,
+ HelperProcess* process) {
+ return process->SendUnmountCommand(path.value());
+}
+
bool ImageLoaderImpl::RemoveComponentAtPath(
const std::string& name,
const base::FilePath& component_root,
diff --git a/imageloader_impl.h b/imageloader_impl.h
index 7c84431..7692b87 100644
--- a/imageloader_impl.h
+++ b/imageloader_impl.h
@@ -40,8 +40,20 @@
bool RegisterComponent(const std::string& name, const std::string& version,
const std::string& component_folder_abs_path);
+ // Remove a component.
bool RemoveComponent(const std::string& name);
+ // Enumerates all mount point paths with prefix of |parent_dir| and returns
+ // them with |paths|. If |dry_run| is true, no mount points are unmounted.
+ // If |dry_run| is false, all mount points returned in |paths| are unmounted.
+ bool CleanupAll(bool dry_run,
+ const base::FilePath& parent_dir,
+ std::vector<std::string>* paths,
+ HelperProcess* process);
+
+ // Cleanup a mount point at |path|.
+ bool Cleanup(const base::FilePath& path, HelperProcess* process);
+
// Get component version given component name.
std::string GetComponentVersion(const std::string& name);
diff --git a/imageloader_main.cc b/imageloader_main.cc
index 79bcacc..d72c99c 100644
--- a/imageloader_main.cc
+++ b/imageloader_main.cc
@@ -90,33 +90,6 @@
return 1;
}
- if (FLAGS_unmount_all) {
- imageloader::VerityMounter mounter;
- std::vector<base::FilePath> paths;
- const base::FilePath parent_dir(FLAGS_loaded_mounts_base);
- bool success = mounter.CleanupAll(FLAGS_dry_run, parent_dir, &paths);
- if (FLAGS_dry_run) {
- for (const auto& path : paths) {
- std::cout << path.value() << "\n";
- }
- }
- if (!success) {
- LOG(ERROR) << "--unmount_all failed!";
- return 1;
- }
- return 0;
- }
-
- if (FLAGS_unmount) {
- if (FLAGS_mount_point.empty()) {
- LOG(ERROR) << "--mount_point=path must be set with --unmount";
- return 1;
- }
-
- imageloader::VerityMounter mounter;
- return mounter.Cleanup(base::FilePath(FLAGS_mount_point)) ? 0 : 1;
- }
-
// Executing this as the helper process if specified.
if (FLAGS_mount_helper_fd >= 0) {
CHECK_GT(FLAGS_mount_helper_fd, -1);
@@ -171,6 +144,48 @@
return 0;
}
+ // Unmount all component mount points and exit.
+ if (FLAGS_unmount_all) {
+ // Run with minimal privilege.
+ imageloader::ImageLoader::EnterSandbox();
+
+ imageloader::ImageLoaderImpl loader(std::move(config));
+ std::vector<std::string> paths;
+ const base::FilePath parent_dir(FLAGS_loaded_mounts_base);
+ bool success = loader.CleanupAll(FLAGS_dry_run,
+ parent_dir, &paths, helper_process.get());
+ if (FLAGS_dry_run) {
+ for (const auto& path : paths) {
+ std::cout << path << "\n";
+ }
+ }
+ if (!success) {
+ LOG(ERROR) << "--unmount_all failed!";
+ return 1;
+ }
+ return 0;
+ }
+
+ // Unmount a component mount point and exit.
+ if (FLAGS_unmount) {
+ // Run with minimal privilege.
+ imageloader::ImageLoader::EnterSandbox();
+
+ if (FLAGS_mount_point.empty()) {
+ LOG(ERROR) << "--mount_point=path must be set with --unmount";
+ return 1;
+ }
+
+ imageloader::ImageLoaderImpl loader(std::move(config));
+ const base::FilePath path(FLAGS_mount_point);
+ bool success = loader.Cleanup(path, helper_process.get());
+ if (!success) {
+ LOG(ERROR) << "--unmount failed!";
+ return 1;
+ }
+ return 0;
+ }
+
// Run as a daemon and wait for dbus requests.
imageloader::ImageLoader daemon(std::move(config), std::move(helper_process));
daemon.Run();
diff --git a/imageloader_unittest.cc b/imageloader_unittest.cc
index 03ef911..b7876ab 100644
--- a/imageloader_unittest.cc
+++ b/imageloader_unittest.cc
@@ -172,6 +172,50 @@
EXPECT_EQ(expected_path, mnt_path);
}
+TEST_F(ImageLoaderTest, CleanupAll) {
+ Keys keys;
+ keys.push_back(
+ std::vector<uint8_t>(std::begin(kDevPublicKey), std::end(kDevPublicKey)));
+
+ auto helper_mock = std::make_unique<MockHelperProcess>();
+ EXPECT_CALL(*helper_mock, SendUnmountAllCommand(_, _, _))
+ .Times(1);
+ ON_CALL(*helper_mock, SendUnmountAllCommand(_, _, _))
+ .WillByDefault(testing::Return(true));
+
+ base::ScopedTempDir scoped_mount_dir;
+ ASSERT_TRUE(scoped_mount_dir.CreateUniqueTempDir());
+
+ ImageLoaderConfig config(keys, temp_dir_.value().c_str(),
+ scoped_mount_dir.path().value().c_str());
+ ImageLoaderImpl loader(std::move(config));
+
+ base::FilePath rootpath("/");
+ std::vector<std::string> paths;
+ EXPECT_EQ(loader.CleanupAll(true, rootpath, &paths, helper_mock.get()), true);
+}
+
+TEST_F(ImageLoaderTest, Cleanup) {
+ Keys keys;
+ keys.push_back(
+ std::vector<uint8_t>(std::begin(kDevPublicKey), std::end(kDevPublicKey)));
+
+ auto helper_mock = std::make_unique<MockHelperProcess>();
+ EXPECT_CALL(*helper_mock, SendUnmountCommand(_)).Times(1);
+ ON_CALL(*helper_mock, SendUnmountCommand(_))
+ .WillByDefault(testing::Return(true));
+
+ base::ScopedTempDir scoped_mount_dir;
+ ASSERT_TRUE(scoped_mount_dir.CreateUniqueTempDir());
+
+ ImageLoaderConfig config(keys, temp_dir_.value().c_str(),
+ scoped_mount_dir.path().value().c_str());
+ ImageLoaderImpl loader(std::move(config));
+
+ base::FilePath path("/");
+ EXPECT_EQ(loader.Cleanup(path, helper_mock.get()), true);
+}
+
TEST_F(ImageLoaderTest, LoadExt4Image) {
Keys keys;
keys.push_back(
diff --git a/ipc.proto b/ipc.proto
index e2aeb83..efd4d09 100644
--- a/ipc.proto
+++ b/ipc.proto
@@ -7,21 +7,36 @@
package imageloader;
-// Optional fields are considered best practice, but since the client and server
-// come from the same code base, this uses required fields to save on
-// validation.
+message ImageCommand {
+ oneof op {
+ MountCommand mount_command = 1;
+ UnmountAllCommand unmount_all_command = 2;
+ UnmountCommand unmount_command = 3;
+ }
+}
-message MountImage {
- required string mount_path = 1;
- required string table = 2;
+message MountCommand {
enum FileSystem {
- UNKNOWN = 0;
SQUASH = 1;
EXT4 = 2;
}
- required FileSystem fs_type = 3 [default = UNKNOWN];
+ required string mount_path = 2;
+ required string table = 3;
+ required FileSystem fs_type = 4 [default = SQUASH];
}
-message MountResponse {
- required bool success = 1;
+message UnmountAllCommand {
+ required bool dry_run = 5 [default = true];
+ required string unmount_rootpath = 6;
+}
+
+message UnmountCommand {
+ required string unmount_path = 7;
+}
+
+message CommandResponse {
+ required bool success = 1 [default = false];
+ // |paths| are only set for response of UnmountAllCommand: paths that can be
+ // unmounted.
+ repeated string paths = 2;
}
diff --git a/mock_helper_process.h b/mock_helper_process.h
index 66bf699..61c54c2 100644
--- a/mock_helper_process.h
+++ b/mock_helper_process.h
@@ -22,6 +22,12 @@
MOCK_METHOD4(SendMountCommand, bool(int, const std::string&,
FileSystem, const std::string&));
+ MOCK_METHOD3(SendUnmountAllCommand, bool(bool,
+ const std::string&,
+ std::vector<std::string>* paths));
+
+ MOCK_METHOD1(SendUnmountCommand, bool(const std::string&));
+
private:
DISALLOW_COPY_AND_ASSIGN(MockHelperProcess);
};
diff --git a/mount_helper.cc b/mount_helper.cc
index a5d6ea2..447b3a6 100644
--- a/mount_helper.cc
+++ b/mount_helper.cc
@@ -80,49 +80,73 @@
struct cmsghdr* cmsg = CMSG_FIRSTHDR(&msg);
- if (cmsg == nullptr) LOG(FATAL) << "no cmsg";
-
- if (cmsg->cmsg_level != SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS)
- LOG(FATAL) << "cmsg is wrong type";
-
- memmove(&pending_fd_, CMSG_DATA(cmsg), sizeof(pending_fd_));
-
- MountImage command;
- if (!command.ParseFromArray(buffer, strlen(buffer)))
+ ImageCommand command;
+ std::string msg_str;
+ msg_str.assign(buffer, bytes - 1);
+ if (!command.ParseFromString(msg_str))
LOG(FATAL) << "error parsing protobuf";
// Handle the command to mount the image.
- MountResponse response = HandleCommand(command);
+ CommandResponse response = HandleCommand(command, cmsg);
// Reply to the parent process with the success or failure.
SendResponse(response);
}
-MountResponse MountHelper::HandleCommand(MountImage& command) {
- // Convert the fs type to a string.
- std::string fs_type;
- switch (command.fs_type()) {
- case MountImage::EXT4:
- fs_type = "ext4";
- break;
- case MountImage::SQUASH:
- fs_type = "squashfs";
- break;
- default:
- LOG(FATAL) << "unknown filesystem type";
+CommandResponse MountHelper::HandleCommand(ImageCommand& imageCommand,
+ struct cmsghdr* cmsg) {
+ CommandResponse response;
+ if (imageCommand.has_mount_command()) {
+ MountCommand command = imageCommand.mount_command();
+ if (cmsg == nullptr)
+ LOG(FATAL) << "no cmsg";
+
+ if (cmsg->cmsg_level != SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS)
+ LOG(FATAL) << "cmsg is wrong type";
+
+ memmove(&pending_fd_, CMSG_DATA(cmsg), sizeof(pending_fd_));
+
+ // Convert the fs type to a string.
+ std::string fs_type;
+ switch (command.fs_type()) {
+ case MountCommand::EXT4:
+ fs_type = "ext4";
+ break;
+ case MountCommand::SQUASH:
+ fs_type = "squashfs";
+ break;
+ default:
+ LOG(FATAL) << "unknown filesystem type";
+ }
+
+ bool status = mounter_.Mount(base::ScopedFD(pending_fd_),
+ base::FilePath(command.mount_path()),
+ fs_type,
+ command.table());
+ if (!status)
+ LOG(ERROR) << "mount failed";
+
+ response.set_success(status);
+ } else if (imageCommand.has_unmount_all_command()) {
+ UnmountAllCommand command = imageCommand.unmount_all_command();
+ std::vector<base::FilePath> paths;
+ const base::FilePath root_dir(command.unmount_rootpath());
+ response.set_success(mounter_.CleanupAll(command.dry_run(),
+ root_dir, &paths));
+ for (const auto& path : paths) {
+ const std::string path_(path.value());
+ response.add_paths(path_);
+ }
+ } else if (imageCommand.has_unmount_command()) {
+ UnmountCommand command = imageCommand.unmount_command();
+ const base::FilePath path(command.unmount_path());
+ response.set_success(mounter_.Cleanup(path));
+ } else {
+ LOG(ERROR) << "unknown operations";
}
-
- bool status = mounter_.Mount(base::ScopedFD(pending_fd_),
- base::FilePath(command.mount_path()),
- fs_type,
- command.table());
- if (!status) LOG(ERROR) << "mount failed";
-
- MountResponse response;
- response.set_success(status);
return response;
}
-void MountHelper::SendResponse(MountResponse& response) {
+void MountHelper::SendResponse(CommandResponse& response) {
std::string response_str;
if (!response.SerializeToString(&response_str))
LOG(FATAL) << "failed to serialize protobuf";
diff --git a/mount_helper.h b/mount_helper.h
index 499a5b3..7fc0b06 100644
--- a/mount_helper.h
+++ b/mount_helper.h
@@ -14,6 +14,8 @@
using MessageLoopForIO = base::MessageLoopForIO;
+struct cmsghdr;
+
namespace imageloader {
// Main loop for the Mount helper process.
@@ -32,8 +34,9 @@
void OnFileCanWriteWithoutBlocking(int fd) override {}
private:
- MountResponse HandleCommand(MountImage& command);
- void SendResponse(MountResponse& response);
+ CommandResponse HandleCommand(ImageCommand& command,
+ struct cmsghdr* cmsg);
+ void SendResponse(CommandResponse& response);
base::ScopedFD control_fd_;
MessageLoopForIO::FileDescriptorWatcher control_watcher_;
diff --git a/seccomp/imageloader-helper-seccomp-amd64.policy b/seccomp/imageloader-helper-seccomp-amd64.policy
index c13d485..d51eced 100644
--- a/seccomp/imageloader-helper-seccomp-amd64.policy
+++ b/seccomp/imageloader-helper-seccomp-amd64.policy
@@ -70,6 +70,7 @@
statfs: 1
sysinfo: 1
tgkill: 1
+umount2: 1
uname: 1
unlink: 1
wait4: 1
diff --git a/seccomp/imageloader-helper-seccomp-arm.policy b/seccomp/imageloader-helper-seccomp-arm.policy
index 53f7da2..d4f2942 100644
--- a/seccomp/imageloader-helper-seccomp-arm.policy
+++ b/seccomp/imageloader-helper-seccomp-arm.policy
@@ -71,6 +71,7 @@
sysinfo: 1
tgkill: 1
ugetrlimit: 1
+umount2: 1
uname: 1
wait4: 1
write: 1
diff --git a/seccomp/imageloader-helper-seccomp-x86.policy b/seccomp/imageloader-helper-seccomp-x86.policy
index cfcba52..62cb278 100644
--- a/seccomp/imageloader-helper-seccomp-x86.policy
+++ b/seccomp/imageloader-helper-seccomp-x86.policy
@@ -72,6 +72,7 @@
tgkill: 1
time: 1
ugetrlimit: 1
+umount2: 1
uname: 1
unlink: 1
wait4: 1
