Skip to contentSkip to conversation

Fix error handling in hfsplus mount failure paths

PATCH In review Chuck Ebbert 2 participants 8 messages Feb 1, 2011 Hfsplus filesystem

lore
Share & tools
lore

Chuck Ebbert

author
#0 lore raw

cover Fix error handling in hfsplus mount failure paths

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.

author
#1 lore raw

PATCH 1/4 hfsplus: Don't leak buffer on error

hfsplus: Don't leak buffer on error

Signed-off-by Chuck Ebbert <[email protected]>
fs/hfsplus/part_tbl.c +2 -2
@@ -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;
 }
#2 lore raw

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

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

Quoted 1 line · Chuck Ebbert

hfsplus: Don't leak buffer on error

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

author
#3 lore raw

PATCH 2/4 hfsplus: Skip cleanup on early mount failures

In reply to 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 <[email protected]>
fs/hfsplus/super.c +6 -5
@@ -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)
author
#4 lore raw

PATCH 3/4 hfsplus: Clear volume header pointers on failure

In reply to 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 <[email protected]>
fs/hfsplus/wrapper.c +4 -2
@@ -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;
 }
#5 lore raw

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

Quoted 5 lines · 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.

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

author
#6 lore raw

PATCH 4/4 hfsplus: Check for NULL volume header in put_super()

In reply to 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 <[email protected]>
fs/hfsplus/super.c +5 -1
@@ -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;
#7 lore raw
In reply to Chuck Ebbert
Quoted 4 lines · 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.

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.