From d6790c4bc64bfd7145b8a14b45e9e3f1a3db6c62 Mon Sep 17 00:00:00 2001 From: Jooyung Han Date: Wed, 21 Aug 2024 14:15:52 +0900 Subject: [PATCH 1/3] init_parser_fuzzer: remove interface checks CheckInterfaceInheritanceHierarchy() is for host_init_verifier to check the interface names at buildtime. We don't need to fuzz the host-side verification code. Bug: 326827772 Test: run init_parser_fuzzer Change-Id: Ie01dc2953fd6e69ef3c2cb9caadf7b9964a3d244 --- init/fuzzer/Android.bp | 2 - init/fuzzer/init_parser_fuzzer.cpp | 61 ------------------------------ 2 files changed, 63 deletions(-) diff --git a/init/fuzzer/Android.bp b/init/fuzzer/Android.bp index 5823932d2..8cfd597d9 100644 --- a/init/fuzzer/Android.bp +++ b/init/fuzzer/Android.bp @@ -30,7 +30,6 @@ cc_defaults { shared_libs: [ "libbase", "libfs_mgr", - "libhidl-gen-utils", "liblog", "libprocessgroup", "libselinux", @@ -49,7 +48,6 @@ cc_fuzz { srcs: [ "init_parser_fuzzer.cpp", ], - shared_libs: ["libhidlmetadata",], defaults: [ "libinit_fuzzer_defaults", ], diff --git a/init/fuzzer/init_parser_fuzzer.cpp b/init/fuzzer/init_parser_fuzzer.cpp index dc764650a..21b04f4b3 100644 --- a/init/fuzzer/init_parser_fuzzer.cpp +++ b/init/fuzzer/init_parser_fuzzer.cpp @@ -15,9 +15,7 @@ */ #include -#include #include -#include #include using namespace android; @@ -34,7 +32,6 @@ const std::string kValidPaths[] = { }; const int32_t kMaxBytes = 256; -const std::string kValidInterfaces = "android.frameworks.vr.composer@2.0::IVrComposerClient"; class InitParserFuzzer { public: @@ -44,9 +41,6 @@ class InitParserFuzzer { private: void InvokeParser(); void InvokeLimitParser(); - void InvokeInterfaceUtils(); - InterfaceInheritanceHierarchyMap GenerateHierarchyMap(); - std::vector GenerateInterfaceMetadata(); FuzzedDataProvider fdp_; }; @@ -64,60 +58,6 @@ void InitParserFuzzer::InvokeLimitParser() { } } -std::vector InitParserFuzzer::GenerateInterfaceMetadata() { - std::vector random_interface; - for (size_t idx = 0; idx < fdp_.ConsumeIntegral(); ++idx) { - HidlInterfaceMetadata metadata; - metadata.name = fdp_.ConsumeRandomLengthString(kMaxBytes); - for (size_t idx1 = 0; idx1 < fdp_.ConsumeIntegral(); ++idx1) { - metadata.inherited.push_back(fdp_.ConsumeRandomLengthString(kMaxBytes)); - } - random_interface.push_back(metadata); - } - return random_interface; -} - -InterfaceInheritanceHierarchyMap InitParserFuzzer::GenerateHierarchyMap() { - InterfaceInheritanceHierarchyMap result; - std::vector random_interface; - if (fdp_.ConsumeBool()) { - random_interface = GenerateInterfaceMetadata(); - } else { - random_interface = HidlInterfaceMetadata::all(); - } - - for (const HidlInterfaceMetadata& iface : random_interface) { - std::set inherited_interfaces; - for (const std::string& intf : iface.inherited) { - FQName fqname; - (void)fqname.setTo(intf); - inherited_interfaces.insert(fqname); - } - FQName fqname; - (void)fqname.setTo(iface.name); - result[fqname] = inherited_interfaces; - } - return result; -} - -void InitParserFuzzer::InvokeInterfaceUtils() { - InterfaceInheritanceHierarchyMap hierarchy_map = GenerateHierarchyMap(); - SetKnownInterfaces(hierarchy_map); - IsKnownInterface(fdp_.ConsumeRandomLengthString(kMaxBytes)); - std::set interface_set; - for (size_t idx = 0; idx < fdp_.ConsumeIntegral(); ++idx) { - auto set_interface_values = fdp_.PickValueInArray>({ - [&]() { - interface_set.insert(("aidl/" + fdp_.ConsumeRandomLengthString(kMaxBytes))); - }, - [&]() { interface_set.insert(fdp_.ConsumeRandomLengthString(kMaxBytes)); }, - [&]() { interface_set.insert(kValidInterfaces); }, - }); - set_interface_values(); - } - CheckInterfaceInheritanceHierarchy(interface_set, hierarchy_map); -} - void InitParserFuzzer::InvokeParser() { Parser parser; std::string name = fdp_.ConsumeBool() ? fdp_.ConsumeRandomLengthString(kMaxBytes) : "import"; @@ -132,7 +72,6 @@ void InitParserFuzzer::Process() { while (fdp_.remaining_bytes()) { auto invoke_parser_fuzzer = fdp_.PickValueInArray>({ [&]() { InvokeParser(); }, - [&]() { InvokeInterfaceUtils(); }, [&]() { InvokeLimitParser(); }, }); invoke_parser_fuzzer(); From 95c4242cf6eb67b0ae36aeb3e15325919765e3df Mon Sep 17 00:00:00 2001 From: Jooyung Han Date: Wed, 21 Aug 2024 13:57:44 +0900 Subject: [PATCH 2/3] host_init_verifier: check interface names directly Previously, ServiceParser did the check, but only when it's invoked by host_init_verifier. Host_init_verifier can do it directly, which removes unnecessary runtime dependencies from init. Bug: 326827772 Test: host_init_verifier detects wrong HIDL interface names. Change-Id: I4c8bb0e89a5def7341c48c52af730795a6ee13c0 --- init/host_init_verifier.cpp | 15 +++++++++++-- init/interface_utils.cpp | 42 ++++++++++++++++++------------------- init/interface_utils.h | 2 -- 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/init/host_init_verifier.cpp b/init/host_init_verifier.cpp index f746ab954..52be0a167 100644 --- a/init/host_init_verifier.cpp +++ b/init/host_init_verifier.cpp @@ -298,8 +298,7 @@ int main(int argc, char** argv) { ServiceList& sl = ServiceList::GetInstance(); Parser parser; parser.AddSectionParser("service", - std::make_unique(&sl, GetSubcontext(), - *interface_inheritance_hierarchy_map)); + std::make_unique(&sl, GetSubcontext(), std::nullopt)); parser.AddSectionParser("on", std::make_unique(&am, GetSubcontext())); parser.AddSectionParser("import", std::make_unique()); @@ -317,11 +316,23 @@ int main(int argc, char** argv) { return EXIT_FAILURE; } } + size_t failures = parser.parse_error_count() + am.CheckAllCommands() + sl.CheckAllCommands(); if (failures > 0) { LOG(ERROR) << "Failed to parse init scripts with " << failures << " error(s)."; return EXIT_FAILURE; } + + for (const auto& service : sl) { + if (const auto& result = CheckInterfaceInheritanceHierarchy( + service->interfaces(), *interface_inheritance_hierarchy_map); + !result.ok()) { + LOG(ERROR) << service->filename() << ": invalid interface in service '" + << service->name() << "': " << result.error(); + return EXIT_FAILURE; + } + } + return EXIT_SUCCESS; } diff --git a/init/interface_utils.cpp b/init/interface_utils.cpp index 1b76bba5f..84407aa70 100644 --- a/init/interface_utils.cpp +++ b/init/interface_utils.cpp @@ -39,27 +39,6 @@ std::string FQNamesToString(const std::set& fqnames) { return android::base::Join(fqname_strings, " "); } -} // namespace - -Result CheckInterfaceInheritanceHierarchy(const std::set& instances, - const InterfaceInheritanceHierarchyMap& hierarchy) { - std::set interface_fqnames; - for (const std::string& instance : instances) { - // There is insufficient build-time information on AIDL interfaces to check them here - // TODO(b/139307527): Rework how services store interfaces to avoid excess string parsing - if (base::Split(instance, "/")[0] == "aidl") { - continue; - } - - FqInstance fqinstance; - if (!fqinstance.setTo(instance)) { - return Error() << "Unable to parse interface instance '" << instance << "'"; - } - interface_fqnames.insert(fqinstance.getFqName()); - } - return CheckInterfaceInheritanceHierarchy(interface_fqnames, hierarchy); -} - Result CheckInterfaceInheritanceHierarchy(const std::set& interfaces, const InterfaceInheritanceHierarchyMap& hierarchy) { std::ostringstream error_stream; @@ -90,6 +69,27 @@ Result CheckInterfaceInheritanceHierarchy(const std::set& interfac return {}; } +} // namespace + +Result CheckInterfaceInheritanceHierarchy(const std::set& instances, + const InterfaceInheritanceHierarchyMap& hierarchy) { + std::set interface_fqnames; + for (const std::string& instance : instances) { + // There is insufficient build-time information on AIDL interfaces to check them here + // TODO(b/139307527): Rework how services store interfaces to avoid excess string parsing + if (base::Split(instance, "/")[0] == "aidl") { + continue; + } + + FqInstance fqinstance; + if (!fqinstance.setTo(instance)) { + return Error() << "Unable to parse interface instance '" << instance << "'"; + } + interface_fqnames.insert(fqinstance.getFqName()); + } + return CheckInterfaceInheritanceHierarchy(interface_fqnames, hierarchy); +} + std::optional> known_interfaces; void SetKnownInterfaces(const InterfaceInheritanceHierarchyMap& hierarchy) { diff --git a/init/interface_utils.h b/init/interface_utils.h index 4ca377f64..214fedae1 100644 --- a/init/interface_utils.h +++ b/init/interface_utils.h @@ -34,8 +34,6 @@ using InterfaceInheritanceHierarchyMap = std::map CheckInterfaceInheritanceHierarchy(const std::set& instances, const InterfaceInheritanceHierarchyMap& hierarchy); -Result CheckInterfaceInheritanceHierarchy(const std::set& interfaces, - const InterfaceInheritanceHierarchyMap& hierarchy); // Saves the set of known interfaces using the provided HIDL interface // inheritance hierarchy. From d51fb54d56ae7628bc29cb0a894ff300cb7230fa Mon Sep 17 00:00:00 2001 From: Jooyung Han Date: Wed, 21 Aug 2024 16:25:10 +0900 Subject: [PATCH 3/3] init: remove interface checks from init HIDL interface checks are done by host_init_verifier at build-time. Bug: 326827772 Test: mmma system/core/init Change-Id: I18e9590aba614bebfdbc6aa8bca7036821a6c4f3 --- init/Android.bp | 3 +-- init/host_init_verifier.cpp | 3 +-- init/init.cpp | 8 +++----- init/init_test.cpp | 9 +++------ init/reboot_test.cpp | 3 +-- init/service_parser.cpp | 27 --------------------------- init/service_parser.h | 11 ++--------- init/service_test.cpp | 3 +-- init/test_utils/service_utils.cpp | 3 +-- 9 files changed, 13 insertions(+), 57 deletions(-) diff --git a/init/Android.bp b/init/Android.bp index ed1f14820..8f1ca3de1 100644 --- a/init/Android.bp +++ b/init/Android.bp @@ -38,7 +38,6 @@ init_common_sources = [ "capabilities.cpp", "epoll.cpp", "import_parser.cpp", - "interface_utils.cpp", "interprocess_fifo.cpp", "keychords.cpp", "parser.cpp", @@ -88,6 +87,7 @@ init_device_sources = [ init_host_sources = [ "check_builtins.cpp", "host_import_parser.cpp", + "interface_utils.cpp", ] soong_config_module_type { @@ -190,7 +190,6 @@ libinit_cc_defaults { "libext4_utils", "libfs_mgr", "libgsi", - "libhidl-gen-utils", "liblog", "liblogwrap", "liblp", diff --git a/init/host_init_verifier.cpp b/init/host_init_verifier.cpp index 52be0a167..287857a5c 100644 --- a/init/host_init_verifier.cpp +++ b/init/host_init_verifier.cpp @@ -297,8 +297,7 @@ int main(int argc, char** argv) { ActionManager& am = ActionManager::GetInstance(); ServiceList& sl = ServiceList::GetInstance(); Parser parser; - parser.AddSectionParser("service", - std::make_unique(&sl, GetSubcontext(), std::nullopt)); + parser.AddSectionParser("service", std::make_unique(&sl, GetSubcontext())); parser.AddSectionParser("on", std::make_unique(&am, GetSubcontext())); parser.AddSectionParser("import", std::make_unique()); diff --git a/init/init.cpp b/init/init.cpp index 4878660e1..6c8089926 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -268,8 +268,8 @@ void DumpState() { Parser CreateParser(ActionManager& action_manager, ServiceList& service_list) { Parser parser; - parser.AddSectionParser("service", std::make_unique( - &service_list, GetSubcontext(), std::nullopt)); + parser.AddSectionParser("service", + std::make_unique(&service_list, GetSubcontext())); parser.AddSectionParser("on", std::make_unique(&action_manager, GetSubcontext())); parser.AddSectionParser("import", std::make_unique(&parser)); @@ -324,9 +324,7 @@ Parser CreateApexConfigParser(ActionManager& action_manager, ServiceList& servic } } #endif // RECOVERY - parser.AddSectionParser("service", - std::make_unique(&service_list, subcontext, - std::nullopt)); + parser.AddSectionParser("service", std::make_unique(&service_list, subcontext)); parser.AddSectionParser("on", std::make_unique(&action_manager, subcontext)); return parser; diff --git a/init/init_test.cpp b/init/init_test.cpp index 50882737c..f280de96d 100644 --- a/init/init_test.cpp +++ b/init/init_test.cpp @@ -62,8 +62,7 @@ void TestInit(const std::string& init_script_file, const BuiltinFunctionMap& tes Action::set_function_map(&test_function_map); Parser parser; - parser.AddSectionParser("service", - std::make_unique(service_list, nullptr, std::nullopt)); + parser.AddSectionParser("service", std::make_unique(service_list, nullptr)); parser.AddSectionParser("on", std::make_unique(action_manager, nullptr)); parser.AddSectionParser("import", std::make_unique(&parser)); @@ -625,8 +624,7 @@ service A something ServiceList service_list; Parser parser; - parser.AddSectionParser("service", - std::make_unique(&service_list, nullptr, std::nullopt)); + parser.AddSectionParser("service", std::make_unique(&service_list, nullptr)); ASSERT_TRUE(parser.ParseConfig(tf.path)); @@ -657,8 +655,7 @@ service A something ServiceList service_list; Parser parser; - parser.AddSectionParser("service", - std::make_unique(&service_list, nullptr, std::nullopt)); + parser.AddSectionParser("service", std::make_unique(&service_list, nullptr)); ASSERT_TRUE(parser.ParseConfig(tf.path)); ASSERT_EQ(1u, parser.parse_error_count()); diff --git a/init/reboot_test.cpp b/init/reboot_test.cpp index b3d038d14..b7a1cfdb5 100644 --- a/init/reboot_test.cpp +++ b/init/reboot_test.cpp @@ -103,8 +103,7 @@ service $name /system/bin/yes "$selabel", GetSecurityContext(), false); ServiceList& service_list = ServiceList::GetInstance(); Parser parser; - parser.AddSectionParser("service", - std::make_unique(&service_list, nullptr, std::nullopt)); + parser.AddSectionParser("service", std::make_unique(&service_list, nullptr)); TemporaryFile tf; ASSERT_TRUE(tf.fd != -1); diff --git a/init/service_parser.cpp b/init/service_parser.cpp index 6781c7083..e6f3af617 100644 --- a/init/service_parser.cpp +++ b/init/service_parser.cpp @@ -27,7 +27,6 @@ #include #include #include -#include #include #include @@ -201,24 +200,6 @@ Result ServiceParser::ParsePriority(std::vector&& args) { Result ServiceParser::ParseInterface(std::vector&& args) { const std::string& interface_name = args[1]; const std::string& instance_name = args[2]; - - // AIDL services don't use fully qualified names and instead just use "interface aidl " - if (interface_name != "aidl") { - FQName fq_name; - if (!FQName::parse(interface_name, &fq_name)) { - return Error() << "Invalid fully-qualified name for interface '" << interface_name - << "'"; - } - - if (!fq_name.isFullyQualified()) { - return Error() << "Interface name not fully-qualified '" << interface_name << "'"; - } - - if (fq_name.isValidValueName()) { - return Error() << "Interface name must not be a value name '" << interface_name << "'"; - } - } - const std::string fullname = interface_name + "/" + instance_name; for (const auto& svc : *service_list_) { @@ -702,14 +683,6 @@ Result ServiceParser::EndSection() { } } - if (interface_inheritance_hierarchy_) { - if (const auto& check_hierarchy_result = CheckInterfaceInheritanceHierarchy( - service_->interfaces(), *interface_inheritance_hierarchy_); - !check_hierarchy_result.ok()) { - return Error() << check_hierarchy_result.error(); - } - } - if (SelinuxGetVendorAndroidVersion() >= __ANDROID_API_R__) { if ((service_->flags() & SVC_CRITICAL) != 0 && (service_->flags() & SVC_ONESHOT) != 0) { return Error() << "service '" << service_->name() diff --git a/init/service_parser.h b/init/service_parser.h index 670a5c6cd..f06cfc47d 100644 --- a/init/service_parser.h +++ b/init/service_parser.h @@ -18,7 +18,6 @@ #include -#include "interface_utils.h" #include "parser.h" #include "service.h" #include "service_list.h" @@ -29,13 +28,8 @@ namespace init { class ServiceParser : public SectionParser { public: - ServiceParser( - ServiceList* service_list, Subcontext* subcontext, - const std::optional& interface_inheritance_hierarchy) - : service_list_(service_list), - subcontext_(subcontext), - interface_inheritance_hierarchy_(interface_inheritance_hierarchy), - service_(nullptr) {} + ServiceParser(ServiceList* service_list, Subcontext* subcontext) + : service_list_(service_list), subcontext_(subcontext), service_(nullptr) {} Result ParseSection(std::vector&& args, const std::string& filename, int line) override; Result ParseLineSection(std::vector&& args, int line) override; @@ -88,7 +82,6 @@ class ServiceParser : public SectionParser { ServiceList* service_list_; Subcontext* subcontext_; - std::optional interface_inheritance_hierarchy_; std::unique_ptr service_; std::string filename_; }; diff --git a/init/service_test.cpp b/init/service_test.cpp index a3590b536..53b53ed5a 100644 --- a/init/service_test.cpp +++ b/init/service_test.cpp @@ -253,8 +253,7 @@ service $name /system/bin/yes "$selabel", GetSecurityContext(), false); ServiceList& service_list = ServiceList::GetInstance(); Parser parser; - parser.AddSectionParser("service", - std::make_unique(&service_list, nullptr, std::nullopt)); + parser.AddSectionParser("service", std::make_unique(&service_list, nullptr)); TemporaryFile tf; ASSERT_GE(tf.fd, 0); diff --git a/init/test_utils/service_utils.cpp b/init/test_utils/service_utils.cpp index 6426ed9fd..7002a67d5 100644 --- a/init/test_utils/service_utils.cpp +++ b/init/test_utils/service_utils.cpp @@ -30,8 +30,7 @@ namespace init { android::base::Result GetOnDeviceServiceInterfacesMap() { ServiceList& service_list = ServiceList::GetInstance(); Parser parser; - parser.AddSectionParser("service", - std::make_unique(&service_list, nullptr, std::nullopt)); + parser.AddSectionParser("service", std::make_unique(&service_list, nullptr)); for (const auto& location : { "/init.rc", "/system/etc/init",