Change in ...coreboot[master]: libpayload: cbfs: Require input size and output size for cbfs_decompress
You-Cheng Syu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31606
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
libpayload: cbfs: Require input size and output size for cbfs_decompress
Currently, cbfs_decompress() calls ulzma() and ulz4f() for LZMA/LZ4 decompression. These two functions don't accept input/output size as parameters. We can make cbfs_decompress more robust by calling ulzman() and ulz4fn() instead. This could prevent us from overflowing destination buffer.
BUG=none BRANCH=none TEST=boot into kernel on Kukui with COMPRESSED_PAYLOAD_LZMA / COMPRESSED_PAYLOAD_LZ4.
Change-Id: Ibe617825bd000ed618791d8e3c5f65bbbd5f7e33 Signed-off-by: You-Cheng Syu youcheng@google.com --- M payloads/libpayload/include/cbfs_core.h M payloads/libpayload/libcbfs/cbfs.c M payloads/libpayload/libcbfs/cbfs_core.c 3 files changed, 13 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/31606/1
diff --git a/payloads/libpayload/include/cbfs_core.h b/payloads/libpayload/include/cbfs_core.h index 364f6c4..0206f5e 100644 --- a/payloads/libpayload/include/cbfs_core.h +++ b/payloads/libpayload/include/cbfs_core.h @@ -253,8 +253,8 @@ void *cbfs_get_file_content(struct cbfs_media *media, const char *name, int type, size_t *sz);
-/* returns decompressed size on success, 0 on failure */ -int cbfs_decompress(int algo, void *src, void *dst, int len); +/* Returns decompressed size on success, 0 on failure. */ +int cbfs_decompress(int algo, const void *src, int srcn, void *dst, int dstn);
/* returns a pointer to CBFS master header, or CBFS_HEADER_INVALID_ADDRESS * on failure */ diff --git a/payloads/libpayload/libcbfs/cbfs.c b/payloads/libpayload/libcbfs/cbfs.c index 577fb20..d679e67 100644 --- a/payloads/libpayload/libcbfs/cbfs.c +++ b/payloads/libpayload/libcbfs/cbfs.c @@ -114,8 +114,9 @@ final_size = cbfs_decompress(stage->compression, ((unsigned char *) stage) + sizeof(struct cbfs_stage), + stage->len, (void *) (uintptr_t) stage->load, - stage->len); + stage->memlen); if (!final_size) { entry = -1; goto out; diff --git a/payloads/libpayload/libcbfs/cbfs_core.c b/payloads/libpayload/libcbfs/cbfs_core.c index 91f2603..75d5e9a 100644 --- a/payloads/libpayload/libcbfs/cbfs_core.c +++ b/payloads/libpayload/libcbfs/cbfs_core.c @@ -48,6 +48,7 @@ * */
+#include <libpayload.h> #include <cbfs.h> #include <string.h> #include <sysinfo.h> @@ -256,7 +257,8 @@ return NULL;
ret = malloc(*size); - if (ret != NULL && !cbfs_decompress(algo, data, ret, *size)) { + if (ret != NULL && + !cbfs_decompress(algo, data, on_media_size, ret, *size)) { free(ret); ret = NULL; } @@ -321,23 +323,25 @@ return NULL; }
-int cbfs_decompress(int algo, void *src, void *dst, int len) +int cbfs_decompress(int algo, const void *src, int srcn, void *dst, int dstn) { + int len; switch (algo) { case CBFS_COMPRESS_NONE: + len = MIN(srcn, dstn); memcpy(dst, src, len); return len; #ifdef CBFS_CORE_WITH_LZMA case CBFS_COMPRESS_LZMA: - return ulzma(src, dst); + return ulzman(src, srcn, dst, dstn); #endif #ifdef CBFS_CORE_WITH_LZ4 case CBFS_COMPRESS_LZ4: - return ulz4f(src, dst); + return ulz4fn(src, srcn, dst, dstn); #endif default: ERROR("tried to decompress %d bytes with algorithm #%x," - "but that algorithm id is unsupported.\n", len, + "but that algorithm id is unsupported.\n", srcn, algo); return 0; }
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/include/cbfs_cor... File payloads/libpayload/include/cbfs_core.h:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/include/cbfs_cor... PS1, Line 257: int srcn These should be changed to size_t while we're here.
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... File payloads/libpayload/libcbfs/cbfs_core.c:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... PS1, Line 331: len = MIN(srcn, dstn); nit: Maybe just
assert(srcn <= dstn); memcpy(dst, src, srcn);
?
Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... File payloads/libpayload/libcbfs/cbfs_core.c:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... PS1, Line 331: len = MIN(srcn, dstn);
nit: Maybe just […]
That'll remove partial decompression from the function? Currently, at least _NONE and _LZMA look supporting it.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... File payloads/libpayload/libcbfs/cbfs_core.c:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... PS1, Line 331: len = MIN(srcn, dstn);
That'll remove partial decompression from the function? Currently, at least _NONE and _LZMA look sup […]
Well, it may be supported here but not in the only two functions calling this (I don't think any payload would call this directly), so I don't think that's particularly important.
Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... File payloads/libpayload/libcbfs/cbfs_core.c:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... PS1, Line 331: len = MIN(srcn, dstn);
Well, it may be supported here but not in the only two functions calling this (I don't think any pay […]
Taking your 'nit' literally, I think we don't have a strong reason to remove it. So, why don't we keep it?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... File payloads/libpayload/libcbfs/cbfs_core.c:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... PS1, Line 331: len = MIN(srcn, dstn);
Taking your 'nit' literally, I think we don't have a strong reason to remove it. […]
It would just separate error behavior from expected behavior better. If you want to preserve the partial decompression thing, how about
len = MIN(srcn, dstn); memcpy(dst, src, len); if (len < srcn) return 0; /* treat not enough buffer space to "decompress" as error */ return len;
? That's in line with the other two (they also return 0 for partial decompression).
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/include/cbfs_cor... File payloads/libpayload/include/cbfs_core.h:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/include/cbfs_cor... PS1, Line 257: int srcn
These should be changed to size_t while we're here.
edit: return value too, please. all the functions called from here return unsigned.
Hello Julius Werner, Daisuke Nojiri, Hung-Te Lin, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31606
to look at the new patch set (#2).
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
libpayload: cbfs: Require input size and output size for cbfs_decompress
Currently, cbfs_decompress() calls ulzma() and ulz4f() for LZMA/LZ4 decompression. These two functions don't accept input/output size as parameters. We can make cbfs_decompress more robust by calling ulzman() and ulz4fn() instead. This could prevent us from overflowing destination buffer.
BUG=none BRANCH=none TEST=boot into kernel on Kukui with COMPRESSED_PAYLOAD_LZMA / COMPRESSED_PAYLOAD_LZ4.
Change-Id: Ibe617825bd000ed618791d8e3c5f65bbbd5f7e33 Signed-off-by: You-Cheng Syu youcheng@google.com --- M payloads/libpayload/include/cbfs_core.h M payloads/libpayload/libcbfs/cbfs.c M payloads/libpayload/libcbfs/cbfs_core.c 3 files changed, 21 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/31606/2
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/include/cbfs_cor... File payloads/libpayload/include/cbfs_core.h:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/include/cbfs_cor... PS1, Line 257: int srcn
edit: return value too, please. all the functions called from here return unsigned.
Done
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... File payloads/libpayload/libcbfs/cbfs_core.c:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... PS1, Line 331: len = MIN(srcn, dstn);
It would just separate error behavior from expected behavior better. […]
I think using assert here is not consistent (it halts while other algorithms just return 0 on error).
'Do memcpy and then return 0 when srcn > dstn' also sounds weird to me because caller only knows 'there is an error' and doesn't know how much bytes can they use, hence there is no need to prepare the partial output. (And it seems like ulz4fn() returns 0 for partial decompression while ulzman() returns the partial decompressed length.)
When srcn > dstn, I think we should either: 1. memcpy(dst, src, MIN(srcn, dstn)); return MIN(srcn, dstn); 2. return 0;
As partial result is not really being used, I think we can just return 0 for now?
Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... File payloads/libpayload/libcbfs/cbfs_core.c:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... PS1, Line 331: len = MIN(srcn, dstn);
I think using assert here is not consistent (it halts while other algorithms just return 0 on error) […]
How about:
- Return whatever number of bytes copied to dst buffer. Callers are responsible for handling size mismatch (src > dst or src < dst). The API doesn't know what is expected but callers do. - If partial decompression isn't supported and dst buffer is short, return 0. - For any other errors, return 0.
I think these are more or less aligned with what developers expect. So, it'll reduce the stress and surprises because the behavior is consistent across algorithms.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... File payloads/libpayload/libcbfs/cbfs_core.c:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... PS1, Line 331: len = MIN(srcn, dstn);
while ulzman() returns the partial decompressed length.
Pretty sure it doesn't. At the end it does 'if (res != 0) return 0'.
Doing the partial copy but still returning 0 would be in line with both other algorithms. Our signatures for now have always been 0: error, >0: complete success, I would be wary of changing that because payloads may expect it (they probably don't call cbfs_decompress(), but they may call ulzman/ulz4fn directly). You could also no longer distinguish success from partial success on the versions that don't pass in the size that way.
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... File payloads/libpayload/libcbfs/cbfs_core.c:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... PS1, Line 331: len = MIN(srcn, dstn);
while ulzman() returns the partial decompressed length. […]
Pretty sure it does. See this: https://pastebin.com/8Eh1AkSU LzmaDecode actually returns 0 for partial decompression.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... File payloads/libpayload/libcbfs/cbfs_core.c:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... PS1, Line 331: len = MIN(srcn, dstn);
Pretty sure it does. See this: https://pastebin.com/8Eh1AkSU […]
Ohh... okay, interesting, So maybe it is LZ4 which we should align to the others then? I think if you just change ulz4fn() to always return (out - dst) directly (and adjust out before the break in the output overrun case), that should do it.
Hello Julius Werner, Daisuke Nojiri, Hung-Te Lin, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31606
to look at the new patch set (#3).
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
libpayload: cbfs: Require input size and output size for cbfs_decompress
Currently, cbfs_decompress() calls ulzma() and ulz4f() for LZMA/LZ4 decompression. These two functions don't accept input/output size as parameters. We can make cbfs_decompress more robust by calling ulzman() and ulz4fn() instead. This could prevent us from overflowing destination buffer.
BUG=none BRANCH=none TEST=boot into kernel on Kukui with COMPRESSED_PAYLOAD_LZMA / COMPRESSED_PAYLOAD_LZ4.
Change-Id: Ibe617825bd000ed618791d8e3c5f65bbbd5f7e33 Signed-off-by: You-Cheng Syu youcheng@google.com --- M payloads/libpayload/include/cbfs_core.h M payloads/libpayload/libcbfs/cbfs.c M payloads/libpayload/libcbfs/cbfs_core.c 3 files changed, 17 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/31606/3
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... File payloads/libpayload/libcbfs/cbfs_core.c:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... PS1, Line 331: len = MIN(srcn, dstn);
Ohh... […]
Then I think we can copy as many bytes as possible here and return the number of copied bytes for now, and change liblz4 in a different CL?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... File payloads/libpayload/libcbfs/cbfs_core.c:
https://review.coreboot.org/#/c/31606/1/payloads/libpayload/libcbfs/cbfs_cor... PS1, Line 331: len = MIN(srcn, dstn);
Then I think we can copy as many bytes as possible here and return the number of copied bytes for no […]
Yes, sounds good, thanks.
Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31606 )
Change subject: libpayload: cbfs: Require input size and output size for cbfs_decompress ......................................................................
libpayload: cbfs: Require input size and output size for cbfs_decompress
Currently, cbfs_decompress() calls ulzma() and ulz4f() for LZMA/LZ4 decompression. These two functions don't accept input/output size as parameters. We can make cbfs_decompress more robust by calling ulzman() and ulz4fn() instead. This could prevent us from overflowing destination buffer.
BUG=none BRANCH=none TEST=boot into kernel on Kukui with COMPRESSED_PAYLOAD_LZMA / COMPRESSED_PAYLOAD_LZ4.
Change-Id: Ibe617825bd000ed618791d8e3c5f65bbbd5f7e33 Signed-off-by: You-Cheng Syu youcheng@google.com Reviewed-on: https://review.coreboot.org/c/31606 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Daisuke Nojiri dnojiri@chromium.org --- M payloads/libpayload/include/cbfs_core.h M payloads/libpayload/libcbfs/cbfs.c M payloads/libpayload/libcbfs/cbfs_core.c 3 files changed, 17 insertions(+), 10 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/include/cbfs_core.h b/payloads/libpayload/include/cbfs_core.h index 364f6c4..a707154 100644 --- a/payloads/libpayload/include/cbfs_core.h +++ b/payloads/libpayload/include/cbfs_core.h @@ -253,8 +253,9 @@ void *cbfs_get_file_content(struct cbfs_media *media, const char *name, int type, size_t *sz);
-/* returns decompressed size on success, 0 on failure */ -int cbfs_decompress(int algo, void *src, void *dst, int len); +/* Returns decompressed size on success, 0 on failure. */ +size_t cbfs_decompress(int algo, const void *src, size_t srcn, void *dst, + size_t dstn);
/* returns a pointer to CBFS master header, or CBFS_HEADER_INVALID_ADDRESS * on failure */ diff --git a/payloads/libpayload/libcbfs/cbfs.c b/payloads/libpayload/libcbfs/cbfs.c index 577fb20..d679e67 100644 --- a/payloads/libpayload/libcbfs/cbfs.c +++ b/payloads/libpayload/libcbfs/cbfs.c @@ -114,8 +114,9 @@ final_size = cbfs_decompress(stage->compression, ((unsigned char *) stage) + sizeof(struct cbfs_stage), + stage->len, (void *) (uintptr_t) stage->load, - stage->len); + stage->memlen); if (!final_size) { entry = -1; goto out; diff --git a/payloads/libpayload/libcbfs/cbfs_core.c b/payloads/libpayload/libcbfs/cbfs_core.c index 91f2603..4ecda30 100644 --- a/payloads/libpayload/libcbfs/cbfs_core.c +++ b/payloads/libpayload/libcbfs/cbfs_core.c @@ -48,6 +48,7 @@ * */
+#include <libpayload.h> #include <cbfs.h> #include <string.h> #include <sysinfo.h> @@ -256,7 +257,8 @@ return NULL;
ret = malloc(*size); - if (ret != NULL && !cbfs_decompress(algo, data, ret, *size)) { + if (ret != NULL && + !cbfs_decompress(algo, data, on_media_size, ret, *size)) { free(ret); ret = NULL; } @@ -321,24 +323,27 @@ return NULL; }
-int cbfs_decompress(int algo, void *src, void *dst, int len) +size_t cbfs_decompress(int algo, const void *src, size_t srcn, void *dst, + size_t dstn) { + size_t len; switch (algo) { case CBFS_COMPRESS_NONE: + len = MIN(srcn, dstn); memcpy(dst, src, len); return len; #ifdef CBFS_CORE_WITH_LZMA case CBFS_COMPRESS_LZMA: - return ulzma(src, dst); + return ulzman(src, srcn, dst, dstn); #endif #ifdef CBFS_CORE_WITH_LZ4 case CBFS_COMPRESS_LZ4: - return ulz4f(src, dst); + return ulz4fn(src, srcn, dst, dstn); #endif default: - ERROR("tried to decompress %d bytes with algorithm #%x," - "but that algorithm id is unsupported.\n", len, - algo); + ERROR("tried to decompress %zu bytes with algorithm " + "#%x, but that algorithm id is unsupported.\n", + srcn, algo); return 0; } }
-
Daisuke Nojiri (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
You-Cheng Syu (Code Review)