[libcamera-devel,RFC/PATCH] libcamera: ipa_module: Relax ipaModuleInfo symbol size check
diff mbox series

Message ID 20221222113122.28662-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit d81505b834105ee1c879a962a2911d08b14ad5fd
Headers show
Series
  • [libcamera-devel,RFC/PATCH] libcamera: ipa_module: Relax ipaModuleInfo symbol size check
Related show

Commit Message

Laurent Pinchart Dec. 22, 2022, 11:31 a.m. UTC 
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@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.
---
 src/libcamera/ipa_module.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


base-commit: f66a5c447b65bce774a1bc2d01034f437bf764b5

Comments

Laurent Pinchart Dec. 22, 2022, 11:29 p.m. UTC | #1 
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.

By the way, if anyone is interested in the topic of ASan and redzone,
https://www.usenix.org/system/files/sec22summer_zhang-yuchen.pdf is an
interesting read. A patch has been proposed to emit the .size directive
for global objects before adding the redzone, but it was rejected:
https://reviews.llvm.org/D123010?id=420100.

> 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@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.
> ---
>  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
Jacopo Mondi Jan. 16, 2023, 8:46 a.m. UTC | #2 
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@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@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
>
Kieran Bingham Jan. 16, 2023, 12:43 p.m. UTC | #3 
Quoting Jacopo Mondi via libcamera-devel (2023-01-16 08:46:56)
> 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@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@ideasonboard.com>

Fine with me too.


Reviewed-by: Kieran Bingham <kieran.bingham@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
> >

Patch
diff mbox series 

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