Change in ...coreboot[master]: util/cbfstool: Use 64 bit integers in multiplications

[Jacob Garber (Code Review)]

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33986

Change subject: util/cbfstool: Use 64 bit integers in multiplications
......................................................................

util/cbfstool: Use 64 bit integers in multiplications

The operands in these multiplications are 16 bit integers, which are
implicitly converted to signed int's before doing the multiplication.
To prevent possible overflow and other sign troubles, cast them to the
appropriate 64 bit types they are stored in before multiplying.

Change-Id: I5391221d46d620d0e5bd629e2f9680be7a53342e
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 12297{03,04,05,06,07,08,09,10}
---
M util/cbfstool/elfheaders.c
1 file changed, 6 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/33986/1

diff --git a/util/cbfstool/elfheaders.c b/util/cbfstool/elfheaders.c
index 8da54d0..1c43fdb 100644
--- a/util/cbfstool/elfheaders.c
+++ b/util/cbfstool/elfheaders.c
@@ -262,7 +262,7 @@
     * per the ELF spec, You'd be surprised how many ELF
     * readers miss this little detail.
     */
-	buffer_splice(&b, in, ehdr->e_phoff, ehdr->e_phentsize * ehdr->e_phnum);
+	buffer_splice(&b, in, ehdr->e_phoff, (size_t)ehdr->e_phentsize * ehdr->e_phnum);
    if (check_size(in, ehdr->e_phoff, buffer_size(&b), "program headers"))
    	return -1;

@@ -304,7 +304,7 @@
     * per the ELF spec, You'd be surprised how many ELF
     * readers miss this little detail.
     */
-	buffer_splice(&b, in, ehdr->e_shoff, ehdr->e_shentsize * ehdr->e_shnum);
+	buffer_splice(&b, in, ehdr->e_shoff, (size_t)ehdr->e_shentsize * ehdr->e_shnum);
    if (check_size(in, ehdr->e_shoff, buffer_size(&b), "section headers"))
    	return -1;

@@ -1180,8 +1180,8 @@
    ew->ehdr.e_shnum = ew->num_secs;
    metadata_size = 0;
    metadata_size += ew->ehdr.e_ehsize;
-	metadata_size += ew->ehdr.e_shnum * ew->ehdr.e_shentsize;
-	metadata_size += ew->ehdr.e_phnum * ew->ehdr.e_phentsize;
+	metadata_size += (Elf64_Xword)ew->ehdr.e_shnum * ew->ehdr.e_shentsize;
+	metadata_size += (Elf64_Xword)ew->ehdr.e_phnum * ew->ehdr.e_phentsize;
    shstroffset = metadata_size;
    /* Align up section header string size and metadata size to 4KiB */
    metadata_size = ALIGN(metadata_size + shstrlen, 4096);
@@ -1200,11 +1200,11 @@
     */
    ew->ehdr.e_shoff = ew->ehdr.e_ehsize;
    ew->ehdr.e_phoff = ew->ehdr.e_shoff +
-	                   ew->ehdr.e_shnum * ew->ehdr.e_shentsize;
+			   (Elf64_Off)ew->ehdr.e_shnum * ew->ehdr.e_shentsize;

buffer_splice(&metadata, out, 0, metadata_size);
    buffer_splice(&phdrs, out, ew->ehdr.e_phoff,
-	              ew->ehdr.e_phnum * ew->ehdr.e_phentsize);
+		      (size_t)ew->ehdr.e_phnum * ew->ehdr.e_phentsize);
    buffer_splice(&data, out, metadata_size, program_size);
    /* Set up the section header string table contents. */
    strtab = &ew->shstrtab_sec->content;

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33986 )

Change subject: util/cbfstool: Use 64 bit integers in multiplications
......................................................................

Patch Set 1:

(1 comment)

I'm somewhat confused how multiplying two u16 into an s32 could cause overflow or sign issues? This seems unnecessary. (Also, if you do want to do this, note that size_t is dependent on the host architecture which is not guaranteed to be x86_64. You should use an explicit-width type like Elf64_Off or uint64_t.)

https://review.coreboot.org/#/c/33986/1/util/cbfstool/elfheaders.c 
File util/cbfstool/elfheaders.c:

https://review.coreboot.org/#/c/33986/1/util/cbfstool/elfheaders.c@265 
PS1, Line 265: 	buffer_splice(&b, in, ehdr->e_phoff, (size_t)ehdr->e_phentsize * ehdr->e_phnum);
Stick to 80 char limit, please (at least until the whole file is converted to 96)

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33986 )

Change subject: util/cbfstool: Use 64 bit integers in multiplications
......................................................................

Patch Set 1:
> Patch Set 1:
>
> (1 comment)
>
> I'm somewhat confused how multiplying two u16 into an s32 could cause overflow or sign issues? This seems unnecessary. (Also, if you do want to do this, note that size_t is dependent on the host architecture which is not guaranteed to be x86_64. You should use an explicit-width type like Elf64_Off or uint64_t.)

It is possible for the two u16 to overflow an s32, which will lead to a negative number (which is already UB), and then converting to a u64 will sign-extend it to an extremely large integer. Eg.

int32_t x = -1;
    uint64_t y = x; // x is sign-extended to UINT64_MAX

For the size_t issue I suppose 64 bits itself doesn't matter as long as it is at least a u32, which can always hold a u16 * u16. I think this should be true on all platforms we support. I'll clarify this in the commit message.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33986 )

Change subject: util/cbfstool: Use 64 bit integers in multiplications
......................................................................

Patch Set 1:
> It is possible for the two u16 to overflow an s32, which will lead to a negative number (which is already UB), and then converting to a u64 will sign-extend it to an extremely large integer. Eg.

Right, sorry, wasn't thinking.
> For the size_t issue I suppose 64 bits itself doesn't matter as long as it is at least a u32, which can always hold a u16 * u16. I think this should be true on all platforms we support. I'll clarify this in the commit message.

I still think a fixed-width type would be more appropriate there to clarify your intention, even though size_t should work in practice.

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33986 )

Change subject: util/cbfstool: Use 64 bit integers in multiplications
......................................................................

Patch Set 1:
> > For the size_t issue I suppose 64 bits itself doesn't matter as long as it is at least a u32, which can always hold a u16 * u16. I think this should be true on all platforms we support. I'll clarify this in the commit message.
>
> I still think a fixed-width type would be more appropriate there to clarify your intention, even though size_t should work in practice.

Right, I'll use a u32 there instead.

Change in ...coreboot[master]: util/cbfstool: Prevent overflow of 16 bit multiplications

[Jacob Garber (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33986

to look at the new patch set (#2).

Change subject: util/cbfstool: Prevent overflow of 16 bit multiplications
......................................................................

util/cbfstool: Prevent overflow of 16 bit multiplications

Considering the following integer multiplication:

u64 = u16 * u16

What on earth, one might wonder, is the problem with this? Well, due to
C's unfortunately abstruse integer semantics, both u16's are implicitly
converted to int before the multiplication, which cannot hold
all possible values of a u16 * u16. Even worse, after overflow the
intermediate result will be a negative number, which during the
conversion to a u64 will be sign-extended to a huge integer. Not good.

The solution is to manually cast one of the u16 to a u32 or u64, which
are large enough to not have any overflow and will prevent the implicit
conversion. The type of the u64 is preferred, though a u32 is used
instead of size_t, since that can change depending on the platform.

Change-Id: I5391221d46d620d0e5bd629e2f9680be7a53342e
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 12297{03,04,05,06,07,08,09,10}
---
M util/cbfstool/elfheaders.c
1 file changed, 8 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/33986/2

Change in ...coreboot[master]: util/cbfstool: Prevent overflow of 16 bit multiplications

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33986 )

Change subject: util/cbfstool: Prevent overflow of 16 bit multiplications
......................................................................

Patch Set 2: Code-Review+2

Change in ...coreboot[master]: util/cbfstool: Prevent overflow of 16 bit multiplications

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33986 )

Change subject: util/cbfstool: Prevent overflow of 16 bit multiplications
......................................................................

Patch Set 2: Code-Review+1

Change in coreboot[master]: util/cbfstool: Prevent overflow of 16 bit multiplications

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33986 )

Change subject: util/cbfstool: Prevent overflow of 16 bit multiplications
......................................................................

util/cbfstool: Prevent overflow of 16 bit multiplications

Considering the following integer multiplication:

u64 = u16 * u16

What on earth, one might wonder, is the problem with this? Well, due to
C's unfortunately abstruse integer semantics, both u16's are implicitly
converted to int before the multiplication, which cannot hold
all possible values of a u16 * u16. Even worse, after overflow the
intermediate result will be a negative number, which during the
conversion to a u64 will be sign-extended to a huge integer. Not good.

The solution is to manually cast one of the u16 to a u32 or u64, which
are large enough to not have any overflow and will prevent the implicit
conversion. The type of the u64 is preferred, though a u32 is used
instead of size_t, since that can change depending on the platform.

Change-Id: I5391221d46d620d0e5bd629e2f9680be7a53342e
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 12297{03,04,05,06,07,08,09,10}
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33986
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
Reviewed-by: Paul Menzel 
---
M util/cbfstool/elfheaders.c
1 file changed, 8 insertions(+), 6 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Julius Werner: Looks good to me, approved

diff --git a/util/cbfstool/elfheaders.c b/util/cbfstool/elfheaders.c
index 8da54d0..676a635 100644
--- a/util/cbfstool/elfheaders.c
+++ b/util/cbfstool/elfheaders.c
@@ -262,7 +262,8 @@
     * per the ELF spec, You'd be surprised how many ELF
     * readers miss this little detail.
     */
-	buffer_splice(&b, in, ehdr->e_phoff, ehdr->e_phentsize * ehdr->e_phnum);
+	buffer_splice(&b, in, ehdr->e_phoff,
+		      (uint32_t)ehdr->e_phentsize * ehdr->e_phnum);
    if (check_size(in, ehdr->e_phoff, buffer_size(&b), "program headers"))
    	return -1;

@@ -304,7 +305,8 @@
     * per the ELF spec, You'd be surprised how many ELF
     * readers miss this little detail.
     */
-	buffer_splice(&b, in, ehdr->e_shoff, ehdr->e_shentsize * ehdr->e_shnum);
+	buffer_splice(&b, in, ehdr->e_shoff,
+		      (uint32_t)ehdr->e_shentsize * ehdr->e_shnum);
    if (check_size(in, ehdr->e_shoff, buffer_size(&b), "section headers"))
    	return -1;

@@ -1180,8 +1182,8 @@
    ew->ehdr.e_shnum = ew->num_secs;
    metadata_size = 0;
    metadata_size += ew->ehdr.e_ehsize;
-	metadata_size += ew->ehdr.e_shnum * ew->ehdr.e_shentsize;
-	metadata_size += ew->ehdr.e_phnum * ew->ehdr.e_phentsize;
+	metadata_size += (Elf64_Xword)ew->ehdr.e_shnum * ew->ehdr.e_shentsize;
+	metadata_size += (Elf64_Xword)ew->ehdr.e_phnum * ew->ehdr.e_phentsize;
    shstroffset = metadata_size;
    /* Align up section header string size and metadata size to 4KiB */
    metadata_size = ALIGN(metadata_size + shstrlen, 4096);
@@ -1200,11 +1202,11 @@
     */
    ew->ehdr.e_shoff = ew->ehdr.e_ehsize;
    ew->ehdr.e_phoff = ew->ehdr.e_shoff +
-	                   ew->ehdr.e_shnum * ew->ehdr.e_shentsize;
+			   (Elf64_Off)ew->ehdr.e_shnum * ew->ehdr.e_shentsize;

buffer_splice(&metadata, out, 0, metadata_size);
    buffer_splice(&phdrs, out, ew->ehdr.e_phoff,
-	              ew->ehdr.e_phnum * ew->ehdr.e_phentsize);
+		      (uint32_t)ew->ehdr.e_phnum * ew->ehdr.e_phentsize);
    buffer_splice(&data, out, metadata_size, program_size);
    /* Set up the section header string table contents. */
    strtab = &ew->shstrtab_sec->content;