| Message ID | 20200605150858.116564-3-email@uajain.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Fri, Jun 05, 2020 at 03:09:16PM +0000, Umang Jain wrote: > Given how the elfSection() is used, the sub-expression > (idx * eHdr->e_shentsize) > has effectively two (16 bits, unsigned) operands. > The sub-expression is promoted to type int (32 bits, signed) for > multiplication and then added to eHdr->e_shoff, which is uint32_t > on 32-bit platforms and uint64_t on 64-bit platforms. Since eHdr->e_shoff > is unsigned, the integer conversion rules dictates that the other signed > operand(i.e. the resultant of aforementioned sub-expression) will be > converted to unsigned type too. This causes sign-extension for both of > the above operands to match eHdr->e_shoff's type and should be avoided. > > The solution is to explicitly cast one of the operands of the > sub-expression with unsigned int type. Hence, the other operand will be > integer promoted and the resultant will also be of unsigned int type, > not requiring to bother about a sign-extension. > > Reported-by: Coverity CID=280008 > Reported-by: Coverity CID=280009 > Reported-by: Coverity CID=280010 > Signed-off-by: Umang Jain <email@uajain.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/ipa_module.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp > index f54dd8b..d79151d 100644 > --- a/src/libcamera/ipa_module.cpp > +++ b/src/libcamera/ipa_module.cpp > @@ -93,7 +93,8 @@ ElfW(Shdr) *elfSection(Span<uint8_t> elf, ElfW(Ehdr) *eHdr, unsigned int idx) > if (idx >= eHdr->e_shnum) > return nullptr; > > - off_t offset = eHdr->e_shoff + idx * eHdr->e_shentsize; > + off_t offset = eHdr->e_shoff + idx * > + static_cast<uint32_t>(eHdr->e_shentsize); Is this needed now that idx is an unsigned int and not an uint16_t anymore ? > return elfPointer<ElfW(Shdr)>(elf, offset); > }
Hi Laurent, On 6/5/20 10:11 PM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Fri, Jun 05, 2020 at 03:09:16PM +0000, Umang Jain wrote: >> Given how the elfSection() is used, the sub-expression >> (idx * eHdr->e_shentsize) >> has effectively two (16 bits, unsigned) operands. >> The sub-expression is promoted to type int (32 bits, signed) for >> multiplication and then added to eHdr->e_shoff, which is uint32_t >> on 32-bit platforms and uint64_t on 64-bit platforms. Since eHdr->e_shoff >> is unsigned, the integer conversion rules dictates that the other signed >> operand(i.e. the resultant of aforementioned sub-expression) will be >> converted to unsigned type too. This causes sign-extension for both of >> the above operands to match eHdr->e_shoff's type and should be avoided. >> >> The solution is to explicitly cast one of the operands of the >> sub-expression with unsigned int type. Hence, the other operand will be >> integer promoted and the resultant will also be of unsigned int type, >> not requiring to bother about a sign-extension. >> >> Reported-by: Coverity CID=280008 >> Reported-by: Coverity CID=280009 >> Reported-by: Coverity CID=280010 >> Signed-off-by: Umang Jain <email@uajain.com> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/ipa_module.cpp | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp >> index f54dd8b..d79151d 100644 >> --- a/src/libcamera/ipa_module.cpp >> +++ b/src/libcamera/ipa_module.cpp >> @@ -93,7 +93,8 @@ ElfW(Shdr) *elfSection(Span<uint8_t> elf, ElfW(Ehdr) *eHdr, unsigned int idx) >> if (idx >= eHdr->e_shnum) >> return nullptr; >> >> - off_t offset = eHdr->e_shoff + idx * eHdr->e_shentsize; >> + off_t offset = eHdr->e_shoff + idx * >> + static_cast<uint32_t>(eHdr->e_shentsize); > Is this needed now that idx is an unsigned int and not an uint16_t > anymore ? Should I assume `unsigned int` will be 32-bit minimum? If yes, then you are right. Well, I have not heard "16-bit platforms" in recent years but I am not sure if there is any obscurity out there in wild. > >> return elfPointer<ElfW(Shdr)>(elf, offset); >> }
Hi Umang, On Sat, Jun 06, 2020 at 04:32:28PM +0000, Umang Jain wrote: > On 6/5/20 10:11 PM, Laurent Pinchart wrote: > > On Fri, Jun 05, 2020 at 03:09:16PM +0000, Umang Jain wrote: > >> Given how the elfSection() is used, the sub-expression > >> (idx * eHdr->e_shentsize) > >> has effectively two (16 bits, unsigned) operands. > >> The sub-expression is promoted to type int (32 bits, signed) for > >> multiplication and then added to eHdr->e_shoff, which is uint32_t > >> on 32-bit platforms and uint64_t on 64-bit platforms. Since eHdr->e_shoff > >> is unsigned, the integer conversion rules dictates that the other signed > >> operand(i.e. the resultant of aforementioned sub-expression) will be > >> converted to unsigned type too. This causes sign-extension for both of > >> the above operands to match eHdr->e_shoff's type and should be avoided. > >> > >> The solution is to explicitly cast one of the operands of the > >> sub-expression with unsigned int type. Hence, the other operand will be > >> integer promoted and the resultant will also be of unsigned int type, > >> not requiring to bother about a sign-extension. > >> > >> Reported-by: Coverity CID=280008 > >> Reported-by: Coverity CID=280009 > >> Reported-by: Coverity CID=280010 > >> Signed-off-by: Umang Jain <email@uajain.com> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> src/libcamera/ipa_module.cpp | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp > >> index f54dd8b..d79151d 100644 > >> --- a/src/libcamera/ipa_module.cpp > >> +++ b/src/libcamera/ipa_module.cpp > >> @@ -93,7 +93,8 @@ ElfW(Shdr) *elfSection(Span<uint8_t> elf, ElfW(Ehdr) *eHdr, unsigned int idx) > >> if (idx >= eHdr->e_shnum) > >> return nullptr; > >> > >> - off_t offset = eHdr->e_shoff + idx * eHdr->e_shentsize; > >> + off_t offset = eHdr->e_shoff + idx * > >> + static_cast<uint32_t>(eHdr->e_shentsize); > > Is this needed now that idx is an unsigned int and not an uint16_t > > anymore ? > > Should I assume `unsigned int` will be 32-bit minimum? If yes, then you > are right. > Well, I have not heard "16-bit platforms" in recent years but I am not > sure if there is any obscurity out there in wild. I think we can guarantee that all platforms we support will be 32-bit or more, but if you want to be on the safe side, you can make idx a uin32_t. Or make it a ElfW(Half) in patch 1/2 (as it can't be larger than that), and keep this patch to cast it to uint32_t. That make be cleaner. > >> return elfPointer<ElfW(Shdr)>(elf, offset); > >> }
diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp index f54dd8b..d79151d 100644 --- a/src/libcamera/ipa_module.cpp +++ b/src/libcamera/ipa_module.cpp @@ -93,7 +93,8 @@ ElfW(Shdr) *elfSection(Span<uint8_t> elf, ElfW(Ehdr) *eHdr, unsigned int idx) if (idx >= eHdr->e_shnum) return nullptr; - off_t offset = eHdr->e_shoff + idx * eHdr->e_shentsize; + off_t offset = eHdr->e_shoff + idx * + static_cast<uint32_t>(eHdr->e_shentsize); return elfPointer<ElfW(Shdr)>(elf, offset); }
Given how the elfSection() is used, the sub-expression (idx * eHdr->e_shentsize) has effectively two (16 bits, unsigned) operands. The sub-expression is promoted to type int (32 bits, signed) for multiplication and then added to eHdr->e_shoff, which is uint32_t on 32-bit platforms and uint64_t on 64-bit platforms. Since eHdr->e_shoff is unsigned, the integer conversion rules dictates that the other signed operand(i.e. the resultant of aforementioned sub-expression) will be converted to unsigned type too. This causes sign-extension for both of the above operands to match eHdr->e_shoff's type and should be avoided. The solution is to explicitly cast one of the operands of the sub-expression with unsigned int type. Hence, the other operand will be integer promoted and the resultant will also be of unsigned int type, not requiring to bother about a sign-extension. Reported-by: Coverity CID=280008 Reported-by: Coverity CID=280009 Reported-by: Coverity CID=280010 Signed-off-by: Umang Jain <email@uajain.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/ipa_module.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)