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

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: [email protected]
>Assisted-by: Codex:gpt-5
>Signed-off-by: Laxman Acharya Padhya <[email protected]>
>---
> 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
>