Alexandre Rebert has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39174 )
Change subject: lz4: Fix out-of-bounds reads ......................................................................
lz4: Fix out-of-bounds reads
Fix two out-of-bounds reads in lz4 decompression:
1) LZ4_decompress_generic could read one byte past the input buffer when decoding variable length literals due to a missing bounds check. This issue was resolved in libpayload, commonlib and cbfstool
2) ulz4fn could read up to 4 bytes past the input buffer when reading a lz4_block_header due to a missing bounds check. This issue was resolved in libpayload and commonlib.
Change-Id: I5afdf7e1d43ecdb06c7b288be46813c1017569fc Signed-off-by: Alex Rebert alexandre.rebert@gmail.com Found-by: Mayhem --- M payloads/libpayload/liblz4/lz4.c.inc M payloads/libpayload/liblz4/lz4_wrapper.c M src/commonlib/bsd/lz4.c.inc M src/commonlib/bsd/lz4_wrapper.c M util/cbfstool/lz4/lib/lz4.c 5 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/39174/1
diff --git a/payloads/libpayload/liblz4/lz4.c.inc b/payloads/libpayload/liblz4/lz4.c.inc index baa9110..68fac47 100644 --- a/payloads/libpayload/liblz4/lz4.c.inc +++ b/payloads/libpayload/liblz4/lz4.c.inc @@ -150,6 +150,7 @@ if ((length=(token>>ML_BITS)) == RUN_MASK) { unsigned s; + if ((endOnInput) && unlikely(ip>=iend-RUN_MASK)) goto _output_error; /* overflow detection */ do { s = *ip++; diff --git a/payloads/libpayload/liblz4/lz4_wrapper.c b/payloads/libpayload/liblz4/lz4_wrapper.c index d125ce33..3d17fe6 100644 --- a/payloads/libpayload/liblz4/lz4_wrapper.c +++ b/payloads/libpayload/liblz4/lz4_wrapper.c @@ -141,6 +141,9 @@ }
while (1) { + if ((size_t)(in - src) + sizeof(struct lz4_block_header) > srcn) + break; /* input overrun */ + struct lz4_block_header b = { .raw = le32toh(*(uint32_t *)in) }; in += sizeof(struct lz4_block_header);
diff --git a/src/commonlib/bsd/lz4.c.inc b/src/commonlib/bsd/lz4.c.inc index b3be4e5..8c75e2f 100644 --- a/src/commonlib/bsd/lz4.c.inc +++ b/src/commonlib/bsd/lz4.c.inc @@ -150,6 +150,7 @@ if ((length=(token>>ML_BITS)) == RUN_MASK) { unsigned s; + if ((endOnInput) && unlikely(ip>=iend-RUN_MASK)) goto _output_error; /* overflow detection */ do { s = *ip++; diff --git a/src/commonlib/bsd/lz4_wrapper.c b/src/commonlib/bsd/lz4_wrapper.c index 2367afc..3822e8c 100644 --- a/src/commonlib/bsd/lz4_wrapper.c +++ b/src/commonlib/bsd/lz4_wrapper.c @@ -129,6 +129,9 @@ }
while (1) { + if ((size_t)(in - src) + sizeof(struct lz4_block_header) > srcn) + break; /* input overrun */ + struct lz4_block_header b = { { .raw = le32toh(*(const uint32_t *)in) } }; diff --git a/util/cbfstool/lz4/lib/lz4.c b/util/cbfstool/lz4/lib/lz4.c index 9c9a9a0..e393690 100644 --- a/util/cbfstool/lz4/lib/lz4.c +++ b/util/cbfstool/lz4/lib/lz4.c @@ -1206,6 +1206,7 @@ if ((length=(token>>ML_BITS)) == RUN_MASK) { unsigned s; + if ((endOnInput) && unlikely(ip>=iend-RUN_MASK)) goto _output_error; /* overflow detection */ do { s = *ip++;
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39174 )
Change subject: lz4: Fix out-of-bounds reads ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39174/1/util/cbfstool/lz4/lib/lz4.c File util/cbfstool/lz4/lib/lz4.c:
https://review.coreboot.org/c/coreboot/+/39174/1/util/cbfstool/lz4/lib/lz4.c... PS1, Line 1209: if ((endOnInput) && unlikely(ip>=iend-RUN_MASK)) goto _output_error; /* overflow detection */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/39174/1/util/cbfstool/lz4/lib/lz4.c... PS1, Line 1209: if ((endOnInput) && unlikely(ip>=iend-RUN_MASK)) goto _output_error; /* overflow detection */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39174/1/util/cbfstool/lz4/lib/lz4.c... PS1, Line 1209: if ((endOnInput) && unlikely(ip>=iend-RUN_MASK)) goto _output_error; /* overflow detection */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39174/1/util/cbfstool/lz4/lib/lz4.c... PS1, Line 1209: if ((endOnInput) && unlikely(ip>=iend-RUN_MASK)) goto _output_error; /* overflow detection */ spaces required around that '>=' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39174/1/util/cbfstool/lz4/lib/lz4.c... PS1, Line 1209: if ((endOnInput) && unlikely(ip>=iend-RUN_MASK)) goto _output_error; /* overflow detection */ trailing statements should be on next line
Alexandre Rebert has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39174 )
Change subject: lz4: Fix out-of-bounds reads ......................................................................
Patch Set 1:
I ignored the checkpath warnings in cbfstool for consistency within util/cbfstool/lz4/lib/lz4.c, since this file did not seem to follow the enforced style guide.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39174 )
Change subject: lz4: Fix out-of-bounds reads ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39174 )
Change subject: lz4: Fix out-of-bounds reads ......................................................................
lz4: Fix out-of-bounds reads
Fix two out-of-bounds reads in lz4 decompression:
1) LZ4_decompress_generic could read one byte past the input buffer when decoding variable length literals due to a missing bounds check. This issue was resolved in libpayload, commonlib and cbfstool
2) ulz4fn could read up to 4 bytes past the input buffer when reading a lz4_block_header due to a missing bounds check. This issue was resolved in libpayload and commonlib.
Change-Id: I5afdf7e1d43ecdb06c7b288be46813c1017569fc Signed-off-by: Alex Rebert alexandre.rebert@gmail.com Found-by: Mayhem Reviewed-on: https://review.coreboot.org/c/coreboot/+/39174 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Georgi pgeorgi@google.com --- M payloads/libpayload/liblz4/lz4.c.inc M payloads/libpayload/liblz4/lz4_wrapper.c M src/commonlib/bsd/lz4.c.inc M src/commonlib/bsd/lz4_wrapper.c M util/cbfstool/lz4/lib/lz4.c 5 files changed, 9 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/payloads/libpayload/liblz4/lz4.c.inc b/payloads/libpayload/liblz4/lz4.c.inc index baa9110..68fac47 100644 --- a/payloads/libpayload/liblz4/lz4.c.inc +++ b/payloads/libpayload/liblz4/lz4.c.inc @@ -150,6 +150,7 @@ if ((length=(token>>ML_BITS)) == RUN_MASK) { unsigned s; + if ((endOnInput) && unlikely(ip>=iend-RUN_MASK)) goto _output_error; /* overflow detection */ do { s = *ip++; diff --git a/payloads/libpayload/liblz4/lz4_wrapper.c b/payloads/libpayload/liblz4/lz4_wrapper.c index d125ce33..3d17fe6 100644 --- a/payloads/libpayload/liblz4/lz4_wrapper.c +++ b/payloads/libpayload/liblz4/lz4_wrapper.c @@ -141,6 +141,9 @@ }
while (1) { + if ((size_t)(in - src) + sizeof(struct lz4_block_header) > srcn) + break; /* input overrun */ + struct lz4_block_header b = { .raw = le32toh(*(uint32_t *)in) }; in += sizeof(struct lz4_block_header);
diff --git a/src/commonlib/bsd/lz4.c.inc b/src/commonlib/bsd/lz4.c.inc index b3be4e5..8c75e2f 100644 --- a/src/commonlib/bsd/lz4.c.inc +++ b/src/commonlib/bsd/lz4.c.inc @@ -150,6 +150,7 @@ if ((length=(token>>ML_BITS)) == RUN_MASK) { unsigned s; + if ((endOnInput) && unlikely(ip>=iend-RUN_MASK)) goto _output_error; /* overflow detection */ do { s = *ip++; diff --git a/src/commonlib/bsd/lz4_wrapper.c b/src/commonlib/bsd/lz4_wrapper.c index 2367afc..3822e8c 100644 --- a/src/commonlib/bsd/lz4_wrapper.c +++ b/src/commonlib/bsd/lz4_wrapper.c @@ -129,6 +129,9 @@ }
while (1) { + if ((size_t)(in - src) + sizeof(struct lz4_block_header) > srcn) + break; /* input overrun */ + struct lz4_block_header b = { { .raw = le32toh(*(const uint32_t *)in) } }; diff --git a/util/cbfstool/lz4/lib/lz4.c b/util/cbfstool/lz4/lib/lz4.c index 9c9a9a0..e393690 100644 --- a/util/cbfstool/lz4/lib/lz4.c +++ b/util/cbfstool/lz4/lib/lz4.c @@ -1206,6 +1206,7 @@ if ((length=(token>>ML_BITS)) == RUN_MASK) { unsigned s; + if ((endOnInput) && unlikely(ip>=iend-RUN_MASK)) goto _output_error; /* overflow detection */ do { s = *ip++;
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39174 )
Change subject: lz4: Fix out-of-bounds reads ......................................................................
Patch Set 2:
Can you please upstream the lz4.c.inc stuff to https://github.com/lz4/lz4 if you haven't already. I'd prefer not to diverge from there.
Alexandre Rebert has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39174 )
Change subject: lz4: Fix out-of-bounds reads ......................................................................
Patch Set 2:
Patch Set 2:
Can you please upstream the lz4.c.inc stuff to https://github.com/lz4/lz4 if you haven't already. I'd prefer not to diverge from there.
Upstream actually has a patch for this issue already. They refactored that loop (and others) in a read_variable_length function at https://github.com/lz4/lz4/blob/dev/lib/lz4.c#L1630, which has a pre-loop check like the one I added.
In terms of divergence, it seems like upstream changed quite a bit since it was pulled into coreboot. I considered pulling from upstream, but given that coreboot also made modifications to lz4.c.inc and that I'm fairly new to coreboot, I was a bit hesitant to generate a large patch. I can give it a shot if you'd like though.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39174 )
Change subject: lz4: Fix out-of-bounds reads ......................................................................
Patch Set 2:
Upstream actually has a patch for this issue already. They refactored that loop (and others) in a read_variable_length function at https://github.com/lz4/lz4/blob/dev/lib/lz4.c#L1630, which has a pre-loop check like the one I added.
Okay, sounds good!
In terms of divergence, it seems like upstream changed quite a bit since it was pulled into coreboot. I considered pulling from upstream, but given that coreboot also made modifications to lz4.c.inc and that I'm fairly new to coreboot, I was a bit hesitant to generate a large patch. I can give it a shot if you'd like though.
I don't think(?) we made modifications (except for whitespace on a few lines) since I pulled it in, other than this one now. But I also pulled it from the tip of the development branch right after I upstreamed something myself, not sure how much of that made it back to master.
Since it works fine as is I'm a bit wary about making a big uprev. You can try if you want to, but we'd definitely need to check that performance didn't regress. We also need the inPlaceDecode thing, which I added upstream but the maintainer probably doesn't care too much about, so we'd need to make sure that's still there and is still checked for in all cases even where the code was refactored.
-
Alexandre Rebert (Code Review) -
build bot (Jenkins) (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)