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

[Nick Terrell <terrelln@fb.com>]

> On Nov 11, 2021, at 2:50 AM, Guo Xuenan <guoxuenan@huawei.com> 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@google.com/
> [2] https://github.com/lz4/lz4/commit/c5d6f8a8be3927c0bec91bcc58667a6cfad244ad#
> [3] https://lore.kernel.org/all/CC666AE8-4CA4-4951-B6FB-A2EFDE3AC03B@fb.com/
> 
> Reported-by: syzbot+63d688f1d899c588fb71@syzkaller.appspotmail.com
> Cc: hsiangkao@linux.alibaba.com
> Cc: terrelln@fb.com
> Cc: cyan@fb.com
> Cc: cy.fan@huawei.com
> Signed-off-by: Guo Xuenan <guoxuenan@huawei.com>

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

Reviewed-by: Nick Terrell <terrelln@fb.com>

> ---
> 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
>