linux-kernel - Re: [PATCH] lib/lzo: Avoid output overruns when compressing

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] [day] [month] [year] [list]

Message-ID: <hymyvszwshcvqngjlomeyltmpghx6gges76muaz23a6cit5oe2@eas2xjgfynnu>
Date: Sun, 23 Feb 2025 17:40:31 +0900
From: Sergey Senozhatsky <senozhatsky@...omium.org>
To: Herbert Xu <herbert@...dor.apana.org.au>
Cc: Linux Crypto Mailing List <linux-crypto@...r.kernel.org>, 
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, Nitin Gupta <nitingupta910@...il.com>, 
	Richard Purdie <rpurdie@...nedhand.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Linus Torvalds <torvalds@...ux-foundation.org>, Sergey Senozhatsky <senozhatsky@...omium.org>, 
	"Markus F.X.J. Oberhumer" <markus@...rhumer.com>, Dave Rodgman <dave.rodgman@....com>
Subject: Re: [PATCH] lib/lzo: Avoid output overruns when compressing

On (25/02/23 14:55), Herbert Xu wrote:
[..]
>  		} else if (m_off <= M3_MAX_OFFSET) {
>  			m_off -= 1;
> +			if (!HAVE_OP(1))
> +				return LZO_E_OUTPUT_OVERRUN;
>  			if (m_len <= M3_MAX_LEN)
>  				*op++ = (M3_MARKER | (m_len - 2));
>  			else {
[..]
>  		} else {
>  			m_off -= 0x4000;
> +			if (!HAVE_OP(1))
> +				return LZO_E_OUTPUT_OVERRUN;
>  			if (m_len <= M4_MAX_LEN)
>  				*op++ = (M4_MARKER | ((m_off >> 11) & 8)
>  						| (m_len - 2));

Made me wonder if HAVE_OP() in these two cases should have been
under if, but it covers both if and else branches, so it's all
right.

[..]
> +++ b/lib/lzo/lzo1x_decompress_safe.c
> @@ -21,7 +21,6 @@
>  #include "lzodefs.h"
>  
>  #define HAVE_IP(x)      ((size_t)(ip_end - ip) >= (size_t)(x))
> -#define HAVE_OP(x)      ((size_t)(op_end - op) >= (size_t)(x))
>  #define NEED_IP(x)      if (!HAVE_IP(x)) goto input_overrun
>  #define NEED_OP(x)      if (!HAVE_OP(x)) goto output_overrun

A bit of a pity that NEED_OP() with "goto output_overrun" is only
for decompression.  It looks equally relevant to the compression
path.  I'm not insisting on doing something similar in compression
tho.

Overall this look right, and kudos to Herbert for doing this.

I did some testing (using my usual zram test scripts, but modified
zram to allocate only one physical page for comp buf and to check
for overrun ret status).  Haven't noticed any problems.

FWIW
Reviewed-and-tested-by: Sergey Senozhatsky <senozhatsky@...omium.org>