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: Enzo Matsumiya <ematsumiya@suse.de>
Signed-off-by: Steve French <stfrench@microsoft.com>

diff --git a/fs/smb/client/compress.c b/fs/smb/client/compress.c
index 3d1e73f5d9af92f7cccbbdf8d41a75b95b454ba0..be9023f841e691703c22540612b7304e8ce86bd1 100644 (file)
--- a/fs/smb/client/compress.c
+++ b/fs/smb/client/compress.c
@@ -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;

diff --git a/fs/smb/client/compress/lz77.c b/fs/smb/client/compress/lz77.c
index cdd6b53766b0a2b90704d2f36f07603593c03852..c1e7fada6e61c425818be03563deddd1a18ebc94 100644 (file)
--- a/fs/smb/client/compress/lz77.c
+++ b/fs/smb/client/compress/lz77.c
@@ -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)

diff --git a/fs/smb/client/compress/lz77.h b/fs/smb/client/compress/lz77.h
index cdcb191b48a23b0795dd400daa709ed804d226d6..2603eab9e071c0566e559274ad661471f2c527fc 100644 (file)
--- a/fs/smb/client/compress/lz77.h
+++ b/fs/smb/client/compress/lz77.h
@@ -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 */