[{"id":1780,"web_url":"https://patchwork.libcamera.org/comment/1780/","msgid":"<20190606095158.GE12825@pendragon.ideasonboard.com>","date":"2019-06-06T09:51:58","subject":"Re: [libcamera-devel] [RFC PATCH 05/10] libcamera: ipa_module_info:\n\tadd field for isolation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Jun 05, 2019 at 06:18:12PM -0400, Paul Elder wrote:\n> Add a field to IPAModuleInfo that determines whether or not the IPA\n> module needs to be isolated in a separated process.\n> \n> Also increment the IPA_MODULE_API_VERSION, due to the change to struct\n> IPAModuleInfo.\n\nAs we haven't released any stable version yet I think you can skip this.\n\n> Update the dummy IPA and IPA test to conform to the new struct layout.\n> \n> Signed-off-by: Paul Elder \n> ---\n> include/libcamera/ipa/ipa_module_info.h | 3 ++-\n> src/ipa/ipa_dummy.cpp | 1 +\n> test/ipa/ipa_test.cpp | 1 +\n> 3 files changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/ipa/ipa_module_info.h b/include/libcamera/ipa/ipa_module_info.h\n> index 585f753..cb112e4 100644\n> --- a/include/libcamera/ipa/ipa_module_info.h\n> +++ b/include/libcamera/ipa/ipa_module_info.h\n> @@ -9,7 +9,7 @@\n> \n> #include \n> \n> -#define IPA_MODULE_API_VERSION 1\n> +#define IPA_MODULE_API_VERSION 2\n> \n> namespace libcamera {\n> \n> @@ -18,6 +18,7 @@ struct IPAModuleInfo {\n> \tuint32_t pipelineVersion;\n> \tchar pipelineName[256];\n> \tchar name[256];\n> +\tint isolate;\n\nSo what will prevent a closed-source IPA module from telling it\nshouldn't be isolated ? :-) I think we need something a bit more secure\nhere.\n\nWhether to isolate a module or load it directly is a decision that\nlibcamera should take itself. A very simple implementation would just\nbase the decision on a configuration file, but we could also help\nlibcamera decide automatically. I've been thinking about signing modules\nwith a random key that is created when building libcamera and then\nthrown away, so that open-source modules built as part of libcamera\ncould then be trusted. That would require more time than we should spend\nright not on this topic though. A possibly short-term approach could be\nto report the license that covers the module, and isolate all closed\nmodules. It wouldn't prevent a module from facking it, but if a vendor\ndecides to publish a binary-only module while explicitly stating in the\nmodule info that it is covered by the GPL, they will at least not be\nable to argue that they're not abusing the system.\n\n> } __attribute__((packed));\n> \n> extern \"C\" {\n> diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp\n> index ee7a3a8..a8ff88c 100644\n> --- a/src/ipa/ipa_dummy.cpp\n> +++ b/src/ipa/ipa_dummy.cpp\n> @@ -34,6 +34,7 @@ const struct IPAModuleInfo ipaModuleInfo = {\n> \t0,\n> \t\"PipelineHandlerVimc\",\n> \t\"Dummy IPA for Vimc\",\n> +\t0,\n> };\n> \n> IPAInterface *ipaCreate()\n> diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp\n> index bbef069..2682bae 100644\n> --- a/test/ipa/ipa_test.cpp\n> +++ b/test/ipa/ipa_test.cpp\n> @@ -59,6 +59,7 @@ protected:\n> \t\t\t0,\n> \t\t\t\"PipelineHandlerVimc\",\n> \t\t\t\"Dummy IPA for Vimc\",\n> +\t\t\t0,\n> \t\t};\n> \n> \t\tcount += runTest(\"src/ipa/ipa_dummy.so\", testInfo);","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1790767BB7\n\tfor ;\n\tThu, 6 Jun 2019 11:52:13 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:a03f:44f0:8500:ca05:8177:199c:fed4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 990F633B;\n\tThu, 6 Jun 2019 11:52:12 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559814732;\n\tbh=nnPYzHxqUpJljpV0cdOUfj4ElyPTPxPpceqM6B0YGkU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=F8kIhwGKR1mnqf8ot2/R4Vjt56Gsn578sKc2yPpeHTNl/6mDs34PlscJG3puQnlUF\n\tWVqwPHh3VrYi9+c4LtZjiJNUHmQ6FQ2TEqwM17EdF6XDXxJDYHl/SySixDgCrEuMhX\n\tFStDFjmkdGhQ5j3FE/vCDOty5i+9Gqb1Zn0OIMf4=","Date":"Thu, 6 Jun 2019 12:51:58 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190606095158.GE12825@pendragon.ideasonboard.com>","References":"<20190605221817.966-1-paul.elder@ideasonboard.com>\n\t<20190605221817.966-6-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190605221817.966-6-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH 05/10] libcamera: ipa_module_info:\n\tadd field for isolation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 06 Jun 2019 09:52:13 -0000"}},{"id":1783,"web_url":"https://patchwork.libcamera.org/comment/1783/","msgid":"<20190606143947.GT32191@localhost.localdomain>","date":"2019-06-06T14:39:47","subject":"Re: [libcamera-devel] [RFC PATCH 05/10] libcamera: ipa_module_info:\n\tadd field for isolation","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Thu, Jun 06, 2019 at 12:51:58PM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n\nThank you for the review.\n\n> On Wed, Jun 05, 2019 at 06:18:12PM -0400, Paul Elder wrote:\n> > Add a field to IPAModuleInfo that determines whether or not the IPA\n> > module needs to be isolated in a separated process.\n> > \n> > Also increment the IPA_MODULE_API_VERSION, due to the change to struct\n> > IPAModuleInfo.\n> \n> As we haven't released any stable version yet I think you can skip this.\n\nOkay. It was trying to read modules that I had built and installed from master\nand then complaining about the size mismatch.\n\n> > Update the dummy IPA and IPA test to conform to the new struct layout.\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > include/libcamera/ipa/ipa_module_info.h | 3 ++-\n> > src/ipa/ipa_dummy.cpp | 1 +\n> > test/ipa/ipa_test.cpp | 1 +\n> > 3 files changed, 4 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/include/libcamera/ipa/ipa_module_info.h b/include/libcamera/ipa/ipa_module_info.h\n> > index 585f753..cb112e4 100644\n> > --- a/include/libcamera/ipa/ipa_module_info.h\n> > +++ b/include/libcamera/ipa/ipa_module_info.h\n> > @@ -9,7 +9,7 @@\n> > \n> > #include \n> > \n> > -#define IPA_MODULE_API_VERSION 1\n> > +#define IPA_MODULE_API_VERSION 2\n> > \n> > namespace libcamera {\n> > \n> > @@ -18,6 +18,7 @@ struct IPAModuleInfo {\n> > \tuint32_t pipelineVersion;\n> > \tchar pipelineName[256];\n> > \tchar name[256];\n> > +\tint isolate;\n> \n> So what will prevent a closed-source IPA module from telling it\n> shouldn't be isolated ? :-) I think we need something a bit more secure\n> here.\n\n:)\n\nI was actually thinking about the license method, but just put this flag\nfor now as a draft.\n\n> Whether to isolate a module or load it directly is a decision that\n> libcamera should take itself. A very simple implementation would just\n> base the decision on a configuration file, but we could also help\n> libcamera decide automatically. I've been thinking about signing modules\n> with a random key that is created when building libcamera and then\n> thrown away, so that open-source modules built as part of libcamera\n> could then be trusted.\n\nHow would that work? What about open source modules that are\nout-of-tree?\n\n> That would require more time than we should spend\n> right not on this topic though. A possibly short-term approach could be\n> to report the license that covers the module, and isolate all closed\n> modules. It wouldn't prevent a module from facking it, but if a vendor\n> decides to publish a binary-only module while explicitly stating in the\n> module info that it is covered by the GPL, they will at least not be\n> able to argue that they're not abusing the system.\n> \n> > } __attribute__((packed));\n> > \n> > extern \"C\" {\n> > diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp\n> > index ee7a3a8..a8ff88c 100644\n> > --- a/src/ipa/ipa_dummy.cpp\n> > +++ b/src/ipa/ipa_dummy.cpp\n> > @@ -34,6 +34,7 @@ const struct IPAModuleInfo ipaModuleInfo = {\n> > \t0,\n> > \t\"PipelineHandlerVimc\",\n> > \t\"Dummy IPA for Vimc\",\n> > +\t0,\n> > };\n> > \n> > IPAInterface *ipaCreate()\n> > diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp\n> > index bbef069..2682bae 100644\n> > --- a/test/ipa/ipa_test.cpp\n> > +++ b/test/ipa/ipa_test.cpp\n> > @@ -59,6 +59,7 @@ protected:\n> > \t\t\t0,\n> > \t\t\t\"PipelineHandlerVimc\",\n> > \t\t\t\"Dummy IPA for Vimc\",\n> > +\t\t\t0,\n> > \t\t};\n> > \n> > \t\tcount += runTest(\"src/ipa/ipa_dummy.so\", testInfo);\n\nThanks,\n\nPaul","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 21D246835E\n\tfor ;\n\tThu, 6 Jun 2019 16:39:55 +0200 (CEST)","from localhost.localdomain (unknown [96.44.9.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6475633B;\n\tThu, 6 Jun 2019 16:39:54 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559831994;\n\tbh=nQqpRSRleIeT01n4H27DKSH/Up/uWL/AIYdORsgcl/g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mW2WDHOS0/g8nyiGwe7uqY9biVQ9y8n1di6LbtI4xrA22/HiPIQqhIglPXxBCtfun\n\t+ki++NC9Y1Hd25a/1W7icQegADOQmMA/4Kk9nNVkfl5N2oVFzUNaH4c2tSy7bzTqo5\n\t9yYENca+Yv9CET1Ox4Kb+61ijWVpSBAw/6l897UY=","Date":"Thu, 6 Jun 2019 10:39:47 -0400","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190606143947.GT32191@localhost.localdomain>","References":"<20190605221817.966-1-paul.elder@ideasonboard.com>\n\t<20190605221817.966-6-paul.elder@ideasonboard.com>\n\t<20190606095158.GE12825@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20190606095158.GE12825@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH 05/10] libcamera: ipa_module_info:\n\tadd field for isolation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 06 Jun 2019 14:39:55 -0000"}},{"id":1784,"web_url":"https://patchwork.libcamera.org/comment/1784/","msgid":"<20190606155849.GH12825@pendragon.ideasonboard.com>","date":"2019-06-06T15:58:49","subject":"Re: [libcamera-devel] [RFC PATCH 05/10] libcamera: ipa_module_info:\n\tadd field for isolation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Thu, Jun 06, 2019 at 10:39:47AM -0400, Paul Elder wrote:\n> On Thu, Jun 06, 2019 at 12:51:58PM +0300, Laurent Pinchart wrote:\n> > On Wed, Jun 05, 2019 at 06:18:12PM -0400, Paul Elder wrote:\n> >> Add a field to IPAModuleInfo that determines whether or not the IPA\n> >> module needs to be isolated in a separated process.\n> >> \n> >> Also increment the IPA_MODULE_API_VERSION, due to the change to struct\n> >> IPAModuleInfo.\n> > \n> > As we haven't released any stable version yet I think you can skip this.\n> \n> Okay. It was trying to read modules that I had built and installed from master\n> and then complaining about the size mismatch.\n> \n> >> Update the dummy IPA and IPA test to conform to the new struct layout.\n> >> \n> >> Signed-off-by: Paul Elder \n> >> ---\n> >> include/libcamera/ipa/ipa_module_info.h | 3 ++-\n> >> src/ipa/ipa_dummy.cpp | 1 +\n> >> test/ipa/ipa_test.cpp | 1 +\n> >> 3 files changed, 4 insertions(+), 1 deletion(-)\n> >> \n> >> diff --git a/include/libcamera/ipa/ipa_module_info.h b/include/libcamera/ipa/ipa_module_info.h\n> >> index 585f753..cb112e4 100644\n> >> --- a/include/libcamera/ipa/ipa_module_info.h\n> >> +++ b/include/libcamera/ipa/ipa_module_info.h\n> >> @@ -9,7 +9,7 @@\n> >> \n> >> #include \n> >> \n> >> -#define IPA_MODULE_API_VERSION 1\n> >> +#define IPA_MODULE_API_VERSION 2\n> >> \n> >> namespace libcamera {\n> >> \n> >> @@ -18,6 +18,7 @@ struct IPAModuleInfo {\n> >> \tuint32_t pipelineVersion;\n> >> \tchar pipelineName[256];\n> >> \tchar name[256];\n> >> +\tint isolate;\n> > \n> > So what will prevent a closed-source IPA module from telling it\n> > shouldn't be isolated ? :-) I think we need something a bit more secure\n> > here.\n> \n> :)\n> \n> I was actually thinking about the license method, but just put this flag\n> for now as a draft.\n> \n> > Whether to isolate a module or load it directly is a decision that\n> > libcamera should take itself. A very simple implementation would just\n> > base the decision on a configuration file, but we could also help\n> > libcamera decide automatically. I've been thinking about signing modules\n> > with a random key that is created when building libcamera and then\n> > thrown away, so that open-source modules built as part of libcamera\n> > could then be trusted.\n> \n> How would that work? What about open source modules that are\n> out-of-tree?\n\nIn that case out-of-tree modules would be treated as closed-source\nmodules, but that may be a feature more than a bug. The decision to load\na module directly should be based on the trust we put in the module.\nHaving access to the source code increases the trust, but possibly only\nmarginally if the module is out-of-tree.\n\n> > That would require more time than we should spend\n> > right not on this topic though. A possibly short-term approach could be\n> > to report the license that covers the module, and isolate all closed\n> > modules. It wouldn't prevent a module from facking it, but if a vendor\n> > decides to publish a binary-only module while explicitly stating in the\n> > module info that it is covered by the GPL, they will at least not be\n> > able to argue that they're not abusing the system.\n> > \n> >> } __attribute__((packed));\n> >> \n> >> extern \"C\" {\n> >> diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp\n> >> index ee7a3a8..a8ff88c 100644\n> >> --- a/src/ipa/ipa_dummy.cpp\n> >> +++ b/src/ipa/ipa_dummy.cpp\n> >> @@ -34,6 +34,7 @@ const struct IPAModuleInfo ipaModuleInfo = {\n> >> \t0,\n> >> \t\"PipelineHandlerVimc\",\n> >> \t\"Dummy IPA for Vimc\",\n> >> +\t0,\n> >> };\n> >> \n> >> IPAInterface *ipaCreate()\n> >> diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp\n> >> index bbef069..2682bae 100644\n> >> --- a/test/ipa/ipa_test.cpp\n> >> +++ b/test/ipa/ipa_test.cpp\n> >> @@ -59,6 +59,7 @@ protected:\n> >> \t\t\t0,\n> >> \t\t\t\"PipelineHandlerVimc\",\n> >> \t\t\t\"Dummy IPA for Vimc\",\n> >> +\t\t\t0,\n> >> \t\t};\n> >> \n> >> \t\tcount += runTest(\"src/ipa/ipa_dummy.so\", testInfo);","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1FD2F685B1\n\tfor ;\n\tThu, 6 Jun 2019 17:59:04 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:a03f:44f0:8500:ca05:8177:199c:fed4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A79A933B;\n\tThu, 6 Jun 2019 17:59:03 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559836743;\n\tbh=W4HS8QX8kShuGO2MnZOxVtTSZBGkii0++Xg07VlBsnM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PR6Rf40IT4/glHCuYd+Z+NlCoD4ULqhFTpLvz1AeHGt/0g5JXAhlVroNgHwQQ9obd\n\tM8ykiI1s8K1JbmPDmqFEpnG4Hdylb+zGTcaVv1Yk/Mz++L0Nha2h4pGGlnBv9vm97J\n\tWY+j0OnVKp49F6uOt3HuADIBGiEroQ1XhiA6JR6A=","Date":"Thu, 6 Jun 2019 18:58:49 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190606155849.GH12825@pendragon.ideasonboard.com>","References":"<20190605221817.966-1-paul.elder@ideasonboard.com>\n\t<20190605221817.966-6-paul.elder@ideasonboard.com>\n\t<20190606095158.GE12825@pendragon.ideasonboard.com>\n\t<20190606143947.GT32191@localhost.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190606143947.GT32191@localhost.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH 05/10] libcamera: ipa_module_info:\n\tadd field for isolation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 06 Jun 2019 15:59:04 -0000"}}]