linux-kernel - Re: [PATCH v3] lz4: fix LZ4_decompress_safe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CCE83845-DC40-4E14-9105-6319C048FACB@fb.com>
Date:   Fri, 19 Nov 2021 18:23:24 +0000
From:   Nick Terrell <terrelln@...com>
To:     Guo Xuenan <guoxuenan@...wei.com>
CC:     Andrew Morton <akpm@...ux-foundation.org>,
        Chengyang Fan <cy.fan@...wei.com>, Yann Collet <cyan@...com>,
        "fangwei1@...wei.com" <fangwei1@...wei.com>,
        "hsiangkao@...ux.alibaba.com" <hsiangkao@...ux.alibaba.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "syzbot+63d688f1d899c588fb71@...kaller.appspotmail.com" 
        <syzbot+63d688f1d899c588fb71@...kaller.appspotmail.com>,
        "wangli74@...wei.com" <wangli74@...wei.com>
Subject: Re: [PATCH v3] lz4: fix LZ4_decompress_safe_partial read out of bound



> On Nov 11, 2021, at 2:50 AM, Guo Xuenan <guoxuenan@...wei.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@...kaller.appspotmail.com
> Cc: hsiangkao@...ux.alibaba.com
> Cc: terrelln@...com
> Cc: cyan@...com
> Cc: cy.fan@...wei.com
> Signed-off-by: Guo Xuenan <guoxuenan@...wei.com>

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

Reviewed-by: Nick Terrell <terrelln@...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
>