Re: [PATCH] Btrfs: avoid buffer overrun in mount option handling

[Jim Meyering]

Josef Bacik wrote:
> On Wed, Apr 25, 2012 at 09:51:53PM +0200, Jim Meyering wrote:
>> 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,

Hi Josef,

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

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index eca8dea..5ddf172 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -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=");