[libcamera-devel] [RFC/PATCH] libcamera: ipa_module: Relax ipaModuleInfo symbol size check

[Jacopo Mondi]

Hi Laurent

On Thu, Dec 22, 2022 at 01:31:22PM +0200, Laurent Pinchart via libcamera-devel wrote:
> When an IPA module is loaded, the loadIPAModuleInfo() function validates
> the ipaModuleInfo structure. As part of that process, it checks that the
> ipaModuleInfo symbol size matches the expected structure size. This
> check breaks with clang and ASan, as the LLVM's address sanitizer
> implementation includes the redzone after the structure in the symbol
> size, currently growing it by 156 bytes (on x86-64). This causes all IPA
> modules to fail to load.
>
> Fix the problem by relaxing the size check to only ensure that the
> symbol is large enough to contain the structure.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Relaxing checks increases the chance of false negatives, but I think
> it's totally safe in this case. If we want to validate the structure
> size, we should add a size field within the data. Other candidates for
> new fields would be a magic number.

I'm not sure it is worth the effort, even if we allow larger IPA
modules to be accepted, we only copy the relevant portion to the info_
module, so it -should- theoretically be safe as it was before ?

As I don't have better proposals:
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

> ---
>  src/libcamera/ipa_module.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index c9ff7de30e21..c152153c180f 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -288,12 +288,12 @@ int IPAModule::loadIPAModuleInfo()
>  	}
>
>  	Span<const uint8_t> info = elfLoadSymbol(data, "ipaModuleInfo");
> -	if (info.size() != sizeof(info_)) {
> +	if (info.size() < sizeof(info_)) {
>  		LOG(IPAModule, Error) << "IPA module has no valid info";
>  		return -EINVAL;
>  	}
>
> -	memcpy(&info_, info.data(), info.size());
> +	memcpy(&info_, info.data(), sizeof(info_));
>
>  	if (info_.moduleAPIVersion != IPA_MODULE_API_VERSION) {
>  		LOG(IPAModule, Error) << "IPA module API version mismatch";
>
> base-commit: f66a5c447b65bce774a1bc2d01034f437bf764b5
> --
> Regards,
>
> Laurent Pinchart
>