Btrfs: avoid buffer overrun in mount option handling

PATCH In review Jim Meyering 2 participants 3 messages Apr 25, 2012 – Apr 26, 2012 Btrfs file system

Share & tools

Jim Meyering

author

2012-04-25 19:52 UTC #0 lore raw

PATCH 1/1 Btrfs: avoid buffer overrun in mount option handling

There is an off-by-one error: allocating room for a maximal result string but without room for a trailing NUL. That, can lead to returning a transformed string that is not NUL-terminated, and then to a caller reading beyond end of the malloc'd buffer.

Worse still, we could write one non-NUL byte beyond the end of that malloc'd result buffer when given a mount option of "subvol=," (i.e., no arg after the "=") because here the replacement "subvolid=0" is 3 bytes longer, not just 2.

Also changed: s/kzalloc/kmalloc/, remove unwarranted use of strncpy (the result is guaranteed to fit), remove dead strlen at end, and change a few variable names and comments.

Signed-off-by Jim Meyering <[email protected]>

---
Hi Josef,

I took the liberty of rewriting this function.
If you would prefer a smaller patch, let me know.

Also, while this removes more lines than it adds, it could easily remove two more if only we could rely on the stpcpy function, but alas, I think that's not an option, since these would be the first two uses:

@@ -952,12 +952,10 @@ static char *setup_root_args(char *args)
*/
if (src != args) {
*src++ = '\0';
- strcpy(buf, args);
- dst += strlen(args);
+ dst = stpcpy(buf, args);
}

- strcpy(dst, "subvolid=0");
- dst += strlen("subvolid=0");
+ dst = stpcpy(dst, "subvolid=0");

/*
* If there is a "," after the original subvol=... string,

 fs/btrfs/super.c |   67 +++++++++++++++++++++---------------------------------
 1 file changed, 26 insertions(+), 41 deletions(-)

fs/btrfs/super.c +26 ~~-41~~

index 8d5d380..eca8dea 100644
@@ -926,63 +926,48 @@ static inline int is_subvolume_inode(struct inode *inode)
  */
 static char *setup_root_args(char *args)
 {
-	unsigned copied = 0;
-	unsigned len = strlen(args) + 2;
-	char *pos;
-	char *ret;
+	unsigned len = strlen(args) + 3 + 1;
+	char *src, *dst, *buf;
 
 	/*
-	 * We need the same args as before, but minus
+	 * We need the same args as before, but with this substitution:
+	 * s!subvol=[^,]*!subvolid=0!
 	 *
-	 * subvol=a
-	 *
-	 * and add
-	 *
-	 * subvolid=0
-	 *
-	 * which is a difference of 2 characters, so we allocate strlen(args) +
-	 * 2 characters.
+	 * Since the replacement string is up to 3 bytes longer than the
+	 * original, allocate strlen(args) + 3 + 1 bytes.
 	 */
-	ret = kzalloc(len * sizeof(char), GFP_NOFS);
-	if (!ret)
-		return NULL;
-	pos = strstr(args, "subvol=");
 
+	src = strstr(args, "subvol=");
 	/* This shouldn't happen, but just in case.. */
-	if (!pos) {
-		kfree(ret);
+	if (!src)
+		return NULL;
+
+	buf = dst = kmalloc(len, GFP_NOFS);
+	if (!buf)
 		return NULL;
-	}
 
 	/*
-	 * The subvol=<> arg is not at the front of the string, copy everybody
-	 * up to that into ret.
+	 * If the subvol= arg is not at the start of the string,
+	 * copy whatever precedes it into buf.
 	 */
-	if (pos != args) {
-		*pos = '\0';
-		strcpy(ret, args);
-		copied += strlen(args);
-		pos++;
+	if (src != args) {
+		*src++ = '\0';
+		strcpy(buf, args);
+		dst += strlen(args);
 	}
 
-	strncpy(ret + copied, "subvolid=0", len - copied);
-
-	/* Length of subvolid=0 */
-	copied += 10;
+	strcpy(dst, "subvolid=0");
+	dst += strlen("subvolid=0");
 
 	/*
-	 * If there is no , after the subvol= option then we know there's no
-	 * other options and we can just return.
+	 * If there is a "," after the original subvol=... string,
+	 * copy that suffix into our buffer.  Otherwise, we're done.
 	 */
-	pos = strchr(pos, ',');
-	if (!pos)
-		return ret;
+	src = strchr(src, ',');
+	if (src)
+		strcpy(dst, src);
 
-	/* Copy the rest of the arguments into our buffer */
-	strncpy(ret + copied, pos, len - copied);
-	copied += strlen(pos);
-
-	return ret;
+	return buf;
 }
 
 static struct dentry *mount_subvol(const char *subvol_name, int flags,

--
1.7.10.335.g879d8

Josef Bacik

2012-04-25 20:33 UTC #1 lore raw

Quoted 10 lines · Jim Meyering

There is an off-by-one error: allocating room for a maximal result string but without room for a trailing NUL. That, can lead to returning a transformed string that is not NUL-terminated, and then to a caller reading beyond end of the malloc'd buffer.
Worse still, we could write one non-NUL byte beyond the end of that malloc'd result buffer when given a mount option of "subvol=," (i.e., no arg after the "=") because here the replacement "subvolid=0" is 3 bytes longer, not just 2.

So this can't happen, since it would be caught by btrfs_parse_early_options which expects subvolid=%d, and if it doesn't get that it would just error out, so while adding another byte isn't bad, it's not correct either, so fix that and I'd say it's good to go. Thanks,

Josef

Jim Meyering

author Reviewed-by Josef Bacik

2012-04-26 09:49 UTC #2 lore raw

Quoted 11 lines · Josef Bacik

Quoted 9 lines · Jim Meyering
There is an off-by-one error: allocating room for a maximal result string but without room for a trailing NUL. That, can lead to returning a transformed string that is not NUL-terminated, and then to a caller reading beyond end of the malloc'd buffer.
Worse still, we could write one non-NUL byte beyond the end of that malloc'd result buffer when given a mount option of "subvol=," (i.e., no arg after the "=") because here the replacement "subvolid=0" is 3 bytes longer, not just 2.

Quoted 4 lines

So this can't happen, since it would be caught by btrfs_parse_early_options which expects subvolid=%d, and if it doesn't get that it would just error out, so while adding another byte isn't bad, it's not correct either, so fix that and I'd say it's good to go. Thanks,

Hi Josef,

Thanks for the quick review.
Here's the incremental change, then the full revised commit:

fs/btrfs/super.c +4 -4

index eca8dea..5ddf172 100644
@@ -926,15 +926,15 @@ static inline int is_subvolume_inode(struct inode *inode)
  */
 static char *setup_root_args(char *args)
 {
-	unsigned len = strlen(args) + 3 + 1;
+	unsigned len = strlen(args) + 2 + 1;
 	char *src, *dst, *buf;
 
 	/*
 	 * We need the same args as before, but with this substitution:
-	 * s!subvol=[^,]*!subvolid=0!
+	 * s!subvol=[^,]+!subvolid=0!
 	 *
-	 * Since the replacement string is up to 3 bytes longer than the
-	 * original, allocate strlen(args) + 3 + 1 bytes.
+	 * Since the replacement string is up to 2 bytes longer than the
+	 * original, allocate strlen(args) + 2 + 1 bytes.
 	 */
 
 	src = strstr(args, "subvol=");

Quoted 1 line

From 312b051535dea0fb208a7dcbe6ac6f30d948825e Mon Sep 17 00:00:00 2001

From: Jim Meyering <[email protected]>
Date: Wed, 25 Apr 2012 21:24:17 +0200
Subject: [PATCH] Btrfs: avoid buffer overrun in mount option handling

Rewrite to s/kzalloc/kmalloc/, remove unwarranted use of strncpy (the result is guaranteed to fit), remove dead strlen at end, and change a few variable names and comments.

Reviewed-by Josef Bacik <[email protected]>

Signed-off-by Jim Meyering <[email protected]>

---
 fs/btrfs/super.c |   67 +++++++++++++++++++++---------------------------------
 1 file changed, 26 insertions(+), 41 deletions(-)

fs/btrfs/super.c +26 ~~-41~~

89 lines not on this page · raw message

--
1.7.10.335.g879d8