Re: [PATCH v3] lz4: fix LZ4_decompress_safe_partial read out of bound

From: Nick Terrell
Date: Fri Nov 19 2021 - 13:23:35 EST




> On Nov 11, 2021, at 2:50 AM, Guo Xuenan <guoxuenan@xxxxxxxxxx> wrote:
>
> When partialDecoding, it is EOF if we've either, filled the output
> buffer or can't proceed with reading an offset for following match.
>
> In some extreme corner cases when compressed data is crusted corrupted,
> UAF will occur. As reported by KASAN [1], LZ4_decompress_safe_partial
> may lead to read out of bound problem during decoding. lz4 upstream has
> fixed it [2] and this issue has been disscussed here [3] before.
>
> current decompression routine was ported from lz4 v1.8.3, bumping lib/lz4
> to v1.9.+ is certainly a huge work to be done later, so, we'd better fix
> it first.
>
> [1] https://lore.kernel.org/all/000000000000830d1205cf7f0477@xxxxxxxxxx/
> [2] https://github.com/lz4/lz4/commit/c5d6f8a8be3927c0bec91bcc58667a6cfad244ad#
> [3] https://lore.kernel.org/all/CC666AE8-4CA4-4951-B6FB-A2EFDE3AC03B@xxxxxx/
>
> Reported-by: syzbot+63d688f1d899c588fb71@xxxxxxxxxxxxxxxxxxxxxxxxx
> Cc: hsiangkao@xxxxxxxxxxxxxxxxx
> Cc: terrelln@xxxxxx
> Cc: cyan@xxxxxx
> Cc: cy.fan@xxxxxxxxxx
> Signed-off-by: Guo Xuenan <guoxuenan@xxxxxxxxxx>

Sorry I’m a bit late to the party, but this looks good to me!

Reviewed-by: Nick Terrell <terrelln@xxxxxx>

> ---
> lib/lz4/lz4_decompress.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/lib/lz4/lz4_decompress.c b/lib/lz4/lz4_decompress.c
> index 926f4823d5ea..fd1728d94bab 100644
> --- a/lib/lz4/lz4_decompress.c
> +++ b/lib/lz4/lz4_decompress.c
> @@ -271,8 +271,12 @@ static FORCE_INLINE int LZ4_decompress_generic(
> ip += length;
> op += length;
>
> - /* Necessarily EOF, due to parsing restrictions */
> - if (!partialDecoding || (cpy == oend))
> + /* Necessarily EOF when !partialDecoding.
> + * When partialDecoding, it is EOF if we've either
> + * filled the output buffer or
> + * can't proceed with reading an offset for following match.
> + */
> + if (!partialDecoding || (cpy == oend) || (ip >= (iend - 2)))
> break;
> } else {
> /* may overwrite up to WILDCOPYLENGTH beyond cpy */
> --
> 2.31.1
>