[kernel] Require valid rsrc to debuglog_create if readable
Bug: 32044
Fixed: 94740
Change-Id: Iedf2368c07e9d27c9f8930220a575edf34e21532
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/653202
Reviewed-by: Marco Vanotti <mvanotti@google.com>
Fuchsia-Auto-Submit: Drew Fisher <zarvox@google.com>
Reviewed-by: Abdulla Kamar <abdulla@google.com>
Commit-Queue: Auto-Submit <auto-submit@fuchsia-infra.iam.gserviceaccount.com>
diff --git a/src/lib/zircon/rust/src/debuglog.rs b/src/lib/zircon/rust/src/debuglog.rs
index 3a33ca3..9e92221 100644
--- a/src/lib/zircon/rust/src/debuglog.rs
+++ b/src/lib/zircon/rust/src/debuglog.rs
@@ -89,12 +89,23 @@
mod tests {
use super::*;
use crate::{cprng_draw, Signals, Time};
+ use fidl_fuchsia_boot as fboot;
+ use fuchsia_component::client::connect_channel_to_protocol;
// expect_message_in_debuglog will read the last 10000 messages in zircon's debuglog, looking
// for a message that equals `sent_msg`. If found, the function returns. If the first 10,000
// messages doesn't contain `sent_msg`, it will panic.
fn expect_message_in_debuglog(sent_msg: String) {
- let resource = Resource::from(Handle::invalid());
+ use fuchsia_zircon::{Channel, HandleBased};
+ let (client_end, server_end) = Channel::create().unwrap();
+ connect_channel_to_protocol::<fboot::RootResourceMarker>(server_end).unwrap();
+ let service = fboot::RootResourceSynchronousProxy::new(client_end);
+ let resource =
+ service.get(fuchsia_zircon::Time::INFINITE).expect("couldn't get root resource");
+ // This test and fuchsia-zircon are different crates, so we need
+ // to use from_raw to convert between the fuchsia_zircon handle and this test handle.
+ // See https://fxbug.dev/91562 for details.
+ let resource = unsafe { Resource::from(Handle::from_raw(resource.into_raw())) };
let debuglog = DebugLog::create(&resource, DebugLogOpts::READABLE).unwrap();
for _ in 0..10000 {
match debuglog.read() {
diff --git a/src/sys/component_manager/lib/logger/BUILD.gn b/src/sys/component_manager/lib/logger/BUILD.gn
index d4e3e99..d1c176a2 100644
--- a/src/sys/component_manager/lib/logger/BUILD.gn
+++ b/src/sys/component_manager/lib/logger/BUILD.gn
@@ -28,7 +28,11 @@
"//third_party/rust_crates:log",
]
- test_deps = [ "//third_party/rust_crates:rand" ]
+ test_deps = [
+ "//sdk/fidl/fuchsia.boot:fuchsia.boot-rustc",
+ "//src/lib/fuchsia-component",
+ "//third_party/rust_crates:rand",
+ ]
sources = [
"src/fmt.rs",
@@ -39,5 +43,6 @@
}
fuchsia_unittest_package("component_manager_logger_test") {
+ manifest = "meta/component_manager_logger_test.cml"
deps = [ ":logger_test" ]
}
diff --git a/src/sys/component_manager/lib/logger/meta/component_manager_logger_test.cml b/src/sys/component_manager/lib/logger/meta/component_manager_logger_test.cml
new file mode 100644
index 0000000..5eea9d0
--- /dev/null
+++ b/src/sys/component_manager/lib/logger/meta/component_manager_logger_test.cml
@@ -0,0 +1,13 @@
+{
+ include: [
+ "//src/sys/test_manager/system-test.shard.cml",
+ "//src/sys/test_runners/rust/default.shard.cml",
+ "syslog/client.shard.cml",
+ ],
+ program: {
+ binary: "bin/cm_logger_lib_test",
+ },
+ use: [
+ { protocol: "fuchsia.boot.RootResource" },
+ ],
+}
diff --git a/src/sys/component_manager/lib/logger/src/klog.rs b/src/sys/component_manager/lib/logger/src/klog.rs
index 1640cb9..750f2bf 100644
--- a/src/sys/component_manager/lib/logger/src/klog.rs
+++ b/src/sys/component_manager/lib/logger/src/klog.rs
@@ -109,17 +109,27 @@
use {
super::*,
anyhow::Context,
+ fidl_fuchsia_boot as fboot,
+ fuchsia_component::client::connect_channel_to_protocol,
fuchsia_zircon::{AsHandleRef, HandleBased},
log::*,
rand::Rng,
std::panic,
};
+ fn get_root_resource() -> zx::Resource {
+ let (client_end, server_end) = zx::Channel::create().unwrap();
+ connect_channel_to_protocol::<fboot::RootResourceMarker>(server_end).unwrap();
+ let service = fboot::RootResourceSynchronousProxy::new(client_end);
+ let resource = service.get(zx::Time::INFINITE).expect("couldn't get root resource");
+ resource
+ }
+
// expect_message_in_debuglog will read the last 10000 messages in zircon's debuglog, looking
// for a message that equals `sent_msg`. If found, the function returns. If the first 10,000
// messages doesn't contain `sent_msg`, it will panic.
fn expect_message_in_debuglog(sent_msg: String) {
- let resource = zx::Resource::from(zx::Handle::invalid());
+ let resource = get_root_resource();
let debuglog = zx::DebugLog::create(&resource, zx::DebugLogOpts::READABLE).unwrap();
for _ in 0..10000 {
match debuglog.read() {
diff --git a/src/sys/component_manager/src/builtin/log.rs b/src/sys/component_manager/src/builtin/log.rs
index 73acd3c..0644fcf 100644
--- a/src/sys/component_manager/src/builtin/log.rs
+++ b/src/sys/component_manager/src/builtin/log.rs
@@ -106,6 +106,7 @@
cm_task_scope::TaskScope,
fidl::endpoints::ClientEnd,
fuchsia_async as fasync,
+ fuchsia_component::client::connect_to_protocol,
fuchsia_zircon::sys,
fuchsia_zircon::AsHandleRef,
futures::lock::Mutex,
@@ -114,11 +115,28 @@
std::sync::Weak,
};
+ fn root_resource_available() -> bool {
+ let bin = std::env::args().next();
+ match bin.as_ref().map(String::as_ref) {
+ Some("/pkg/bin/component_manager_test") => false,
+ Some("/pkg/bin/component_manager_boot_env_test") => true,
+ _ => panic!("Unexpected test binary name {:?}", bin),
+ }
+ }
+
+ async fn get_root_resource() -> Result<zx::Resource, Error> {
+ let root_resource_provider = connect_to_protocol::<fboot::RootResourceMarker>()?;
+ let root_resource_handle = root_resource_provider.get().await?;
+ Ok(zx::Resource::from(root_resource_handle))
+ }
+
#[fuchsia::test]
async fn has_correct_rights_for_read_only() -> Result<(), Error> {
- // The kernel does not currently require a valid `Resource` to be
- // provided when creating a `Debuglog`. This will change in the future.
- let read_only_log = ReadOnlyLog::new(Resource::from(zx::Handle::invalid()));
+ if !root_resource_available() {
+ return Ok(());
+ }
+ let resource = get_root_resource().await?;
+ let read_only_log = ReadOnlyLog::new(resource);
let (proxy, stream) =
fidl::endpoints::create_proxy_and_stream::<fboot::ReadOnlyLogMarker>()?;
fasync::Task::local(
@@ -136,9 +154,11 @@
#[fuchsia::test]
async fn can_connect_to_read_only() -> Result<(), Error> {
- // The kernel does not currently require a valid `Resource` to be
- // provided when creating a `Debuglog`. This will change in the future.
- let read_only_log = ReadOnlyLog::new(Resource::from(zx::Handle::invalid()));
+ if !root_resource_available() {
+ return Ok(());
+ }
+ let resource = get_root_resource().await?;
+ let read_only_log = ReadOnlyLog::new(resource);
let hooks = Hooks::new(None);
hooks.install(read_only_log.hooks()).await;
@@ -176,8 +196,8 @@
#[fuchsia::test]
async fn has_correct_rights_for_write_only() -> Result<(), Error> {
- // The kernel does not currently require a valid `Resource` to be
- // provided when creating a `Debuglog`. This will change in the future.
+ // The kernel requires a valid `Resource` to be provided when creating a `Debuglog` that
+ // can be read from, but not one that can be written to. This may change in the future.
let resource = Resource::from(zx::Handle::invalid());
let write_only_log =
WriteOnlyLog::new(zx::DebugLog::create(&resource, zx::DebugLogOpts::empty()).unwrap());
@@ -199,8 +219,8 @@
#[fuchsia::test]
async fn can_connect_to_write_only() -> Result<(), Error> {
- // The kernel does not currently require a valid `Resource` to be
- // provided when creating a `Debuglog`. This will change in the future.
+ // The kernel requires a valid `Resource` to be provided when creating a `Debuglog` that
+ // can be read from, but not one that can be written to. This may change in the future.
let resource = Resource::from(zx::Handle::invalid());
let write_only_log =
WriteOnlyLog::new(zx::DebugLog::create(&resource, zx::DebugLogOpts::empty()).unwrap());
diff --git a/zircon/kernel/lib/syscalls/zircon.cc b/zircon/kernel/lib/syscalls/zircon.cc
index 6142451..1d9b6d3 100644
--- a/zircon/kernel/lib/syscalls/zircon.cc
+++ b/zircon/kernel/lib/syscalls/zircon.cc
@@ -125,8 +125,13 @@
zx_status_t sys_debuglog_create(zx_handle_t rsrc, uint32_t options, user_out_handle* out) {
LTRACEF("options 0x%x\n", options);
- // TODO(fxbug.dev/32044) Require a non-INVALID handle.
- if (rsrc != ZX_HANDLE_INVALID) {
+ // To support allowing the libc dynamic linker to emit log messages even
+ // before process bootstrap is complete, we allow creating a debuglog with
+ // options == 0 (write-only) without yet having a valid `rsrc` handle.
+ // Otherwise, we should require a valid `rsrc` handle.
+ // Inversely: if a resource handle is given, or if `options` is nonzero,
+ // require that `rsrc` be a valid root resource handle.
+ if (rsrc != ZX_HANDLE_INVALID || options != 0) {
// TODO(fxbug.dev/30918): finer grained validation
zx_status_t status = validate_resource(rsrc, ZX_RSRC_KIND_ROOT);
if (status != ZX_OK)
diff --git a/zircon/system/utest/core/debuglog/debuglog.cc b/zircon/system/utest/core/debuglog/debuglog.cc
index 2517213..1fc073f 100644
--- a/zircon/system/utest/core/debuglog/debuglog.cc
+++ b/zircon/system/utest/core/debuglog/debuglog.cc
@@ -49,6 +49,28 @@
EXPECT_EQ(log_handle, 0);
}
+TEST(DebugLogTest, InvalidHandleAllowedOnlyForWrite) {
+ zx_handle_t log_handle = 0;
+
+ // Requesting a read-write handle with a valid root resource should work
+ ASSERT_OK(zx_debuglog_create(get_root_resource(), ZX_LOG_FLAG_READABLE, &log_handle));
+ ASSERT_OK(zx_handle_close(log_handle));
+ log_handle = ZX_HANDLE_INVALID;
+
+ // Requesting a write-only handle with an invalid root resource should work,
+ // since the dynamic linker needs to be able to log something somewhere when
+ // it can't bootstrap a process
+ ASSERT_OK(zx_debuglog_create(ZX_HANDLE_INVALID, 0, &log_handle));
+ ASSERT_OK(zx_handle_close(log_handle));
+ log_handle = ZX_HANDLE_INVALID;
+
+ // But requesting a read-write handle with an invalid root resource represents
+ // an information leak, so the kernel checks the first arg, and if it's not a
+ // valid handle, then we get an error
+ EXPECT_EQ(zx_debuglog_create(ZX_HANDLE_INVALID, ZX_LOG_FLAG_READABLE, &log_handle),
+ ZX_ERR_BAD_HANDLE);
+}
+
TEST(DebugLogTest, MaxMessageSize) {
zx_handle_t log_handle = ZX_HANDLE_INVALID;
ASSERT_OK(zx_debuglog_create(get_root_resource(), ZX_LOG_FLAG_READABLE, &log_handle));
