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

[Kieran Bingham]

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

Fine with me too.


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