[libcamera-devel,2/2] libcamera: ipa_module: elfLoadSymbol find symbol regardless of size
diff mbox series

Message ID 20190701141336.10273-2-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • [libcamera-devel,v2,1/2] libcamera: ipa_module: add path to module loading error message
Related show

Commit Message

Paul Elder July 1, 2019, 2:13 p.m. UTC 
Make elfLoadSymbol more generic by making the symbol size an output
rather than an input. Also move the memcpy out of elfLoadSymbol.

If the size of struct IPAModuleInfo changes between versions, we still
want to be able to load it and perhaps do conversions for backwards
compatibility. In this case the size should not be a restriction when
searching for the symbol.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/libcamera/ipa_module.cpp | 64 ++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 24 deletions(-)

Comments

Laurent Pinchart July 1, 2019, 5:43 p.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Mon, Jul 01, 2019 at 11:13:36PM +0900, Paul Elder wrote:
> Make elfLoadSymbol more generic by making the symbol size an output
> rather than an input. Also move the memcpy out of elfLoadSymbol.
> 
> If the size of struct IPAModuleInfo changes between versions, we still
> want to be able to load it and perhaps do conversions for backwards
> compatibility. In this case the size should not be a restriction when
> searching for the symbol.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/libcamera/ipa_module.cpp | 64 ++++++++++++++++++++++--------------
>  1 file changed, 40 insertions(+), 24 deletions(-)
> 
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index 5a46ec3..d82ac69 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -17,6 +17,8 @@
>  #include <sys/types.h>
>  #include <unistd.h>
>  
> +#include <tuple>
> +

No need to separate this from the previous headers, it can fit between
sys/types.h and unistd.h.

>  #include "log.h"
>  #include "pipeline_handler.h"
>  
> @@ -81,18 +83,27 @@ int elfVerifyIdent(void *map, size_t soSize)
>  	return 0;
>  }
>  
> +/**
> + * \brief Retrieve address and size of a symbol from an mmap'ed ELF file
> + * \param[in] map Address of mmap'ed ELF file
> + * \param[in] soSize Size of mmap'ed ELF file (in bytes)
> + * \param[in] symbol Symbol name
> + *
> + * \return zero or error code, address or nullptr, size of symbol or zero,
> + * respectively
> + */
>  template<class ElfHeader, class SecHeader, class SymHeader>
> -int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
> -			  const char *symbol)
> +std::tuple<int, void *, size_t>
> +elfLoadSymbol(void *map, size_t soSize, const char *symbol)
>  {
>  	ElfHeader *eHdr = elfPointer<ElfHeader>(map, 0, soSize);
>  	if (!eHdr)
> -		return -ENOEXEC;
> +		return std::make_tuple(-ENOEXEC, nullptr, 0);
>  
>  	off_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;
>  	SecHeader *sHdr = elfPointer<SecHeader>(map, offset, soSize);
>  	if (!sHdr)
> -		return -ENOEXEC;
> +		return std::make_tuple(-ENOEXEC, nullptr, 0);
>  	off_t shnameoff = sHdr->sh_offset;
>  
>  	/* Locate .dynsym section header. */
> @@ -101,12 +112,12 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
>  		offset = eHdr->e_shoff + eHdr->e_shentsize * i;
>  		sHdr = elfPointer<SecHeader>(map, offset, soSize);
>  		if (!sHdr)
> -			return -ENOEXEC;
> +			return std::make_tuple(-ENOEXEC, nullptr, 0);
>  
>  		offset = shnameoff + sHdr->sh_name;
>  		char *name = elfPointer<char[8]>(map, offset, soSize);
>  		if (!name)
> -			return -ENOEXEC;
> +			return std::make_tuple(-ENOEXEC, nullptr, 0);
>  
>  		if (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, ".dynsym")) {
>  			dynsym = sHdr;
> @@ -116,13 +127,13 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
>  
>  	if (dynsym == nullptr) {
>  		LOG(IPAModule, Error) << "ELF has no .dynsym section";
> -		return -ENOEXEC;
> +		return std::make_tuple(-ENOEXEC, nullptr, 0);
>  	}
>  
>  	offset = eHdr->e_shoff + eHdr->e_shentsize * dynsym->sh_link;
>  	sHdr = elfPointer<SecHeader>(map, offset, soSize);
>  	if (!sHdr)
> -		return -ENOEXEC;
> +		return std::make_tuple(-ENOEXEC, nullptr, 0);
>  	off_t dynsym_nameoff = sHdr->sh_offset;
>  
>  	/* Locate symbol in the .dynsym section. */
> @@ -132,16 +143,16 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
>  		offset = dynsym->sh_offset + dynsym->sh_entsize * i;
>  		SymHeader *sym = elfPointer<SymHeader>(map, offset, soSize);
>  		if (!sym)
> -			return -ENOEXEC;
> +			return std::make_tuple(-ENOEXEC, nullptr, 0);
>  
>  		offset = dynsym_nameoff + sym->st_name;
>  		char *name = elfPointer<char>(map, offset, soSize,
>  					      strlen(symbol) + 1);
>  		if (!name)
> -			return -ENOEXEC;
> +			return std::make_tuple(-ENOEXEC, nullptr, 0);
>  
>  		if (!strcmp(name, symbol) &&
> -		    sym->st_info & STB_GLOBAL && sym->st_size == size) {
> +		    sym->st_info & STB_GLOBAL) {
>  			targetSymbol = sym;
>  			break;
>  		}
> @@ -149,24 +160,22 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
>  
>  	if (targetSymbol == nullptr) {
>  		LOG(IPAModule, Error) << "Symbol " << symbol << " not found";
> -		return -ENOEXEC;
> +		return std::make_tuple(-ENOEXEC, nullptr, 0);
>  	}
>  
>  	/* Locate and return data of symbol. */
>  	if (targetSymbol->st_shndx >= eHdr->e_shnum)
> -		return -ENOEXEC;
> +		return std::make_tuple(-ENOEXEC, nullptr, 0);
>  	offset = eHdr->e_shoff + targetSymbol->st_shndx * eHdr->e_shentsize;
>  	sHdr = elfPointer<SecHeader>(map, offset, soSize);
>  	if (!sHdr)
> -		return -ENOEXEC;
> +		return std::make_tuple(-ENOEXEC, nullptr, 0);
>  	offset = sHdr->sh_offset + (targetSymbol->st_value - sHdr->sh_addr);
> -	char *data = elfPointer<char>(map, offset, soSize, size);
> +	char *data = elfPointer<char>(map, offset, soSize, targetSymbol->st_size);
>  	if (!data)
> -		return -ENOEXEC;
> -
> -	memcpy(dst, data, size);
> +		return std::make_tuple(-ENOEXEC, nullptr, 0);
>  
> -	return 0;
> +	return std::make_tuple(0, data, targetSymbol->st_size);

As the only error code that is returned is -ENOEXEC, would it make sense
to drop it and base the error check on the address being nullptr ? I
don't really mind either way, so with or without the change (but with
the include issue fixed),

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  }
>  
>  } /* namespace */
> @@ -257,8 +266,10 @@ int IPAModule::loadIPAModuleInfo()
>  		return ret;
>  	}
>  
> -	size_t soSize;
> +	void *data;
> +	size_t dataSize;
>  	void *map;
> +	size_t soSize;
>  	struct stat st;
>  	int ret = fstat(fd, &st);
>  	if (ret < 0)
> @@ -275,11 +286,16 @@ int IPAModule::loadIPAModuleInfo()
>  		goto unmap;
>  
>  	if (sizeof(unsigned long) == 4)
> -		ret = elfLoadSymbol<Elf32_Ehdr, Elf32_Shdr, Elf32_Sym>
> -				   (&info_, sizeof(info_), map, soSize, "ipaModuleInfo");
> +		std::tie(ret, data, dataSize) =
> +			elfLoadSymbol<Elf32_Ehdr, Elf32_Shdr, Elf32_Sym>
> +				     (map, soSize, "ipaModuleInfo");
>  	else
> -		ret = elfLoadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>
> -				   (&info_, sizeof(info_), map, soSize, "ipaModuleInfo");
> +		std::tie(ret, data, dataSize) =
> +			elfLoadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>
> +				     (map, soSize, "ipaModuleInfo");
> +
> +	if (!ret && dataSize == sizeof(info_))
> +		memcpy(&info_, data, dataSize);
>  
>  	if (info_.moduleAPIVersion != IPA_MODULE_API_VERSION) {
>  		LOG(IPAModule, Error) << "IPA module API version mismatch";

Patch
diff mbox series 

diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
index 5a46ec3..d82ac69 100644
--- a/src/libcamera/ipa_module.cpp
+++ b/src/libcamera/ipa_module.cpp
@@ -17,6 +17,8 @@ 
 #include <sys/types.h>
 #include <unistd.h>
 
+#include <tuple>
+
 #include "log.h"
 #include "pipeline_handler.h"
 
@@ -81,18 +83,27 @@  int elfVerifyIdent(void *map, size_t soSize)
 	return 0;
 }
 
+/**
+ * \brief Retrieve address and size of a symbol from an mmap'ed ELF file
+ * \param[in] map Address of mmap'ed ELF file
+ * \param[in] soSize Size of mmap'ed ELF file (in bytes)
+ * \param[in] symbol Symbol name
+ *
+ * \return zero or error code, address or nullptr, size of symbol or zero,
+ * respectively
+ */
 template<class ElfHeader, class SecHeader, class SymHeader>
-int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
-			  const char *symbol)
+std::tuple<int, void *, size_t>
+elfLoadSymbol(void *map, size_t soSize, const char *symbol)
 {
 	ElfHeader *eHdr = elfPointer<ElfHeader>(map, 0, soSize);
 	if (!eHdr)
-		return -ENOEXEC;
+		return std::make_tuple(-ENOEXEC, nullptr, 0);
 
 	off_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;
 	SecHeader *sHdr = elfPointer<SecHeader>(map, offset, soSize);
 	if (!sHdr)
-		return -ENOEXEC;
+		return std::make_tuple(-ENOEXEC, nullptr, 0);
 	off_t shnameoff = sHdr->sh_offset;
 
 	/* Locate .dynsym section header. */
@@ -101,12 +112,12 @@  int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
 		offset = eHdr->e_shoff + eHdr->e_shentsize * i;
 		sHdr = elfPointer<SecHeader>(map, offset, soSize);
 		if (!sHdr)
-			return -ENOEXEC;
+			return std::make_tuple(-ENOEXEC, nullptr, 0);
 
 		offset = shnameoff + sHdr->sh_name;
 		char *name = elfPointer<char[8]>(map, offset, soSize);
 		if (!name)
-			return -ENOEXEC;
+			return std::make_tuple(-ENOEXEC, nullptr, 0);
 
 		if (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, ".dynsym")) {
 			dynsym = sHdr;
@@ -116,13 +127,13 @@  int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
 
 	if (dynsym == nullptr) {
 		LOG(IPAModule, Error) << "ELF has no .dynsym section";
-		return -ENOEXEC;
+		return std::make_tuple(-ENOEXEC, nullptr, 0);
 	}
 
 	offset = eHdr->e_shoff + eHdr->e_shentsize * dynsym->sh_link;
 	sHdr = elfPointer<SecHeader>(map, offset, soSize);
 	if (!sHdr)
-		return -ENOEXEC;
+		return std::make_tuple(-ENOEXEC, nullptr, 0);
 	off_t dynsym_nameoff = sHdr->sh_offset;
 
 	/* Locate symbol in the .dynsym section. */
@@ -132,16 +143,16 @@  int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
 		offset = dynsym->sh_offset + dynsym->sh_entsize * i;
 		SymHeader *sym = elfPointer<SymHeader>(map, offset, soSize);
 		if (!sym)
-			return -ENOEXEC;
+			return std::make_tuple(-ENOEXEC, nullptr, 0);
 
 		offset = dynsym_nameoff + sym->st_name;
 		char *name = elfPointer<char>(map, offset, soSize,
 					      strlen(symbol) + 1);
 		if (!name)
-			return -ENOEXEC;
+			return std::make_tuple(-ENOEXEC, nullptr, 0);
 
 		if (!strcmp(name, symbol) &&
-		    sym->st_info & STB_GLOBAL && sym->st_size == size) {
+		    sym->st_info & STB_GLOBAL) {
 			targetSymbol = sym;
 			break;
 		}
@@ -149,24 +160,22 @@  int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
 
 	if (targetSymbol == nullptr) {
 		LOG(IPAModule, Error) << "Symbol " << symbol << " not found";
-		return -ENOEXEC;
+		return std::make_tuple(-ENOEXEC, nullptr, 0);
 	}
 
 	/* Locate and return data of symbol. */
 	if (targetSymbol->st_shndx >= eHdr->e_shnum)
-		return -ENOEXEC;
+		return std::make_tuple(-ENOEXEC, nullptr, 0);
 	offset = eHdr->e_shoff + targetSymbol->st_shndx * eHdr->e_shentsize;
 	sHdr = elfPointer<SecHeader>(map, offset, soSize);
 	if (!sHdr)
-		return -ENOEXEC;
+		return std::make_tuple(-ENOEXEC, nullptr, 0);
 	offset = sHdr->sh_offset + (targetSymbol->st_value - sHdr->sh_addr);
-	char *data = elfPointer<char>(map, offset, soSize, size);
+	char *data = elfPointer<char>(map, offset, soSize, targetSymbol->st_size);
 	if (!data)
-		return -ENOEXEC;
-
-	memcpy(dst, data, size);
+		return std::make_tuple(-ENOEXEC, nullptr, 0);
 
-	return 0;
+	return std::make_tuple(0, data, targetSymbol->st_size);
 }
 
 } /* namespace */
@@ -257,8 +266,10 @@  int IPAModule::loadIPAModuleInfo()
 		return ret;
 	}
 
-	size_t soSize;
+	void *data;
+	size_t dataSize;
 	void *map;
+	size_t soSize;
 	struct stat st;
 	int ret = fstat(fd, &st);
 	if (ret < 0)
@@ -275,11 +286,16 @@  int IPAModule::loadIPAModuleInfo()
 		goto unmap;
 
 	if (sizeof(unsigned long) == 4)
-		ret = elfLoadSymbol<Elf32_Ehdr, Elf32_Shdr, Elf32_Sym>
-				   (&info_, sizeof(info_), map, soSize, "ipaModuleInfo");
+		std::tie(ret, data, dataSize) =
+			elfLoadSymbol<Elf32_Ehdr, Elf32_Shdr, Elf32_Sym>
+				     (map, soSize, "ipaModuleInfo");
 	else
-		ret = elfLoadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>
-				   (&info_, sizeof(info_), map, soSize, "ipaModuleInfo");
+		std::tie(ret, data, dataSize) =
+			elfLoadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>
+				     (map, soSize, "ipaModuleInfo");
+
+	if (!ret && dataSize == sizeof(info_))
+		memcpy(&info_, data, dataSize);
 
 	if (info_.moduleAPIVersion != IPA_MODULE_API_VERSION) {
 		LOG(IPAModule, Error) << "IPA module API version mismatch";