From cad62aa727a60085f98aa7aaebd5ff474c24fe01 Mon Sep 17 00:00:00 2001 From: Fabian Albert Date: Fri, 17 Jan 2025 11:53:58 +0100 Subject: [PATCH] Apply internal review suggestions --- src/lib/prov/pkcs11/p11.cpp | 32 +++++++++++---------------- src/lib/prov/pkcs11/p11.h | 2 +- src/lib/prov/pkcs11/p11_error.cpp | 8 +++++++ src/lib/prov/pkcs11/p11_interface.cpp | 5 +++++ src/lib/utils/dyn_load/dyn_load.cpp | 13 ++++++----- src/lib/utils/dyn_load/dyn_load.h | 28 ++++++++++++++++++++--- 6 files changed, 59 insertions(+), 29 deletions(-) diff --git a/src/lib/prov/pkcs11/p11.cpp b/src/lib/prov/pkcs11/p11.cpp index 4e967be270..e471e90a1e 100644 --- a/src/lib/prov/pkcs11/p11.cpp +++ b/src/lib/prov/pkcs11/p11.cpp @@ -104,15 +104,12 @@ bool LowLevel::C_GetInterfaceList(const Dynamically_Loaded_Library& pkcs11_modul Ulong* count_ptr, ReturnValue* return_value) { using get_interface_list = CK_RV (*)(Interface*, Ulong*); - get_interface_list get_interface_list_ptr; - try { - get_interface_list_ptr = pkcs11_module.resolve("C_GetInterfaceList"); - } catch(Invalid_Argument&) { - // Loading the library function failed. Probably due to a cryptoki library with PKCS #11 < 3.0. - return handle_return_value(CKR_GENERAL_ERROR, return_value); + if(auto get_interface_list_ptr = pkcs11_module.try_resolve_symbol("C_GetInterfaceList"); + get_interface_list_ptr.has_value()) { + return handle_return_value(get_interface_list_ptr.value()(interface_list_ptr, count_ptr), return_value); } - - return handle_return_value(get_interface_list_ptr(interface_list_ptr, count_ptr), return_value); + // Loading the library function failed. Probably due to a cryptoki library with PKCS #11 < 3.0. + return handle_return_value(CKR_GENERAL_ERROR, return_value); } bool LowLevel::C_GetInterface(const Dynamically_Loaded_Library& pkcs11_module, @@ -123,18 +120,15 @@ bool LowLevel::C_GetInterface(const Dynamically_Loaded_Library& pkcs11_module, ReturnValue* return_value) { using get_interface = CK_RV (*)(Utf8Char* interface_name_ptr, Version* version_ptr, Interface* interface_ptr_ptr, Flags flags); - get_interface get_interface_ptr; - try { - get_interface_ptr = pkcs11_module.resolve("C_GetInterface"); - } catch(Invalid_Argument&) { - // Loading the library function failed. Probably due to a cryptoki library with PKCS #11 < 3.0. - return handle_return_value(CKR_GENERAL_ERROR, return_value); + if(auto get_interface_ptr = pkcs11_module.try_resolve_symbol("C_GetInterface"); + get_interface_ptr.has_value()) { + return handle_return_value( + get_interface_ptr.value()( + const_cast(interface_name_ptr), const_cast(version_ptr), interface_ptr_ptr, flags), + return_value); } - - return handle_return_value( - get_interface_ptr( - const_cast(interface_name_ptr), const_cast(version_ptr), interface_ptr_ptr, flags), - return_value); + // Loading the library function failed. Probably due to a cryptoki library with PKCS #11 < 3.0. + return handle_return_value(CKR_GENERAL_ERROR, return_value); } /****************************** Slot and token management functions ******************************/ diff --git a/src/lib/prov/pkcs11/p11.h b/src/lib/prov/pkcs11/p11.h index bf86c3278d..8deece9bed 100644 --- a/src/lib/prov/pkcs11/p11.h +++ b/src/lib/prov/pkcs11/p11.h @@ -1196,7 +1196,7 @@ class BOTAN_PUBLIC_API(3, 7) InterfaceWrapper { public: /// Basic constructor using an interface. - InterfaceWrapper(Interface raw_interface) : m_interface(raw_interface) {} + InterfaceWrapper(Interface raw_interface); InterfaceWrapper(const InterfaceWrapper&) = default; InterfaceWrapper& operator=(const InterfaceWrapper&) = default; diff --git a/src/lib/prov/pkcs11/p11_error.cpp b/src/lib/prov/pkcs11/p11_error.cpp index d0a043bc1b..a39773a966 100644 --- a/src/lib/prov/pkcs11/p11_error.cpp +++ b/src/lib/prov/pkcs11/p11_error.cpp @@ -1,3 +1,11 @@ +/* +* PKCS #11 Error Information +* (C) 2025 Jack Lloyd +* (C) 2025 Fabian Albert - Rohde & Schwarz Cybersecurity GmbH +* +* Botan is released under the Simplified BSD License (see license.txt) +*/ + #include #include diff --git a/src/lib/prov/pkcs11/p11_interface.cpp b/src/lib/prov/pkcs11/p11_interface.cpp index 010a4b51b4..f3143a594d 100644 --- a/src/lib/prov/pkcs11/p11_interface.cpp +++ b/src/lib/prov/pkcs11/p11_interface.cpp @@ -43,6 +43,11 @@ std::basic_string_view name_of(const Interface& interface) { } // namespace +InterfaceWrapper::InterfaceWrapper(Interface raw_interface) : m_interface(raw_interface) { + BOTAN_ASSERT_NONNULL(raw_interface.pInterfaceName); + BOTAN_ASSERT_NONNULL(raw_interface.pFunctionList); +} + Version InterfaceWrapper::version() const { return version_of(m_interface); } diff --git a/src/lib/utils/dyn_load/dyn_load.cpp b/src/lib/utils/dyn_load/dyn_load.cpp index dba66d0984..362b5f6b86 100644 --- a/src/lib/utils/dyn_load/dyn_load.cpp +++ b/src/lib/utils/dyn_load/dyn_load.cpp @@ -66,18 +66,19 @@ Dynamically_Loaded_Library::~Dynamically_Loaded_Library() { } void* Dynamically_Loaded_Library::resolve_symbol(const std::string& symbol) const { - void* addr = nullptr; + if(void* addr = resolve_symbol_internal(symbol)) { + return addr; + } + throw Invalid_Argument(fmt("Failed to resolve symbol {} in {}", symbol, m_lib_name)); +} +void* Dynamically_Loaded_Library::resolve_symbol_internal(const std::string& symbol) const { + void* addr = nullptr; #if defined(BOTAN_TARGET_OS_HAS_POSIX1) addr = ::dlsym(m_lib, symbol.c_str()); #elif defined(BOTAN_TARGET_OS_HAS_WIN32) addr = reinterpret_cast(::GetProcAddress(reinterpret_cast(m_lib), symbol.c_str())); #endif - - if(!addr) { - throw Invalid_Argument(fmt("Failed to resolve symbol {} in {}", symbol, m_lib_name)); - } - return addr; } diff --git a/src/lib/utils/dyn_load/dyn_load.h b/src/lib/utils/dyn_load/dyn_load.h index c8a25fd2ba..74094d3b72 100644 --- a/src/lib/utils/dyn_load/dyn_load.h +++ b/src/lib/utils/dyn_load/dyn_load.h @@ -9,6 +9,7 @@ #define BOTAN_DYNAMIC_LOADER_H_ #include +#include #include namespace Botan { @@ -36,10 +37,25 @@ class BOTAN_TEST_API Dynamically_Loaded_Library final { */ ~Dynamically_Loaded_Library(); + /** + * Try to load a symbol + * @param symbol names the symbol to load + * @return address of the loaded symbol or std::nullopt if the symbol + * was not found + */ + template + std::optional try_resolve_symbol(const std::string& symbol) const + requires(std::is_pointer_v) + { + void* addr = resolve_symbol_internal(symbol); + return addr ? reinterpret_cast(addr) : std::nullopt; + } + /** * Load a symbol (or fail with an exception) * @param symbol names the symbol to load * @return address of the loaded symbol + * @throws Invalid_Argument if the symbol is not found */ void* resolve_symbol(const std::string& symbol) const; @@ -47,13 +63,19 @@ class BOTAN_TEST_API Dynamically_Loaded_Library final { * Convenience function for casting symbol to the right type * @param symbol names the symbol to load * @return address of the loaded symbol + * @throws Invalid_Argument if the symbol is not found */ - template - T resolve(const std::string& symbol) const { - return reinterpret_cast(resolve_symbol(symbol)); + template + PtrT resolve(const std::string& symbol) const + requires(std::is_pointer_v) + { + return reinterpret_cast(resolve_symbol(symbol)); } private: + /// Returns a pointer to the symbol or nullptr if the symbol is not found. + void* resolve_symbol_internal(const std::string& symbol) const; + Dynamically_Loaded_Library(const Dynamically_Loaded_Library&); Dynamically_Loaded_Library& operator=(const Dynamically_Loaded_Library&);