Commit 4c221711 authored by Enzo Matsumiya's avatar Enzo Matsumiya Committed by Steve French
Browse files

smb: client: compress: fix buffer overrun in lz77_compress() 



@dst buffer is allocated with same size as @src, which, for good
compression cases, works fine.

However, when compression goes bad (e.g. random bytes payloads), the
compressed size can increase significantly, and even by stopping the
main loop at 7/8 of @slen, writing leftover literals could write past
the end of @dst because of LZ77 metadata.

To fix this, add lz77_compressed_alloc_size() helper to compute the
correct allocation size for @dst, accounting for metadata and worst
cast scenario (all literals).

While this is overprovisioning memory, it's not only correct, but also
allows lz77_compress() main loop to run without ever checking @dst
limits (i.e. a perf improvement).

Signed-off-by: default avatarEnzo Matsumiya <ematsumiya@suse.de>
Signed-off-by: default avatarSteve French <stfrench@microsoft.com>
parent a55a6088

fs/smb/client/compress.c

+1 −5
Original line number Diff line number Diff line
@@ -329,11 +329,7 @@ int smb_compress(struct TCP_Server_Info *server, struct smb_rqst *rq, compress_s 
		goto err_free;

	}



	/*

	 * This is just overprovisioning, as the algorithm will error out if @dst reaches 7/8

	 * of @slen.

	 */

	dlen = slen;

	dlen = lz77_compressed_alloc_size(slen);

	dst = kvzalloc(dlen, GFP_KERNEL);

	if (!dst) {

		ret = -ENOMEM;

fs/smb/client/compress/lz77.c

+4 −10
Original line number Diff line number Diff line
@@ -137,6 +137,10 @@ noinline int lz77_compress(const void *src, u32 slen, void *dst, u32 *dlen) 
	long flag = 0;

	u64 *htable;



	/* This is probably a bug, so throw a warning. */

	if (WARN_ON_ONCE(*dlen < lz77_compressed_alloc_size(slen)))

		return -EINVAL;



	srcp = src;

	end = src + slen;

	dstp = dst;
@@ -180,15 +184,6 @@ noinline int lz77_compress(const void *src, u32 slen, void *dst, u32 *dlen) 
			continue;

		}



		/*

		 * Bail out if @dstp reached >= 7/8 of @slen -- already compressed badly, not worth

		 * going further.

		 */

		if (unlikely(dstp - dst >= slen - (slen >> 3))) {

			*dlen = slen;

			goto out;

		}



		dstp = lz77_write_match(dstp, &nib, dist, len);

		srcp += len;
@@ -225,7 +220,6 @@ noinline int lz77_compress(const void *src, u32 slen, void *dst, u32 *dlen) 
	lz77_write32(flag_pos, flag);



	*dlen = dstp - dst;

out:

	kvfree(htable);



	if (*dlen < slen)

fs/smb/client/compress/lz77.h

+28 −0
Original line number Diff line number Diff line
@@ -11,5 +11,33 @@ 


#include <linux/kernel.h>



/**

 * lz77_compressed_alloc_size() - Compute compressed buffer size.

 * @size:	uncompressed (src) size

 *

 * Compute allocation size for the compressed buffer based on uncompressed size.

 * Accounts for metadata and overprovision for the worst case scenario.

 *

 * LZ77 metadata is a 4-byte flag that is written:

 * - on dst begin (pos 0)

 * - every 32 literals or matches

 * - on end-of-stream (possibly, if last write was another flag)

 *

 * Worst case scenario is an all-literal compression, which means:

 * metadata bytes = 4 + ((@size / 32) * 4) + 4, or, simplified, (@size >> 3) + 8

 *

 * The worst case scenario rarely happens, but such overprovisioning also allows lz77_compress()

 * main loop to run without ever bound checking dst, which is a huge perf improvement, while also

 * being safe when compression goes bad.

 *

 * Return: required (*) allocation size for compressed buffer.

 *

 * (*) checked once in the beginning of lz77_compress()

 */

static __always_inline u32 lz77_compressed_alloc_size(const u32 size)

{

	return size + (size >> 3) + 8;

}



int lz77_compress(const void *src, u32 slen, void *dst, u32 *dlen);

#endif /* _SMB_COMPRESS_LZ77_H */