[libcamera-devel] [PATCH] libcamera: ipa_module: Fix implicit sign-extension in eflLoadSymbol
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jun 5 11:57:11 CEST 2020
Hi Umang,
On Fri, Jun 05, 2020 at 09:04:25AM +0000, Umang Jain wrote:
> On 6/5/20 1:31 PM, Kieran Bingham wrote:
> > On 05/06/2020 08:49, Umang Jain wrote:
> >> This sub-expression of two (16 bits, unsigned) operands
> >> (targetSymbol->st_shndx * eHdr->e_shentsize)
> >> is promoted to type int (32 bits, signed) for multiplication and then
> >> added to eHdr->e_shoff, which is of the type long (64 bits, unsigned).
> >> 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
> >
> > Ohhh triple-whammy! :-D
> >
> >> Signed-off-by: Umang Jain <email at uajain.com>
> >> ---
> >> src/libcamera/ipa_module.cpp | 6 ++++--
> >> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> >> index 91534b6..dd7538b 100644
> >> --- a/src/libcamera/ipa_module.cpp
> >> +++ b/src/libcamera/ipa_module.cpp
> >> @@ -102,7 +102,8 @@ Span<uint8_t> elfLoadSymbol(Span<uint8_t> elf, const char *symbol)
> >> if (!eHdr)
> >> return {};
> >>
> >> - off_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;
> >> + off_t offset = eHdr->e_shoff + ((uint64_t)eHdr->e_shentsize *
> >> + eHdr->e_shstrndx);
> >
> > Interesting, I had to solve a similar issue in Commit: 90240a79
> > ("libcamera: media_device: prevent sign extension on casts"), which hit
> > a real world issue causing breakage on the RPi when running a 32 bit
> > userspace with a 64 bit kernel.
> >
> > I fear hitting more of those issues too, So I'm pleased to see these
> > coverity warnings handled.
> >
> > In the patch I created, I ended up using uintptr_t as the cast, over the
> > __u64 types which were already being used.
> >
> > Assuming that __u64 is similar to uint64_t, is there any key difference
> > between uint64_t, and uintptr_t that means we should choose one over the
> > other?
>
> I am not sure. It seems to me that uintptr_t is a pointer cast that makes
> it platform-independent/portable.
>
> > At face value, they are both 64 bit values, though I guess actually
> > uintptr_t is 'a value which stores a pointer or an integer' so it could
> > be different in some cases.
>
> Yes, both seem to end up being 64-bit but main issue I see is that,
> uintptr_t seems to be used with casting of pointers, not actual
> data-type values. Here we have a struct member containing the value
> that needs to be explicitly casted.
>
> I am not sure if there is something similar to uintptr_t, but for values?
Not to my knowledge, but I don't think that would be needed. uintptr_t
is meant to cast pointers to integers, so it's not the right type here.
Given that eHdr->e_shentsize and eHdr->e_shstrndx and both uint16_t, and
eHdr->e_shoff is uint32_t on 32-bit platforms and uint64_t on 64-bit
platforms, I would case eHdr->e_shentsize to uint32_t.
While at it, would it make sense to extract the section header lookup to
a separate function, to ensure the same issue will not occur again in
the future ? Something like the following code maybe ?
diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
index 91534b61e2e4..1a6d097db47c 100644
--- a/src/libcamera/ipa_module.cpp
+++ b/src/libcamera/ipa_module.cpp
@@ -88,6 +88,15 @@ int elfVerifyIdent(Span<uint8_t> elf)
return 0;
}
+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;
+ return elfPointer<ElfW(Shdr)>(elf, offset);
+}
+
/**
* \brief Retrieve address and size of a symbol from an mmap'ed ELF file
* \param[in] elf Address and size of mmap'ed ELF file
@@ -102,8 +111,7 @@ Span<uint8_t> elfLoadSymbol(Span<uint8_t> elf, const char *symbol)
if (!eHdr)
return {};
- off_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;
- ElfW(Shdr) *sHdr = elfPointer<ElfW(Shdr)>(elf, offset);
+ ElfW(Shdr) *sHdr = elfSection(elf, eHdr, eHdr->e_shstrndx);
if (!sHdr)
return {};
off_t shnameoff = sHdr->sh_offset;
@@ -111,12 +119,11 @@ Span<uint8_t> elfLoadSymbol(Span<uint8_t> elf, const char *symbol)
/* Locate .dynsym section header. */
ElfW(Shdr) *dynsym = nullptr;
for (unsigned int i = 0; i < eHdr->e_shnum; i++) {
- offset = eHdr->e_shoff + eHdr->e_shentsize * i;
- sHdr = elfPointer<ElfW(Shdr)>(elf, offset);
+ sHdr = elfSection(elf, eHdr, i);
if (!sHdr)
return {};
- offset = shnameoff + sHdr->sh_name;
+ off_t offset = shnameoff + sHdr->sh_name;
char *name = elfPointer<char[8]>(elf, offset);
if (!name)
return {};
@@ -132,8 +139,7 @@ Span<uint8_t> elfLoadSymbol(Span<uint8_t> elf, const char *symbol)
return {};
}
- offset = eHdr->e_shoff + eHdr->e_shentsize * dynsym->sh_link;
- sHdr = elfPointer<ElfW(Shdr)>(elf, offset);
+ sHdr = elfSection(elf, eHdr, dynsym->sh_link);
if (!sHdr)
return {};
off_t dynsym_nameoff = sHdr->sh_offset;
@@ -142,7 +148,7 @@ Span<uint8_t> elfLoadSymbol(Span<uint8_t> elf, const char *symbol)
ElfW(Sym) *targetSymbol = nullptr;
unsigned int dynsym_num = dynsym->sh_size / dynsym->sh_entsize;
for (unsigned int i = 0; i < dynsym_num; i++) {
- offset = dynsym->sh_offset + dynsym->sh_entsize * i;
+ off_t offset = dynsym->sh_offset + dynsym->sh_entsize * i;
ElfW(Sym) *sym = elfPointer<ElfW(Sym)>(elf, offset);
if (!sym)
return {};
@@ -165,13 +171,10 @@ Span<uint8_t> elfLoadSymbol(Span<uint8_t> elf, const char *symbol)
}
/* Locate and return data of symbol. */
- if (targetSymbol->st_shndx >= eHdr->e_shnum)
- return {};
- offset = eHdr->e_shoff + targetSymbol->st_shndx * eHdr->e_shentsize;
- sHdr = elfPointer<ElfW(Shdr)>(elf, offset);
+ sHdr = elfSection(elf, eHdr, targetSymbol->st_shndx);
if (!sHdr)
return {};
- offset = sHdr->sh_offset + (targetSymbol->st_value - sHdr->sh_addr);
+ off_t offset = sHdr->sh_offset + (targetSymbol->st_value - sHdr->sh_addr);
uint8_t *data = elfPointer<uint8_t>(elf, offset, targetSymbol->st_size);
if (!data)
return {};
> > Anyway, beyond the particular choice of cast destination, of which
> > uint64_t is likely suitable in this case if it solves the issue, we
> > should use C++ casts:
> >
> > static_cast<uint64_t>(eHdr->e_shentsize) * ...
>
> Ah correct. Was reading too much C code on stackoverflow for this :P
>
> >> ElfW(Shdr) *sHdr = elfPointer<ElfW(Shdr)>(elf, offset);
> >> if (!sHdr)
> >> return {};
> >> @@ -167,7 +168,8 @@ Span<uint8_t> elfLoadSymbol(Span<uint8_t> elf, const char *symbol)
> >> /* Locate and return data of symbol. */
> >> if (targetSymbol->st_shndx >= eHdr->e_shnum)
> >> return {};
> >> - offset = eHdr->e_shoff + targetSymbol->st_shndx * eHdr->e_shentsize;
> >> + offset = eHdr->e_shoff + ((uint64_t)targetSymbol->st_shndx *
>>
> > C++ cast style here too
> >
> > With the casting corrected:
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> >> + eHdr->e_shentsize);
> >> sHdr = elfPointer<ElfW(Shdr)>(elf, offset);
> >> if (!sHdr)
> >> return {};
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list