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

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

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.

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

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. 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);

-- 
Regards,

Laurent Pinchart