Change in ...coreboot[master]: libpayload: cbfs: Check decompressed size when loading files.

[You-Cheng Syu (Code Review)]

You-Cheng Syu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31564

Change subject: libpayload: cbfs: Check decompressed size when loading files.
......................................................................

libpayload: cbfs: Check decompressed size when loading files.

After loading compressed files in CBFS, we should check the decompressed
size is equal to the expected size. This might help us detect file
content corruption or compressor/decompressor bugs.

BUG=none
BRANCH=none
TEST=boot into kernel on kukui

Change-Id: Ia756cc5477670dd0d1d8aa59d4160ab4233c6795
Signed-off-by: You-Cheng Syu 
---
M payloads/libpayload/libcbfs/cbfs_core.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/31564/1

diff --git a/payloads/libpayload/libcbfs/cbfs_core.c b/payloads/libpayload/libcbfs/cbfs_core.c
index 91f2603..e234214 100644
--- a/payloads/libpayload/libcbfs/cbfs_core.c
+++ b/payloads/libpayload/libcbfs/cbfs_core.c
@@ -256,7 +256,7 @@
    	return NULL;

ret = malloc(*size);
-	if (ret != NULL && !cbfs_decompress(algo, data, ret, *size)) {
+	if (ret != NULL && cbfs_decompress(algo, data, ret, *size) != *size) {
    	free(ret);
    	ret = NULL;
    }

[Paul Menzel (Code Review)]

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

Change subject: libpayload: cbfs: Check decompressed size when loading files.
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31564/1/payloads/libpayload/libcbfs/cbfs_cor... 
File payloads/libpayload/libcbfs/cbfs_core.c:

https://review.coreboot.org/#/c/31564/1/payloads/libpayload/libcbfs/cbfs_cor... 
PS1, Line 259: 	if (ret != NULL && cbfs_decompress(algo, data, ret, *size) != *size) {
I’d prefer a separate check, with a separate error message.

[Daisuke Nojiri (Code Review)]

Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31564 )

Change subject: libpayload: cbfs: Check decompressed size when loading files.
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31564/1/payloads/libpayload/libcbfs/cbfs_cor... 
File payloads/libpayload/libcbfs/cbfs_core.c:

https://review.coreboot.org/#/c/31564/1/payloads/libpayload/libcbfs/cbfs_cor... 
PS1, Line 259: cbfs_decompress
What is more scary is if the size doesn't match, we can overflow the dest buffer because ...

unsigned long ulzma(const unsigned char *src, unsigned char *dst)
{
    return ulzman(src, (unsigned long)(-1), dst, (unsigned long)(-1));
}

[Julius Werner (Code Review)]

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

Change subject: libpayload: cbfs: Check decompressed size when loading files.
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/31564/1/payloads/libpayload/libcbfs/cbfs_cor... 
File payloads/libpayload/libcbfs/cbfs_core.c:

https://review.coreboot.org/#/c/31564/1/payloads/libpayload/libcbfs/cbfs_cor... 
PS1, Line 259: cbfs_decompress
> What is more scary is if the size doesn't match, we can overflow the dest buffer because ... […]

We should probably just change cbfs_decompress to use ulzman() and ulz4n() instead? (Would need to add another parameter for the source size, then.)

https://review.coreboot.org/#/c/31564/1/payloads/libpayload/libcbfs/cbfs_cor... 
PS1, Line 259: 	if (ret != NULL && cbfs_decompress(algo, data, ret, *size) != *size) {
> I’d prefer a separate check, with a separate error message.

I agree an error message would be nice, although one should probably be enough (e.g. "can't decompress ...").

[Paul Menzel (Code Review)]

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

Change subject: libpayload: cbfs: Check decompressed size when loading files.
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/31564/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31564/1//COMMIT_MSG@7 
PS1, Line 7: libpayload: cbfs: Check decompressed size when loading files.
Also, please remove the dot at the end of the commit message summary.

https://review.coreboot.org/#/c/31564/1//COMMIT_MSG@15 
PS1, Line 15: TEST=boot into kernel on kukui
But that worked before, didn’t it? Or do you have a test case, where there is a compressor/decompressor bug?

Change in ...coreboot[master]: libpayload: cbfs: Check decompressed size when loading files

[You-Cheng Syu (Code Review)]

Hello Julius Werner, Daisuke Nojiri, Hung-Te Lin, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: libpayload: cbfs: Check decompressed size when loading files
......................................................................

libpayload: cbfs: Check decompressed size when loading files

After loading compressed files in CBFS, we should check the decompressed
size is equal to the expected size. This might help us detect file
content corruption or compressor/decompressor bugs.

BUG=none
BRANCH=none
TEST=boot into kernel on Kukui

Change-Id: Ia756cc5477670dd0d1d8aa59d4160ab4233c6795
Signed-off-by: You-Cheng Syu 
---
M payloads/libpayload/libcbfs/cbfs_core.c
1 file changed, 11 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/31564/2

Change in ...coreboot[master]: libpayload: cbfs: Check decompressed size when loading files

[You-Cheng Syu (Code Review)]

You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31564 )

Change subject: libpayload: cbfs: Check decompressed size when loading files
......................................................................

Patch Set 2:

(4 comments)

https://review.coreboot.org/#/c/31564/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31564/1//COMMIT_MSG@7 
PS1, Line 7: libpayload: cbfs: Check decompressed size when loading files.
> Also, please remove the dot at the end of the commit message summary.

Done

https://review.coreboot.org/#/c/31564/1//COMMIT_MSG@15 
PS1, Line 15: TEST=boot into kernel on kukui
> But that worked before, didn’t it? Or do you have a test case, where there is a compressor/decompres […]

This CL doesn't aim to fix anything.
Booting into kernel only to check that this CL doesn't screw up something.
Luckily, I don't see any compressor/decompressor bug so far.

https://review.coreboot.org/#/c/31564/1/payloads/libpayload/libcbfs/cbfs_cor... 
File payloads/libpayload/libcbfs/cbfs_core.c:

https://review.coreboot.org/#/c/31564/1/payloads/libpayload/libcbfs/cbfs_cor... 
PS1, Line 259: cbfs_decompress
> We should probably just change cbfs_decompress to use ulzman() and ulz4n() instead? (Would need to a […]

I've created a separate CL for this:
https://review.coreboot.org/c/coreboot/+/31606

https://review.coreboot.org/#/c/31564/1/payloads/libpayload/libcbfs/cbfs_cor... 
PS1, Line 259: 	if (ret != NULL && cbfs_decompress(algo, data, ret, *size) != *size) {
> I agree an error message would be nice, although one should probably be enough (e.g. […]

error message added.

Change in ...coreboot[master]: libpayload: cbfs: Check decompressed size when loading files

[Julius Werner (Code Review)]

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

Change subject: libpayload: cbfs: Check decompressed size when loading files
......................................................................

Patch Set 2: Code-Review+2

Change in ...coreboot[master]: libpayload: cbfs: Check decompressed size when loading files

[Daisuke Nojiri (Code Review)]

Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31564 )

Change subject: libpayload: cbfs: Check decompressed size when loading files
......................................................................

Patch Set 2: Code-Review+2

Change in ...coreboot[master]: libpayload: cbfs: Check decompressed size when loading files

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31564 )

Change subject: libpayload: cbfs: Check decompressed size when loading files
......................................................................

Patch Set 2: Code-Review+1

Change in ...coreboot[master]: libpayload: cbfs: Check decompressed size when loading files

[You-Cheng Syu (Code Review)]

Hello Julius Werner, Daisuke Nojiri, Hung-Te Lin, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: libpayload: cbfs: Check decompressed size when loading files
......................................................................

libpayload: cbfs: Check decompressed size when loading files

After loading compressed files in CBFS, we should check the decompressed
size is equal to the expected size. This might help us detect file
content corruption or compressor/decompressor bugs.

BUG=none
BRANCH=none
TEST=boot into kernel on Kukui

Change-Id: Ia756cc5477670dd0d1d8aa59d4160ab4233c6795
Signed-off-by: You-Cheng Syu 
---
M payloads/libpayload/libcbfs/cbfs_core.c
1 file changed, 11 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/31564/3

Change in ...coreboot[master]: libpayload: cbfs: Check decompressed size when loading files

[Paul Menzel (Code Review)]

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

Change subject: libpayload: cbfs: Check decompressed size when loading files
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31564/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31564/1//COMMIT_MSG@15 
PS1, Line 15: TEST=boot into kernel on kukui
> This CL doesn't aim to fix anything. […]

Understood. Could you please rephrase it this way, that this is just a precaution then?

Change in ...coreboot[master]: libpayload: cbfs: Check decompressed size when loading files

[You-Cheng Syu (Code Review)]

Hello Julius Werner, Daisuke Nojiri, Hung-Te Lin, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: libpayload: cbfs: Check decompressed size when loading files
......................................................................

libpayload: cbfs: Check decompressed size when loading files

After loading compressed files in CBFS, we should check the decompressed
size is equal to the expected size. This might help us detect file
content corruption or compressor/decompressor bugs.

BUG=none
BRANCH=none
TEST=manually (we can still boot into kernel on Kukui, and verify that
     loading files from CBFS still works by seeing ChromiumOS firmware
     screen).

Change-Id: Ia756cc5477670dd0d1d8aa59d4160ab4233c6795
Signed-off-by: You-Cheng Syu 
---
M payloads/libpayload/libcbfs/cbfs_core.c
1 file changed, 11 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/31564/4

Change in ...coreboot[master]: libpayload: cbfs: Check decompressed size when loading files

[You-Cheng Syu (Code Review)]

You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31564 )

Change subject: libpayload: cbfs: Check decompressed size when loading files
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/31564/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31564/1//COMMIT_MSG@15 
PS1, Line 15: TEST=boot into kernel on kukui
> Understood. […]

Done

Change in ...coreboot[master]: libpayload: cbfs: Check decompressed size when loading files

[Julius Werner (Code Review)]

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

Change subject: libpayload: cbfs: Check decompressed size when loading files
......................................................................

Patch Set 4: Code-Review+2

Change in ...coreboot[master]: libpayload: cbfs: Check decompressed size when loading files

[Daisuke Nojiri (Code Review)]

Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31564 )

Change subject: libpayload: cbfs: Check decompressed size when loading files
......................................................................

Patch Set 4: Code-Review+2

Change in ...coreboot[master]: libpayload: cbfs: Check decompressed size when loading files

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31564 )

Change subject: libpayload: cbfs: Check decompressed size when loading files
......................................................................

libpayload: cbfs: Check decompressed size when loading files

After loading compressed files in CBFS, we should check the decompressed
size is equal to the expected size. This might help us detect file
content corruption or compressor/decompressor bugs.

BUG=none
BRANCH=none
TEST=manually (we can still boot into kernel on Kukui, and verify that
     loading files from CBFS still works by seeing ChromiumOS firmware
     screen).

Change-Id: Ia756cc5477670dd0d1d8aa59d4160ab4233c6795
Signed-off-by: You-Cheng Syu 
Reviewed-on: https://review.coreboot.org/c/31564
Reviewed-by: Julius Werner 
Reviewed-by: Daisuke Nojiri 
Tested-by: build bot (Jenkins) 
---
M payloads/libpayload/libcbfs/cbfs_core.c
1 file changed, 11 insertions(+), 9 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved
  Daisuke Nojiri: Looks good to me, approved

diff --git a/payloads/libpayload/libcbfs/cbfs_core.c b/payloads/libpayload/libcbfs/cbfs_core.c
index 4ecda30..39bbdb5 100644
--- a/payloads/libpayload/libcbfs/cbfs_core.c
+++ b/payloads/libpayload/libcbfs/cbfs_core.c
@@ -243,12 +243,9 @@
    }

if (algo == CBFS_COMPRESS_NONE) {
-		if (limit != 0 && limit < on_media_size) {
-			*size = limit;
+		if (limit != 0 && limit < on_media_size)
    		on_media_size = limit;
-		} else {
-			*size = on_media_size;
-		}
+		*size = on_media_size;
    }

void *data = m->map(m, handle->media_offset + handle->content_offset,
@@ -257,10 +254,15 @@
    	return NULL;

ret = malloc(*size);
-	if (ret != NULL &&
-	    !cbfs_decompress(algo, data, on_media_size, ret, *size)) {
-		free(ret);
-		ret = NULL;
+	if (ret != NULL) {
+		size_t final_size = cbfs_decompress(algo, data, on_media_size,
+						    ret, *size);
+		if (final_size != *size) {
+			ERROR("Expect %zu bytes but got %zu bytes after "
+			      "decompression.\n", *size, final_size);
+			free(ret);
+			ret = NULL;
+		}
    }

m->unmap(m, data);