Re: [PATCH] smb: common: fix undefined shifts in LZ77 flag encoding

[Enzo Matsumiya]

Hi,

Thanks for the patch.

On 07/09, Laxman Acharya Padhya wrote:
> The LZ77 encoder emits flags in 32-bit words, but keeps the
> accumulator in a long and can shift it by 32 bits.
>
> This happens when lz77_encode_literals() emits a full all-literal flag
> word, and again when smb_lz77_compress() pads an empty final flag word.
> On 32-bit builds

SMB2 compression code is supposed to be supported on 64-bit (and
little endian) architectures only.

I was going to send a patch to make such checks at build-time (to make
that an explicit "statement"), but I'm waiting for Steve's input on it.


Cheers,

Enzo

> these shift counts are equal to the width of the
> shifted type, so UBSAN can report a runtime error and the encoded flag
> word is undefined.
>
> Use a u32 accumulator and special-case the full-word states so the same
> flag words are emitted without issuing 32-bit shifts.
>
> Fixes: d14bbfff259c ("smb3: mark compression as CONFIG_EXPERIMENTAL and fix missing compression operation")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Codex:gpt-5
> Signed-off-by: Laxman Acharya Padhya <acharyalaxman8848@xxxxxxxxx>
> ---
> fs/smb/common/compress/lz77.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/fs/smb/common/compress/lz77.c b/fs/smb/common/compress/lz77.c
> index 9216d973d87..be6853ad576 100644
> --- a/fs/smb/common/compress/lz77.c
> +++ b/fs/smb/common/compress/lz77.c
> @@ -188,7 +188,7 @@ static __always_inline void *lz77_encode_match(void *dst, void **nib, u16 dist,
>  * MS-XCA 2.3.4 "Plain LZ77 Compression Algorithm Details" - "Processing"
>  */
> static __always_inline void *lz77_encode_literals(const void *start, const void *end, void *dst,
> -						  long *f, u32 *fc, void **fp)
> +						  u32 *f, u32 *fc, void **fp)
> {
> 	if (start >= end)
> 		return dst;
> @@ -201,7 +201,10 @@ static __always_inline void *lz77_encode_literals(const void *start, const void
> 		dst += len;
> 		start += len;
>
> -		*f <<= len;
> +		if (len == LZ77_FLAG_MAX)
> +			*f = 0;
> +		else
> +			*f <<= len;
> 		*fc += len;
> 		if (*fc == LZ77_FLAG_MAX) {
> 			lz77_write32(*fp, *f);
> @@ -225,7 +228,7 @@ noinline int smb_lz77_compress(const void *src, const u32 slen,
> 	const void *srcp, *rlim, *end, *anchor;
> 	u32 *htable, hash, flag_count = 0;
> 	void *dstp, *nib, *flag_pos;
> -	long flag = 0;
> +	u32 flag = 0;
>
> 	/* This is probably a bug, so throw a warning. */
> 	if (WARN_ON_ONCE(*dlen < smb_lz77_compressed_alloc_size(slen)))
> @@ -327,9 +330,13 @@ noinline int smb_lz77_compress(const void *src, const u32 slen,
> out:
> 	dstp = lz77_encode_literals(anchor, end, dstp, &flag, &flag_count, &flag_pos);
>
> -	flag_count = LZ77_FLAG_MAX - flag_count;
> -	flag <<= flag_count;
> -	flag |= (1UL << flag_count) - 1;
> +	if (flag_count) {
> +		flag_count = LZ77_FLAG_MAX - flag_count;
> +		flag <<= flag_count;
> +		flag |= (1U << flag_count) - 1;
> +	} else {
> +		flag = ~0U;
> +	}
> 	lz77_write32(flag_pos, flag);
>
> 	*dlen = dstp - dst;
> --
> 2.53.0