[{"id":26136,"web_url":"https://patchwork.libcamera.org/comment/26136/","msgid":"","date":"2022-12-22T23:29:31","subject":"Re: [libcamera-devel] [RFC/PATCH] libcamera: ipa_module: Relax\n\tipaModuleInfo symbol size check","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Dec 22, 2022 at 01:31:22PM +0200, Laurent Pinchart via libcamera-devel wrote:\n> When an IPA module is loaded, the loadIPAModuleInfo() function validates\n> the ipaModuleInfo structure. As part of that process, it checks that the\n> ipaModuleInfo symbol size matches the expected structure size. This\n> check breaks with clang and ASan, as the LLVM's address sanitizer\n> implementation includes the redzone after the structure in the symbol\n> size, currently growing it by 156 bytes (on x86-64). This causes all IPA\n> modules to fail to load.\n\nBy the way, if anyone is interested in the topic of ASan and redzone,\nhttps://www.usenix.org/system/files/sec22summer_zhang-yuchen.pdf is an\ninteresting read. A patch has been proposed to emit the .size directive\nfor global objects before adding the redzone, but it was rejected:\nhttps://reviews.llvm.org/D123010?id=420100.\n\n> Fix the problem by relaxing the size check to only ensure that the\n> symbol is large enough to contain the structure.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> Relaxing checks increases the chance of false negatives, but I think\n> it's totally safe in this case. If we want to validate the structure\n> size, we should add a size field within the data. Other candidates for\n> new fields would be a magic number.\n> ---\n> src/libcamera/ipa_module.cpp | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index c9ff7de30e21..c152153c180f 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -288,12 +288,12 @@ int IPAModule::loadIPAModuleInfo()\n> \t}\n> \n> \tSpan info = elfLoadSymbol(data, \"ipaModuleInfo\");\n> -\tif (info.size() != sizeof(info_)) {\n> +\tif (info.size() < sizeof(info_)) {\n> \t\tLOG(IPAModule, Error) << \"IPA module has no valid info\";\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tmemcpy(&info_, info.data(), info.size());\n> +\tmemcpy(&info_, info.data(), sizeof(info_));\n> \n> \tif (info_.moduleAPIVersion != IPA_MODULE_API_VERSION) {\n> \t\tLOG(IPAModule, Error) << \"IPA module API version mismatch\";\n> \n> base-commit: f66a5c447b65bce774a1bc2d01034f437bf764b5","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6438FBD78E\n\tfor ;\n\tThu, 22 Dec 2022 23:29:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9EE64633B1;\n\tFri, 23 Dec 2022 00:29:38 +0100 (CET)","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 A335461F14\n\tfor ;\n\tFri, 23 Dec 2022 00:29:36 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 01E2A93\n\tfor ;\n\tFri, 23 Dec 2022 00:29:35 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671751778;\n\tbh=JvySv0rH1tKl1dMrdw770Oh4fuLMieNX+HtobGr/Yfw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=2i0Lqn/PJPw71EQakilAy3qSNGNcqcXNmvjPipbwfQdEqc6G33QtxOprOqeTHe7DB\n\tVxd9kvAUrx3MIyMIMMiTuF5sEFOBEiWDZuDQqDz+OSeehPANoi7qThWA51Hfp7o7xt\n\t6iuCL82qhtEVbX3uxQp5g6xwdoEGMULuI3N5OxPWlUlrDip0tLBd4ZE40vBu2Dgm5m\n\trSMn8l81rexF9Yu3ny8Mgn8gfEHa4ntkVstNoi1jsB0MzWDZMJZku1rI1PYC58tRBd\n\tS33JXhplcao7IiJGxKuCpjzxDrJrCmsN4hl9jygeHM3yxQF9E6/Fdl4e5y4ASgNnhp\n\tOTjdgR/GLucgg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671751776;\n\tbh=JvySv0rH1tKl1dMrdw770Oh4fuLMieNX+HtobGr/Yfw=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=HN3R81Mo2GEalzJFzlEGW5YGM6UHpoWFf7ibM9rcUWTM1V3vgjJvdXmUU2B8seeOZ\n\tGlzp2zuxUKvB3XbkoyzHrZ+JTRHYgjVXnGdk5Imvld+5zpnLuhqM7W+8ncGj8pqe2R\n\t6ZFXaZqc9ziZhY5oXLbSn90p1HL20af8ZRJLXDZQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"HN3R81Mo\"; dkim-atps=neutral","Date":"Fri, 23 Dec 2022 01:29:31 +0200","To":"libcamera-devel@lists.libcamera.org","Message-ID":"","References":"<20221222113122.28662-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221222113122.28662-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC/PATCH] libcamera: ipa_module: Relax\n\tipaModuleInfo symbol size check","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26226,"web_url":"https://patchwork.libcamera.org/comment/26226/","msgid":"<20230116084656.6xybeyta74nhq2fo@uno.localdomain>","date":"2023-01-16T08:46:56","subject":"Re: [libcamera-devel] [RFC/PATCH] libcamera: ipa_module: Relax\n\tipaModuleInfo symbol size check","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Thu, Dec 22, 2022 at 01:31:22PM +0200, Laurent Pinchart via libcamera-devel wrote:\n> When an IPA module is loaded, the loadIPAModuleInfo() function validates\n> the ipaModuleInfo structure. As part of that process, it checks that the\n> ipaModuleInfo symbol size matches the expected structure size. This\n> check breaks with clang and ASan, as the LLVM's address sanitizer\n> implementation includes the redzone after the structure in the symbol\n> size, currently growing it by 156 bytes (on x86-64). This causes all IPA\n> modules to fail to load.\n>\n> Fix the problem by relaxing the size check to only ensure that the\n> symbol is large enough to contain the structure.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> Relaxing checks increases the chance of false negatives, but I think\n> it's totally safe in this case. If we want to validate the structure\n> size, we should add a size field within the data. Other candidates for\n> new fields would be a magic number.\n\nI'm not sure it is worth the effort, even if we allow larger IPA\nmodules to be accepted, we only copy the relevant portion to the info_\nmodule, so it -should- theoretically be safe as it was before ?\n\nAs I don't have better proposals:\nReviewed-by: Jacopo Mondi \n\n> ---\n> src/libcamera/ipa_module.cpp | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index c9ff7de30e21..c152153c180f 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -288,12 +288,12 @@ int IPAModule::loadIPAModuleInfo()\n> \t}\n>\n> \tSpan info = elfLoadSymbol(data, \"ipaModuleInfo\");\n> -\tif (info.size() != sizeof(info_)) {\n> +\tif (info.size() < sizeof(info_)) {\n> \t\tLOG(IPAModule, Error) << \"IPA module has no valid info\";\n> \t\treturn -EINVAL;\n> \t}\n>\n> -\tmemcpy(&info_, info.data(), info.size());\n> +\tmemcpy(&info_, info.data(), sizeof(info_));\n>\n> \tif (info_.moduleAPIVersion != IPA_MODULE_API_VERSION) {\n> \t\tLOG(IPAModule, Error) << \"IPA module API version mismatch\";\n>\n> base-commit: f66a5c447b65bce774a1bc2d01034f437bf764b5\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CC31CC3240\n\tfor ;\n\tMon, 16 Jan 2023 08:47:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0ABE3625E4;\n\tMon, 16 Jan 2023 09:47:01 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 026EC625CC\n\tfor ;\n\tMon, 16 Jan 2023 09:46:59 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 57A64802;\n\tMon, 16 Jan 2023 09:46:59 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673858821;\n\tbh=krCC8AA23LSmrtsYhzxIiaY183m8GGJsX2q2/Sn2qIA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Dxvv0fN5fTM3GezBP2o8AXv65fo/tkNLU+p4U9XW07Er3F5ncGJU/IMY91oX93CWt\n\t4E8bGUfFcS4dTGF0zzRmuy7HDdBwaxtSnFQEQfevzy1mhg8qwvWJxnxpGV14/9OaH0\n\tmbmw2/zfCuOvd5a485nrfmkNGJIWa+YE9rNg2kiO7glZEVTT3VzYkmp0aNm7xrnZ4h\n\tV1QODSJmeQCi79qqP3RF9D1ATbvveXVyz8VYjDh/iQSBU6IaV7hEQeqEW8rlWkKvF9\n\th62Ycs9YogA3ycDAAZoTb8MqBcB7la7ii3ICeDlGDkxjTfC4ypUq4ioerlxf3EfjJj\n\tE9aFpCHm14qTw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1673858819;\n\tbh=krCC8AA23LSmrtsYhzxIiaY183m8GGJsX2q2/Sn2qIA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lqgk7Y9JnKFZa1EKcIFBUfkaICmz9nZnaV86xhUOPVKAD6vUXOvOMobrUcRSQcrOk\n\tV+BBGYNE0oLVt+ghWyNGNddO8J3mzZvQy2J8sqjE6RFM3/WEJ6+DAgk6N6cvfpgmwq\n\tGjtTp5oziJP7CXcqiT93jLE8jOBwrziqUvUf6F04="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lqgk7Y9J\"; dkim-atps=neutral","Date":"Mon, 16 Jan 2023 09:46:56 +0100","To":"Laurent Pinchart ","Message-ID":"<20230116084656.6xybeyta74nhq2fo@uno.localdomain>","References":"<20221222113122.28662-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221222113122.28662-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC/PATCH] libcamera: ipa_module: Relax\n\tipaModuleInfo symbol size check","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26230,"web_url":"https://patchwork.libcamera.org/comment/26230/","msgid":"<167387298748.42371.12917215165095716012@Monstersaurus>","date":"2023-01-16T12:43:07","subject":"Re: [libcamera-devel] [RFC/PATCH] libcamera: ipa_module: Relax\n\tipaModuleInfo symbol size check","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi via libcamera-devel (2023-01-16 08:46:56)\n> Hi Laurent\n> \n> On Thu, Dec 22, 2022 at 01:31:22PM +0200, Laurent Pinchart via libcamera-devel wrote:\n> > When an IPA module is loaded, the loadIPAModuleInfo() function validates\n> > the ipaModuleInfo structure. As part of that process, it checks that the\n> > ipaModuleInfo symbol size matches the expected structure size. This\n> > check breaks with clang and ASan, as the LLVM's address sanitizer\n> > implementation includes the redzone after the structure in the symbol\n> > size, currently growing it by 156 bytes (on x86-64). This causes all IPA\n> > modules to fail to load.\n> >\n> > Fix the problem by relaxing the size check to only ensure that the\n> > symbol is large enough to contain the structure.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Relaxing checks increases the chance of false negatives, but I think\n> > it's totally safe in this case. If we want to validate the structure\n> > size, we should add a size field within the data. Other candidates for\n> > new fields would be a magic number.\n> \n> I'm not sure it is worth the effort, even if we allow larger IPA\n> modules to be accepted, we only copy the relevant portion to the info_\n> module, so it -should- theoretically be safe as it was before ?\n> \n> As I don't have better proposals:\n> Reviewed-by: Jacopo Mondi \n\nFine with me too.\n\n\nReviewed-by: Kieran Bingham \n\n> \n> > ---\n> > src/libcamera/ipa_module.cpp | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> > index c9ff7de30e21..c152153c180f 100644\n> > --- a/src/libcamera/ipa_module.cpp\n> > +++ b/src/libcamera/ipa_module.cpp\n> > @@ -288,12 +288,12 @@ int IPAModule::loadIPAModuleInfo()\n> > }\n> >\n> > Span info = elfLoadSymbol(data, \"ipaModuleInfo\");\n> > - if (info.size() != sizeof(info_)) {\n> > + if (info.size() < sizeof(info_)) {\n> > LOG(IPAModule, Error) << \"IPA module has no valid info\";\n> > return -EINVAL;\n> > }\n> >\n> > - memcpy(&info_, info.data(), info.size());\n> > + memcpy(&info_, info.data(), sizeof(info_));\n> >\n> > if (info_.moduleAPIVersion != IPA_MODULE_API_VERSION) {\n> > LOG(IPAModule, Error) << \"IPA module API version mismatch\";\n> >\n> > base-commit: f66a5c447b65bce774a1bc2d01034f437bf764b5\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 880DDC3240\n\tfor ;\n\tMon, 16 Jan 2023 12:43:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01002625E4;\n\tMon, 16 Jan 2023 13:43:11 +0100 (CET)","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 A6A4F61F01\n\tfor ;\n\tMon, 16 Jan 2023 13:43:10 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1B2C1802;\n\tMon, 16 Jan 2023 13:43:10 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673872992;\n\tbh=9wM1NdQ2tFWBtDI7rb3Bync560t0EHnnehsWhwwpCGI=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=k12qrgIGNNeUFnGL71QexDymHaMSgxdaOmTnQ8Z3+ob8XK8K5ow4dmCGKduSf6xjt\n\t0AJMAeFfKwF95uobniaO4eUEx/lnhgpVK3CKeB4CRFkdjOtviQlSwdhji1fZu/YF+S\n\tbJaE7WxBEw1v0dIsdOd58R9GckayGWFDCpBd6mCL35DlJ3aBXEo6D7WcZbR+QW9oBJ\n\tep2n1GEZldHseJOXF8kDvNSH8bPTKBeX2tn3tfJKkqGFigW4WlX6l6RTbI/kguhc1r\n\tB0wBE19GoyQf6lqbwLhD5UaRjkWRXuJZoSVmtAXWCkSb7tdZY00wPc5P3aFf5o2ooG\n\tOuSPLiB58EIKg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1673872990;\n\tbh=9wM1NdQ2tFWBtDI7rb3Bync560t0EHnnehsWhwwpCGI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=TE3QkwGio5RNU5Go2ySbJxX96ArI7WchEr+FxGnQ1yX476mcBxy0MFR2AgapRxGMh\n\tL1GB+xGqRqbHaiVJjNwmHyfnqRn6No+Nv5cFBWHGnwMlshTvTmlGzXhNAKWnneMSra\n\tudaLj0eiZRppeNS0SV4JiRmXainwPX3sRLqBrejk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TE3QkwGi\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230116084656.6xybeyta74nhq2fo@uno.localdomain>","References":"<20221222113122.28662-1-laurent.pinchart@ideasonboard.com>\n\t<20230116084656.6xybeyta74nhq2fo@uno.localdomain>","To":"Jacopo Mondi ,\n\tJacopo Mondi via libcamera-devel , \n\tLaurent Pinchart ","Date":"Mon, 16 Jan 2023 12:43:07 +0000","Message-ID":"<167387298748.42371.12917215165095716012@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [RFC/PATCH] libcamera: ipa_module: Relax\n\tipaModuleInfo symbol size check","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]