| Message ID | 20200607143012.17752-3-email@uajain.com |
|---|---|
| State | Accepted |
| Commit | 79d666247182b0e42560ab505c29173ca94bdbb0 |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Sun, Jun 07, 2020 at 02:30:18PM +0000, Umang Jain wrote: > Given how the elfSection() is used the sub-expression s/is used/uses/ > (idx * eHdr->e_shentsize) > it 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 s/operand/operand / s/resultant/result/ > 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> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I'll fix the above when applying. Thanks for fixing this issue ! > --- > 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 60aaa34..72e357e 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, ElfW(Half) 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); > } >
Hi Laurent and Kieran On 6/7/20 9:05 PM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Sun, Jun 07, 2020 at 02:30:18PM +0000, Umang Jain wrote: >> Given how the elfSection() is used the sub-expression > s/is used/uses/ > >> (idx * eHdr->e_shentsize) >> it 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 > s/operand/operand / > s/resultant/result/ > >> 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> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I'll fix the above when applying. Thanks for fixing this issue ! Thanks for the suggestions, last-min fixups! and thorough reviews. :) >> --- >> 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 60aaa34..72e357e 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, ElfW(Half) 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); >> } >>
diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp index 60aaa34..72e357e 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, ElfW(Half) 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) it 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(-)