| Message ID | 20190605221817.966-6-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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);
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@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
Hi Paul, On Thu, Jun 06, 2019 at 10:39:47AM -0400, Paul Elder wrote: > On Thu, Jun 06, 2019 at 12:51:58PM +0300, Laurent Pinchart wrote: > > 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@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? In that case out-of-tree modules would be treated as closed-source modules, but that may be a feature more than a bug. The decision to load a module directly should be based on the trust we put in the module. Having access to the source code increases the trust, but possibly only marginally if the module is 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);
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; } __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);
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. Update the dummy IPA and IPA test to conform to the new struct layout. Signed-off-by: Paul Elder <paul.elder@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(-)