[PATCH] um: ubd: validate COW header fields before use

[Michael Bommarito]

read_cow_header() copies the backing-file path and computes geometry
from header fields without validating them, and the historical
"XXX Need to sanity-check the values read from the header" comment was
never addressed. Three image-controlled fields are unsafe:

  - backing_file[]: not guaranteed NUL-terminated. cow_strdup() and the
    later strlen()/printf() walk off the end of the header buffer when a
    crafted image leaves the array unterminated (heap over-read).

  - sectorsize: used as a divisor in cow_sizes()
    (bitmap_len = (size + sectorsize - 1) / (8 * sectorsize)); a zero or
    negative sectorsize causes a divide-by-zero / bogus geometry.

  - For the V3/V3_b layouts the existing "align == 0" check printed a
    warning but fell through and then computed
    ROUND_UP(sizeof(header), 0), another divide-by-zero.

Reject an unterminated backing_file for each header version, turn the
align == 0 warnings into hard errors (goto out), and reject a
non-positive sectorsize before the geometry math runs.

A COW image is parsed from a file the guest opens (ubdN=cow,backing on
the command line, or a cow file an unprivileged user can point ubd at),
so a crafted header is attacker-reachable.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@xxxxxxxxxxxxxxx
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@xxxxxxxxx>
---
Reproduced on a UML KASAN build (ARCH=um) off this base, driving ubd with
crafted COW images. Stock: an unterminated v2 backing_file over-reads the
header buffer (KASAN slab-out-of-bounds read in the strdup/strlen path); a
sectorsize=0 image and a v3 alignment=0 image each hit a divide error in
the cow_sizes()/ROUND_UP geometry. Patched: each crafted image is rejected
in read_cow_header() ('not terminated' / 'invalid sectorsize' / 'align ==
0') and the driver continues. Benign control: a valid COW image opens
cleanly on both stock and patched. Before/after logs available on request.

 arch/um/drivers/cow_user.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/arch/um/drivers/cow_user.c b/arch/um/drivers/cow_user.c
index dc1d1bcd85ec2..bb9f05ac70cf4 100644
--- a/arch/um/drivers/cow_user.c
+++ b/arch/um/drivers/cow_user.c
@@ -279,7 +279,7 @@ int file_reader(__u64 offset, char *buf, int len, void *arg)
 	return pread(fd, buf, len, offset);
 }
 
-/* XXX Need to sanity-check the values read from the header */
+/* Sanity checks are performed after reading each header version below. */
 
 int read_cow_header(int (*reader)(__u64, char *, int, void *), void *arg,
 		    __u32 *version_out, char **backing_file_out,
@@ -325,6 +325,12 @@ int read_cow_header(int (*reader)(__u64, char *, int, void *), void *arg,
 		*sectorsize_out = header->v1.sectorsize;
 		*bitmap_offset_out = sizeof(header->v1);
 		*align_out = *sectorsize_out;
+		if (!memchr(header->v1.backing_file, '\0',
+			    sizeof(header->v1.backing_file))) {
+			cow_printf("%s - V1 backing_file not terminated\n",
+				   __func__);
+			goto out;
+		}
 		file = header->v1.backing_file;
 	}
 	else if (version == 2) {
@@ -338,6 +344,12 @@ int read_cow_header(int (*reader)(__u64, char *, int, void *), void *arg,
 		*sectorsize_out = be32toh(header->v2.sectorsize);
 		*bitmap_offset_out = sizeof(header->v2);
 		*align_out = *sectorsize_out;
+		if (!memchr(header->v2.backing_file, '\0',
+			    sizeof(header->v2.backing_file))) {
+			cow_printf("%s - V2 backing_file not terminated\n",
+				   __func__);
+			goto out;
+		}
 		file = header->v2.backing_file;
 	}
 	/* This is very subtle - see above at union cow_header definition */
@@ -354,8 +366,15 @@ int read_cow_header(int (*reader)(__u64, char *, int, void *), void *arg,
 		if (*align_out == 0) {
 			cow_printf("read_cow_header - invalid COW header, "
 				   "align == 0\n");
+			goto out;
 		}
 		*bitmap_offset_out = ROUND_UP(sizeof(header->v3), *align_out);
+		if (!memchr(header->v3.backing_file, '\0',
+			    sizeof(header->v3.backing_file))) {
+			cow_printf("%s - V3 backing_file not terminated\n",
+				   __func__);
+			goto out;
+		}
 		file = header->v3.backing_file;
 	}
 	else if (version == 3) {
@@ -385,14 +404,27 @@ int read_cow_header(int (*reader)(__u64, char *, int, void *), void *arg,
 		if (*align_out == 0) {
 			cow_printf("read_cow_header - invalid COW header, "
 				   "align == 0\n");
+			goto out;
 		}
 		*bitmap_offset_out = ROUND_UP(sizeof(header->v3_b), *align_out);
+		if (!memchr(header->v3_b.backing_file, '\0',
+			    sizeof(header->v3_b.backing_file))) {
+			cow_printf("%s - V3 backing_file not terminated\n",
+				   __func__);
+			goto out;
+		}
 		file = header->v3_b.backing_file;
 	}
 	else {
 		cow_printf("read_cow_header - invalid COW version\n");
 		goto out;
 	}
+
+	if (*sectorsize_out <= 0) {
+		cow_printf("%s - invalid sectorsize %d\n", __func__,
+			   *sectorsize_out);
+		goto out;
+	}
 	err = -ENOMEM;
 	*backing_file_out = cow_strdup(file);
 	if (*backing_file_out == NULL) {

base-commit: ef0c9f75a19532d7675384708fc8621e10850104
-- 
2.53.0