Null terminate strings passed in from userspace

PATCH New Mark Fasheh 1 participant 1 message Jun 30, 2008 Btrfs file system

Share & tools

Mark Fasheh

author

2008-06-30 22:44 UTC #0 lore raw

PATCH 1/1 Null terminate strings passed in from userspace

The 'char name[BTRFS_PATH_NAME_MAX]' member of struct btrfs_ioctl_vol_args is passed directly to strlen() after being copied from user. I haven't verified this, but in theory a userspace program could pass in an unterminated string and cause a kernel crash as strlen walks off the end of the array.

This patch terminates the ->name string in all btrfs ioctl functions which currently use a 'struct btrfs_ioctl_vol_args'. Since the string is now properly terminated, it's length will never be longer than BTRFS_PATH_NAME_MAX so that error check has been removed.

By the way, it might be better overall to just have the ioctl pass an unterminated string + length structure but I didn't bother with that since it'd change the kernel/user interface.

Signed-off-by Mark Fasheh <[email protected]>

ioctl.c +5 -8

diff -r 3f0eee804974 -r be405df89141 ioctl.c
@@ -301,11 +301,9 @@
 		ret = -EFAULT;
 		goto out;
 	}
+
+	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
 	namelen = strlen(vol_args->name);
-	if (namelen > BTRFS_VOL_NAME_MAX) {
-		ret = -EINVAL;
-		goto out;
-	}
 
 	mutex_lock(&root->fs_info->alloc_mutex);
 	mutex_lock(&root->fs_info->chunk_mutex);

@@ -405,11 +403,8 @@
 		goto out;
 	}
 
+	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
 	namelen = strlen(vol_args->name);
-	if (namelen > BTRFS_VOL_NAME_MAX) {
-		ret = -EINVAL;
-		goto out;
-	}
 	if (strchr(vol_args->name, '/')) {
 		ret = -EINVAL;
 		goto out;

@@ -480,6 +475,7 @@
 		ret = -EFAULT;
 		goto out;
 	}
+	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
 	ret = btrfs_init_new_device(root, vol_args->name);
 
 out:

@@ -501,6 +497,7 @@
 		ret = -EFAULT;
 		goto out;
 	}
+	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
 	ret = btrfs_rm_device(root, vol_args->name);
 
 out: