[libcamera-devel] [RFC PATCH 05/10] libcamera: ipa_module_info: add field for isolation

[Paul Elder]

Hi Laurent,

On Thu, Jun 06, 2019 at 12:51:58PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.

Thank you for the review.

> On Wed, Jun 05, 2019 at 06:18:12PM -0400, Paul Elder wrote:
> > Add a field to IPAModuleInfo that determines whether or not the IPA
> > module needs to be isolated in a separated process.
> > 
> > Also increment the IPA_MODULE_API_VERSION, due to the change to struct
> > IPAModuleInfo.
> 
> As we haven't released any stable version yet I think you can skip this.

Okay. It was trying to read modules that I had built and installed from master
and then complaining about the size mismatch.

> > Update the dummy IPA and IPA test to conform to the new struct layout.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> >  include/libcamera/ipa/ipa_module_info.h | 3 ++-
> >  src/ipa/ipa_dummy.cpp                   | 1 +
> >  test/ipa/ipa_test.cpp                   | 1 +
> >  3 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/libcamera/ipa/ipa_module_info.h b/include/libcamera/ipa/ipa_module_info.h
> > index 585f753..cb112e4 100644
> > --- a/include/libcamera/ipa/ipa_module_info.h
> > +++ b/include/libcamera/ipa/ipa_module_info.h
> > @@ -9,7 +9,7 @@
> >  
> >  #include <stdint.h>
> >  
> > -#define IPA_MODULE_API_VERSION 1
> > +#define IPA_MODULE_API_VERSION 2
> >  
> >  namespace libcamera {
> >  
> > @@ -18,6 +18,7 @@ struct IPAModuleInfo {
> >  	uint32_t pipelineVersion;
> >  	char pipelineName[256];
> >  	char name[256];
> > +	int isolate;
> 
> So what will prevent a closed-source IPA module from telling it
> shouldn't be isolated ? :-) I think we need something a bit more secure
> here.

:)

I was actually thinking about the license method, but just put this flag
for now as a draft.

> Whether to isolate a module or load it directly is a decision that
> libcamera should take itself. A very simple implementation would just
> base the decision on a configuration file, but we could also help
> libcamera decide automatically. I've been thinking about signing modules
> with a random key that is created when building libcamera and then
> thrown away, so that open-source modules built as part of libcamera
> could then be trusted.

How would that work? What about open source modules that are
out-of-tree?

> That would require more time than we should spend
> right not on this topic though. A possibly short-term approach could be
> to report the license that covers the module, and isolate all closed
> modules. It wouldn't prevent a module from facking it, but if a vendor
> decides to publish a binary-only module while explicitly stating in the
> module info that it is covered by the GPL, they will at least not be
> able to argue that they're not abusing the system.
> 
> >  } __attribute__((packed));
> >  
> >  extern "C" {
> > diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp
> > index ee7a3a8..a8ff88c 100644
> > --- a/src/ipa/ipa_dummy.cpp
> > +++ b/src/ipa/ipa_dummy.cpp
> > @@ -34,6 +34,7 @@ const struct IPAModuleInfo ipaModuleInfo = {
> >  	0,
> >  	"PipelineHandlerVimc",
> >  	"Dummy IPA for Vimc",
> > +	0,
> >  };
> >  
> >  IPAInterface *ipaCreate()
> > diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp
> > index bbef069..2682bae 100644
> > --- a/test/ipa/ipa_test.cpp
> > +++ b/test/ipa/ipa_test.cpp
> > @@ -59,6 +59,7 @@ protected:
> >  			0,
> >  			"PipelineHandlerVimc",
> >  			"Dummy IPA for Vimc",
> > +			0,
> >  		};
> >  
> >  		count += runTest("src/ipa/ipa_dummy.so", testInfo);

Thanks,

Paul