[{"id":5030,"web_url":"https://patchwork.libcamera.org/comment/5030/","msgid":"","date":"2020-06-05T08:01:12","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_module: Fix implicit\n\tsign-extension in eflLoadSymbol","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 05/06/2020 08:49, Umang Jain wrote:\n> This sub-expression of two (16 bits, unsigned) operands\n> \t(targetSymbol->st_shndx * eHdr->e_shentsize)\n> is promoted to type int (32 bits, signed) for multiplication and then\n> added to eHdr->e_shoff, which is of the type long (64 bits, unsigned).\n> Since eHdr->e_shoff is unsigned, the integer conversion rules dictates\n> that the other signed operand(i.e. the resultant of aforementioned\n> sub-expression) will be converted to unsigned type too. This causes\n> sign-extension for both of the above operands to match eHdr->e_shoff's\n> type and should be avoided.\n> \n> The solution is to explicitly cast one of the operands of the\n> sub-expression with unsigned int type. Hence, the other operand will be\n> integer promoted and the resultant will also be of unsigned int type,\n> not requiring to bother about a sign-extension.\n> \n> Reported-by: Coverity CID=280008\n> Reported-by: Coverity CID=280009\n> Reported-by: Coverity CID=280010\n\nOhhh triple-whammy! :-D\n\n\n> Signed-off-by: Umang Jain \n> ---\n> src/libcamera/ipa_module.cpp | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index 91534b6..dd7538b 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -102,7 +102,8 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n> \tif (!eHdr)\n> \t\treturn {};\n> \n> -\toff_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;\n> +\toff_t offset = eHdr->e_shoff + ((uint64_t)eHdr->e_shentsize *\n> +\t\t\t\t\teHdr->e_shstrndx);\n\nInteresting, I had to solve a similar issue in Commit: 90240a79\n(\"libcamera: media_device: prevent sign extension on casts\"), which hit\na real world issue causing breakage on the RPi when running a 32 bit\nuserspace with a 64 bit kernel.\n\nI fear hitting more of those issues too, So I'm pleased to see these\ncoverity warnings handled.\n\nIn the patch I created, I ended up using uintptr_t as the cast, over the\n__u64 types which were already being used.\n\nAssuming that __u64 is similar to uint64_t, is there any key difference\nbetween uint64_t, and uintptr_t that means we should choose one over the\nother?\n\nAt face value, they are both 64 bit values, though I guess actually\nuintptr_t is 'a value which stores a pointer or an integer' so it could\nbe different in some cases.\n\nAnyway, beyond the particular choice of cast destination, of which\nuint64_t is likely suitable in this case if it solves the issue, we\nshould use C++ casts:\n\n static_cast(eHdr->e_shentsize) * ...\n\n\n\n\n> \tElfW(Shdr) *sHdr = elfPointer(elf, offset);\n> \tif (!sHdr)\n> \t\treturn {};\n> @@ -167,7 +168,8 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n> \t/* Locate and return data of symbol. */\n> \tif (targetSymbol->st_shndx >= eHdr->e_shnum)\n> \t\treturn {};\n> -\toffset = eHdr->e_shoff + targetSymbol->st_shndx * eHdr->e_shentsize;\n> +\toffset = eHdr->e_shoff + ((uint64_t)targetSymbol->st_shndx *\n\nC++ cast style here too\n\nWith the casting corrected:\n\nReviewed-by: Kieran Bingham \n\n> +\t\t\t\t eHdr->e_shentsize);\n> \tsHdr = elfPointer(elf, offset);\n> \tif (!sHdr)\n> \t\treturn {};\n>","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 76B0E603C7\n\tfor ;\n\tFri, 5 Jun 2020 10:01:16 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B1394253;\n\tFri, 5 Jun 2020 10:01:15 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tbT0xcWd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591344075;\n\tbh=l3w0hyWFQ8FJXict+KSUZT9T+6MpYyzVW8QxSH3XNeE=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=tbT0xcWdl/Kw2BCqi/kKN4H0+ltXp8jTMSexhnPHU0iGFWjLB9YpPEQUBWs+sCJrb\n\ts0TouqbzprkeZByhONPS34zdlD+Odebh4kKqZgr/jhfLOuXVO5txWf8CsgLjp+AsLG\n\t/vJOaSkKkbfRZSnMtOhiRAi+A4YQs7u6vCttm2KE=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Umang Jain , libcamera-devel@lists.libcamera.org","References":"<20200605074856.83927-1-email@uajain.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Fri, 5 Jun 2020 09:01:12 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.7.0","MIME-Version":"1.0","In-Reply-To":"<20200605074856.83927-1-email@uajain.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_module: Fix implicit\n\tsign-extension in eflLoadSymbol","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","X-List-Received-Date":"Fri, 05 Jun 2020 08:01:16 -0000"}},{"id":5038,"web_url":"https://patchwork.libcamera.org/comment/5038/","msgid":"<8e976476-e53b-b2b0-e41d-44f257fee9fa@uajain.com>","date":"2020-06-05T09:04:25","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_module: Fix implicit\n\tsign-extension in eflLoadSymbol","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Kieran,\n\nOn 6/5/20 1:31 PM, Kieran Bingham wrote:\n> Hi Umang,\n>\n> On 05/06/2020 08:49, Umang Jain wrote:\n>> This sub-expression of two (16 bits, unsigned) operands\n>> \t(targetSymbol->st_shndx * eHdr->e_shentsize)\n>> is promoted to type int (32 bits, signed) for multiplication and then\n>> added to eHdr->e_shoff, which is of the type long (64 bits, unsigned).\n>> Since eHdr->e_shoff is unsigned, the integer conversion rules dictates\n>> that the other signed operand(i.e. the resultant of aforementioned\n>> sub-expression) will be converted to unsigned type too. This causes\n>> sign-extension for both of the above operands to match eHdr->e_shoff's\n>> type and should be avoided.\n>>\n>> The solution is to explicitly cast one of the operands of the\n>> sub-expression with unsigned int type. Hence, the other operand will be\n>> integer promoted and the resultant will also be of unsigned int type,\n>> not requiring to bother about a sign-extension.\n>>\n>> Reported-by: Coverity CID=280008\n>> Reported-by: Coverity CID=280009\n>> Reported-by: Coverity CID=280010\n> Ohhh triple-whammy! :-D\n>\n>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/libcamera/ipa_module.cpp | 6 ++++--\n>> 1 file changed, 4 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n>> index 91534b6..dd7538b 100644\n>> --- a/src/libcamera/ipa_module.cpp\n>> +++ b/src/libcamera/ipa_module.cpp\n>> @@ -102,7 +102,8 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n>> \tif (!eHdr)\n>> \t\treturn {};\n>> \n>> -\toff_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;\n>> +\toff_t offset = eHdr->e_shoff + ((uint64_t)eHdr->e_shentsize *\n>> +\t\t\t\t\teHdr->e_shstrndx);\n> Interesting, I had to solve a similar issue in Commit: 90240a79\n> (\"libcamera: media_device: prevent sign extension on casts\"), which hit\n> a real world issue causing breakage on the RPi when running a 32 bit\n> userspace with a 64 bit kernel.\n>\n> I fear hitting more of those issues too, So I'm pleased to see these\n> coverity warnings handled.\n>\n> In the patch I created, I ended up using uintptr_t as the cast, over the\n> __u64 types which were already being used.\n>\n> Assuming that __u64 is similar to uint64_t, is there any key difference\n> between uint64_t, and uintptr_t that means we should choose one over the\n> other?\n\nI am not sure. It seems to me that uintptr_t is a pointer cast that makes\n\nit platform-independent/portable.\n\n> At face value, they are both 64 bit values, though I guess actually\n> uintptr_t is 'a value which stores a pointer or an integer' so it could\n> be different in some cases.\n\nYes, both seem to end up being 64-bit but main issue I see is that, \nuintptr_t\n\nseems to be used with casting of pointers, not actual data-type values. \nHere we have a struct\n\nmember containing the value that needs to be explicitly casted.\n\nI am not sure if there is something similar to uintptr_t, but for values?\n\n>\n> Anyway, beyond the particular choice of cast destination, of which\n> uint64_t is likely suitable in this case if it solves the issue, we\n> should use C++ casts:\n>\n> static_cast(eHdr->e_shentsize) * ...\nAh correct. Was reading too much C code on stackoverflow for this :P\n>\n>\n>\n>\n>> \tElfW(Shdr) *sHdr = elfPointer(elf, offset);\n>> \tif (!sHdr)\n>> \t\treturn {};\n>> @@ -167,7 +168,8 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n>> \t/* Locate and return data of symbol. */\n>> \tif (targetSymbol->st_shndx >= eHdr->e_shnum)\n>> \t\treturn {};\n>> -\toffset = eHdr->e_shoff + targetSymbol->st_shndx * eHdr->e_shentsize;\n>> +\toffset = eHdr->e_shoff + ((uint64_t)targetSymbol->st_shndx *\n> C++ cast style here too\n>\n> With the casting corrected:\n>\n> Reviewed-by: Kieran Bingham \n>\n>> +\t\t\t\t eHdr->e_shentsize);\n>> \tsHdr = elfPointer(elf, offset);\n>> \tif (!sHdr)\n>> \t\treturn {};\n>>","headers":{"Return-Path":"","Received":["from o1.f.az.sendgrid.net (o1.f.az.sendgrid.net [208.117.55.132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DC6EB61027\n\tfor ;\n\tFri, 5 Jun 2020 11:04:26 +0200 (CEST)","by filterdrecv-p3mdw1-6f5df8956d-n66lk with SMTP id\n\tfilterdrecv-p3mdw1-6f5df8956d-n66lk-17-5EDA0A99-28\n\t2020-06-05 09:04:25.472768432 +0000 UTC m=+132630.838595716","from mail.uajain.com (unknown)\n\tby ismtpd0003p1hnd1.sendgrid.net (SG) with ESMTP\n\tid hd5-HpjLRCSs8OiZWZ3FTQ Fri, 05 Jun 2020 09:04:25.178 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=uajain.com\n\theader.i=@uajain.com header.b=\"U3Nalali\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=uajain.com;\n\th=subject:references:from:mime-version:in-reply-to:to:content-type:\n\tcontent-transfer-encoding;\n\ts=s1; bh=mKljdj65KwprFZv+8XiUhEukQE/uZncfkc+bkQfrNy4=;\n\tb=U3NalaliPNjNvp1A/yv13Zg/AX+PTeOn9mgfaQvLH0UC24pTFYPew5Lz4HBwn22ec1UL\n\tpGrFzloZ9XBBemh9loMuUYtHXF16ILrEu2R/JlNxj/uRrsri3hSjhsrjEHhl3x28uukLzX\n\twwjzhz2s8Yq2qKb9KGhDAPpAHY8jos28I=","References":"<20200605074856.83927-1-email@uajain.com>\n\t","From":"Umang Jain ","Message-ID":"<8e976476-e53b-b2b0-e41d-44f257fee9fa@uajain.com>","Date":"Fri, 05 Jun 2020 09:04:25 +0000 (UTC)","Mime-Version":"1.0","In-Reply-To":"","X-SG-EID":"1Q40EQ7YGir8a9gjSIAdTjhngY657NMk9ckeo4dbHZDiOpywc/L3L9rFqlwE4KPcAl6MTtKrYqN2BGXS9ZDVMTpv8JfRm+uYdytKMNKVdQGA+7Jw0BbdCqRGyerqS/r/rPRaUSXZuNW0e0MOl4kWCdkEvA/4HoakhFeNP1n0gwJzHkUcaAeHC/UVrx0bO0sSVEUXAJKbsuJHqLwLz9+RoVxBwhEWVcBQN5K4y+SsLlfa5REyL11SHr+aSmX61G9Mm9YQ6hR66C4jzV5pB2dkZQ==","To":"kieran.bingham@ideasonboard.com, libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=us-ascii; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_module: Fix implicit\n\tsign-extension in eflLoadSymbol","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","X-List-Received-Date":"Fri, 05 Jun 2020 09:04:27 -0000"}},{"id":5039,"web_url":"https://patchwork.libcamera.org/comment/5039/","msgid":"<20200605095711.GD5852@pendragon.ideasonboard.com>","date":"2020-06-05T09:57:11","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_module: Fix implicit\n\tsign-extension in eflLoadSymbol","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Fri, Jun 05, 2020 at 09:04:25AM +0000, Umang Jain wrote:\n> On 6/5/20 1:31 PM, Kieran Bingham wrote:\n> > On 05/06/2020 08:49, Umang Jain wrote:\n> >> This sub-expression of two (16 bits, unsigned) operands\n> >> \t(targetSymbol->st_shndx * eHdr->e_shentsize)\n> >> is promoted to type int (32 bits, signed) for multiplication and then\n> >> added to eHdr->e_shoff, which is of the type long (64 bits, unsigned).\n> >> Since eHdr->e_shoff is unsigned, the integer conversion rules dictates\n> >> that the other signed operand(i.e. the resultant of aforementioned\n> >> sub-expression) will be converted to unsigned type too. This causes\n> >> sign-extension for both of the above operands to match eHdr->e_shoff's\n> >> type and should be avoided.\n> >>\n> >> The solution is to explicitly cast one of the operands of the\n> >> sub-expression with unsigned int type. Hence, the other operand will be\n> >> integer promoted and the resultant will also be of unsigned int type,\n> >> not requiring to bother about a sign-extension.\n> >>\n> >> Reported-by: Coverity CID=280008\n> >> Reported-by: Coverity CID=280009\n> >> Reported-by: Coverity CID=280010\n> >\n> > Ohhh triple-whammy! :-D\n> >\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/libcamera/ipa_module.cpp | 6 ++++--\n> >> 1 file changed, 4 insertions(+), 2 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> >> index 91534b6..dd7538b 100644\n> >> --- a/src/libcamera/ipa_module.cpp\n> >> +++ b/src/libcamera/ipa_module.cpp\n> >> @@ -102,7 +102,8 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n> >> \tif (!eHdr)\n> >> \t\treturn {};\n> >> \n> >> -\toff_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;\n> >> +\toff_t offset = eHdr->e_shoff + ((uint64_t)eHdr->e_shentsize *\n> >> +\t\t\t\t\teHdr->e_shstrndx);\n> >\n> > Interesting, I had to solve a similar issue in Commit: 90240a79\n> > (\"libcamera: media_device: prevent sign extension on casts\"), which hit\n> > a real world issue causing breakage on the RPi when running a 32 bit\n> > userspace with a 64 bit kernel.\n> >\n> > I fear hitting more of those issues too, So I'm pleased to see these\n> > coverity warnings handled.\n> >\n> > In the patch I created, I ended up using uintptr_t as the cast, over the\n> > __u64 types which were already being used.\n> >\n> > Assuming that __u64 is similar to uint64_t, is there any key difference\n> > between uint64_t, and uintptr_t that means we should choose one over the\n> > other?\n> \n> I am not sure. It seems to me that uintptr_t is a pointer cast that makes\n> it platform-independent/portable.\n> \n> > At face value, they are both 64 bit values, though I guess actually\n> > uintptr_t is 'a value which stores a pointer or an integer' so it could\n> > be different in some cases.\n> \n> Yes, both seem to end up being 64-bit but main issue I see is that, \n> uintptr_t seems to be used with casting of pointers, not actual\n> data-type values. Here we have a struct member containing the value\n> that needs to be explicitly casted.\n> \n> I am not sure if there is something similar to uintptr_t, but for values?\n\nNot to my knowledge, but I don't think that would be needed. uintptr_t\nis meant to cast pointers to integers, so it's not the right type here.\nGiven that eHdr->e_shentsize and eHdr->e_shstrndx and both uint16_t, and\neHdr->e_shoff is uint32_t on 32-bit platforms and uint64_t on 64-bit\nplatforms, I would case eHdr->e_shentsize to uint32_t.\n\nWhile at it, would it make sense to extract the section header lookup to\na separate function, to ensure the same issue will not occur again in\nthe future ? Something like the following code maybe ?\n\ndiff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\nindex 91534b61e2e4..1a6d097db47c 100644\n--- a/src/libcamera/ipa_module.cpp\n+++ b/src/libcamera/ipa_module.cpp\n@@ -88,6 +88,15 @@ int elfVerifyIdent(Span elf)\n \treturn 0;\n }\n\n+ElfW(Shdr) *elfSection(Span elf, ElfW(Ehdr) *eHdr, unsigned int idx)\n+{\n+\tif (idx >= eHdr->e_shnum)\n+\t\treturn nullptr;\n+\n+\toff_t offset = eHdr->e_shoff + idx * eHdr->e_shentsize;\n+\treturn elfPointer(elf, offset);\n+}\n+\n /**\n * \\brief Retrieve address and size of a symbol from an mmap'ed ELF file\n * \\param[in] elf Address and size of mmap'ed ELF file\n@@ -102,8 +111,7 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n \tif (!eHdr)\n \t\treturn {};\n\n-\toff_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;\n-\tElfW(Shdr) *sHdr = elfPointer(elf, offset);\n+\tElfW(Shdr) *sHdr = elfSection(elf, eHdr, eHdr->e_shstrndx);\n \tif (!sHdr)\n \t\treturn {};\n \toff_t shnameoff = sHdr->sh_offset;\n@@ -111,12 +119,11 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n \t/* Locate .dynsym section header. */\n \tElfW(Shdr) *dynsym = nullptr;\n \tfor (unsigned int i = 0; i < eHdr->e_shnum; i++) {\n-\t\toffset = eHdr->e_shoff + eHdr->e_shentsize * i;\n-\t\tsHdr = elfPointer(elf, offset);\n+\t\tsHdr = elfSection(elf, eHdr, i);\n \t\tif (!sHdr)\n \t\t\treturn {};\n\n-\t\toffset = shnameoff + sHdr->sh_name;\n+\t\toff_t offset = shnameoff + sHdr->sh_name;\n \t\tchar *name = elfPointer(elf, offset);\n \t\tif (!name)\n \t\t\treturn {};\n@@ -132,8 +139,7 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n \t\treturn {};\n \t}\n\n-\toffset = eHdr->e_shoff + eHdr->e_shentsize * dynsym->sh_link;\n-\tsHdr = elfPointer(elf, offset);\n+\tsHdr = elfSection(elf, eHdr, dynsym->sh_link);\n \tif (!sHdr)\n \t\treturn {};\n \toff_t dynsym_nameoff = sHdr->sh_offset;\n@@ -142,7 +148,7 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n \tElfW(Sym) *targetSymbol = nullptr;\n \tunsigned int dynsym_num = dynsym->sh_size / dynsym->sh_entsize;\n \tfor (unsigned int i = 0; i < dynsym_num; i++) {\n-\t\toffset = dynsym->sh_offset + dynsym->sh_entsize * i;\n+\t\toff_t offset = dynsym->sh_offset + dynsym->sh_entsize * i;\n \t\tElfW(Sym) *sym = elfPointer(elf, offset);\n \t\tif (!sym)\n \t\t\treturn {};\n@@ -165,13 +171,10 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n \t}\n\n \t/* Locate and return data of symbol. */\n-\tif (targetSymbol->st_shndx >= eHdr->e_shnum)\n-\t\treturn {};\n-\toffset = eHdr->e_shoff + targetSymbol->st_shndx * eHdr->e_shentsize;\n-\tsHdr = elfPointer(elf, offset);\n+\tsHdr = elfSection(elf, eHdr, targetSymbol->st_shndx);\n \tif (!sHdr)\n \t\treturn {};\n-\toffset = sHdr->sh_offset + (targetSymbol->st_value - sHdr->sh_addr);\n+\toff_t offset = sHdr->sh_offset + (targetSymbol->st_value - sHdr->sh_addr);\n \tuint8_t *data = elfPointer(elf, offset, targetSymbol->st_size);\n \tif (!data)\n \t\treturn {};\n\n\n> > Anyway, beyond the particular choice of cast destination, of which\n> > uint64_t is likely suitable in this case if it solves the issue, we\n> > should use C++ casts:\n> >\n> > static_cast(eHdr->e_shentsize) * ...\n>\n> Ah correct. Was reading too much C code on stackoverflow for this :P\n>\n> >> \tElfW(Shdr) *sHdr = elfPointer(elf, offset);\n> >> \tif (!sHdr)\n> >> \t\treturn {};\n> >> @@ -167,7 +168,8 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n> >> \t/* Locate and return data of symbol. */\n> >> \tif (targetSymbol->st_shndx >= eHdr->e_shnum)\n> >> \t\treturn {};\n> >> -\toffset = eHdr->e_shoff + targetSymbol->st_shndx * eHdr->e_shentsize;\n> >> +\toffset = eHdr->e_shoff + ((uint64_t)targetSymbol->st_shndx *\n>>\n> > C++ cast style here too\n> >\n> > With the casting corrected:\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >> +\t\t\t\t eHdr->e_shentsize);\n> >> \tsHdr = elfPointer(elf, offset);\n> >> \tif (!sHdr)\n> >> \t\treturn {};","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 604A361027\n\tfor ;\n\tFri, 5 Jun 2020 11:57:29 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CB12127C;\n\tFri, 5 Jun 2020 11:57:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"R1ALXOtm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591351049;\n\tbh=RtZ7AqAOMI4xw0EBjeUTEg3Y62Bo6nTRinGDwR6Boug=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R1ALXOtmv628eW0gDAhQO2lRNIoxl6V+XOBCQsALZZgDRAImq2PmcE9gdCuH6kso6\n\tB3hrYPMsaUNMrjsP6KyUtjnpW9NN2sOr3E8LDV6LxgMzCmqTgsnu8KBZ/S0GMidjdJ\n\tbG/8xc8t7rYtrOSOV4khnu8ZsVqBNJ6d2Su3FM8M=","Date":"Fri, 5 Jun 2020 12:57:11 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"kieran.bingham@ideasonboard.com, libcamera-devel@lists.libcamera.org","Message-ID":"<20200605095711.GD5852@pendragon.ideasonboard.com>","References":"<20200605074856.83927-1-email@uajain.com>\n\t\n\t<8e976476-e53b-b2b0-e41d-44f257fee9fa@uajain.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<8e976476-e53b-b2b0-e41d-44f257fee9fa@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_module: Fix implicit\n\tsign-extension in eflLoadSymbol","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","X-List-Received-Date":"Fri, 05 Jun 2020 09:57:29 -0000"}}]