[libcamera-devel] [PATCH 2/2] libcamera: ipa_module: elfLoadSymbol find symbol regardless of size

[Laurent Pinchart]

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 at 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 at 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";

-- 
Regards,

Laurent Pinchart