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

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

[Laxman Acharya Padhya]

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 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@vger.kernel.org
Assisted-by: Codex:gpt-5
Signed-off-by: Laxman Acharya Padhya <acharyalaxman8848@gmail.com>
---
 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

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@vger.kernel.org
>Assisted-by: Codex:gpt-5
>Signed-off-by: Laxman Acharya Padhya <acharyalaxman8848@gmail.com>
>---
> 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
>

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

[Laxman Acharya Padhya]

Hi Enzo,

Thanks for the clarification.

My concern was that the current tree does not appear to enforce that
constraint yet, so the code can still be built in 32-bit configurations
and hit the 32-bit shift case. If the preferred fix is to make the
64-bit/little-endian requirement explicit at build time, I'm happy to
defer to that direction.

Regards,
Laxman