[Patch 0/4] Fix error handling in hfsplus mount failure paths

[Patch 0/4] Fix error handling in hfsplus mount failure paths

[Chuck Ebbert]

hfsplus can generate NULL pointer exceptions when a user attempts
to mount an invalid filesystem. In some paths it also leaks memory.

This series attempts to fix some of the most obvious problems.

[Patch 2/4] hfsplus: Skip cleanup on early mount failures

[Chuck Ebbert]

hfsplus: Skip cleanup on early mount failures

If we don't assign s_fs_info until later we can skip cleanup code
when handling very early failures.

Signed-Off-By: Chuck Ebbert <cebbert@redhat.com>

--- vanilla-2.6.38-rc2-git9.orig/fs/hfsplus/super.c
+++ vanilla-2.6.38-rc2-git9/fs/hfsplus/super.c
@@ -344,14 +344,13 @@ static int hfsplus_fill_super(struct sup
 	if (!sbi)
 		return -ENOMEM;
 
-	sb->s_fs_info = sbi;
 	mutex_init(&sbi->alloc_mutex);
 	mutex_init(&sbi->vh_mutex);
 	hfsplus_fill_defaults(sbi);
 	if (!hfsplus_parse_options(data, sbi)) {
 		printk(KERN_ERR "hfs: unable to parse mount options\n");
-		err = -EINVAL;
-		goto cleanup;
+		kfree(sbi);
+		return -EINVAL;
 	}
 
 	/* temporarily use utf8 to correctly find the hidden dir below */
@@ -359,10 +358,12 @@ static int hfsplus_fill_super(struct sup
 	sbi->nls = load_nls("utf8");
 	if (!sbi->nls) {
 		printk(KERN_ERR "hfs: unable to load nls for utf8\n");
-		err = -EINVAL;
-		goto cleanup;
+		kfree(sbi);
+		return -EINVAL;
 	}
 
+	sb->s_fs_info = sbi;
+
 	/* Grab the volume header */
 	if (hfsplus_read_wrapper(sb)) {
 		if (!silent)

[Patch 1/4] hfsplus: Don't leak buffer on error

[Chuck Ebbert]

hfsplus: Don't leak buffer on error

Signed-Off-By: Chuck Ebbert <cebbert@redhat.com>

--- vanilla-2.6.38-rc2-git9.orig/fs/hfsplus/part_tbl.c
+++ vanilla-2.6.38-rc2-git9/fs/hfsplus/part_tbl.c
@@ -134,7 +134,7 @@ int hfs_part_find(struct super_block *sb
 	res = hfsplus_submit_bio(sb->s_bdev, *part_start + HFS_PMAP_BLK,
 				 data, READ);
 	if (res)
-		return res;
+		goto out;
 
 	switch (be16_to_cpu(*((__be16 *)data))) {
 	case HFS_OLD_PMAP_MAGIC:
@@ -147,7 +147,7 @@ int hfs_part_find(struct super_block *sb
 		res = -ENOENT;
 		break;
 	}
-
+out:
 	kfree(data);
 	return res;
 }

Re: [Patch 1/4] hfsplus: Don't leak buffer on error

[Christoph Hellwig]

This one looks good though, I'll put it in.

On Tue, Feb 01, 2011 at 04:41:55PM -0500, Chuck Ebbert wrote:
> hfsplus: Don't leak buffer on error

That's already said in the subject, no need to repeat it in the mail
body.

[Patch 3/4] hfsplus: Clear volume header pointers on failure

[Chuck Ebbert]

hfsplus: Clear volume header pointers on failure

The next patch will use NULL volume header to determine whether
to flush the superblock. Also fix two failure cases so they
clear the headers before exiting.

Signed-Off-By: Chuck Ebbert <cebbert@redhat.com>

--- vanilla-2.6.38-rc2-git9.orig/fs/hfsplus/wrapper.c
+++ vanilla-2.6.38-rc2-git9/fs/hfsplus/wrapper.c
@@ -167,7 +167,7 @@ reread:
 		break;
 	case cpu_to_be16(HFSP_WRAP_MAGIC):
 		if (!hfsplus_read_mdb(sbi->s_vhdr, &wd))
-			goto out;
+			goto out_free_backup_vhdr;
 		wd.ablk_size >>= HFSPLUS_SECTOR_SHIFT;
 		part_start += wd.ablk_start + wd.embed_start * wd.ablk_size;
 		part_size = wd.embed_count * wd.ablk_size;
@@ -179,7 +179,7 @@ reread:
 		 * (should do this only for cdrom/loop though)
 		 */
 		if (hfs_part_find(sb, &part_start, &part_size))
-			goto out;
+			goto out_free_backup_vhdr;
 		goto reread;
 	}
 
@@ -230,8 +230,10 @@ reread:
 
 out_free_backup_vhdr:
 	kfree(sbi->s_backup_vhdr);
+	sbi->s_backup_vhdr = NULL;
 out_free_vhdr:
 	kfree(sbi->s_vhdr);
+	sbi->s_vhdr = NULL;
 out:
 	return error;
 }

Re: [Patch 3/4] hfsplus: Clear volume header pointers on failure

[Christoph Hellwig]

On Tue, Feb 01, 2011 at 04:45:07PM -0500, Chuck Ebbert wrote:
> hfsplus: Clear volume header pointers on failure
> 
> The next patch will use NULL volume header to determine whether
> to flush the superblock. Also fix two failure cases so they
> clear the headers before exiting.

Can you resend it without the NULLing out, which is not required
with proper error handling?

[Patch 4/4] hfsplus: Check for NULL volume header in put_super()

[Chuck Ebbert]

hfsplus: Check for NULL volume header in put_super()

If volume header is null there is not much to do in put_super().

Signed-Off-By: Chuck Ebbert <cebbert@redhat.com>

--- vanilla-2.6.38-rc2-git9.orig/fs/hfsplus/super.c
+++ vanilla-2.6.38-rc2-git9/fs/hfsplus/super.c
@@ -237,7 +237,10 @@ static void hfsplus_put_super(struct sup
 	if (!sb->s_fs_info)
 		return;
 
-	if (!(sb->s_flags & MS_RDONLY) && sbi->s_vhdr) {
+	if (!sbi->s_vhdr)
+		goto out_unload_nls;
+
+	if (!(sb->s_flags & MS_RDONLY)) {
 		struct hfsplus_vh *vhdr = sbi->s_vhdr;
 
 		vhdr->modify_date = hfsp_now2mt();
@@ -253,6 +256,7 @@ static void hfsplus_put_super(struct sup
 	iput(sbi->hidden_dir);
 	kfree(sbi->s_vhdr);
 	kfree(sbi->s_backup_vhdr);
+out_unload_nls:
 	unload_nls(sbi->nls);
 	kfree(sb->s_fs_info);
 	sb->s_fs_info = NULL;

Re: [Patch 0/4] Fix error handling in hfsplus mount failure paths

[Christoph Hellwig]

On Tue, Feb 01, 2011 at 04:28:02PM -0500, Chuck Ebbert wrote:
> hfsplus can generate NULL pointer exceptions when a user attempts
> to mount an invalid filesystem. In some paths it also leaks memory.
> 
> This series attempts to fix some of the most obvious problems.

Not in a very nice way, though.  The real problem here is that we

 a) can still return a failure after sb->s_root is set
 b) abuse hfsplus_put_super for error handling, which is totally
    ill-suited.

Take a look at the patch in
https://bugzilla.kernel.org/show_bug.cgi?id=27932 for a proper fix.