* [PATCH 1/2] hfsplus: clean up delayed work if fill_super fails
@ 2018-05-03 22:08 Ernesto A. Fernández
2018-05-03 22:10 ` [PATCH 2/2] hfsplus: always return error " Ernesto A. Fernández
2018-05-03 22:31 ` [PATCH 1/2] hfsplus: clean up delayed work " Al Viro
0 siblings, 2 replies; 5+ messages in thread
From: Ernesto A. Fernández @ 2018-05-03 22:08 UTC (permalink / raw)
To: syzbot
Cc: Andrew Morton, Christoph Hellwig, Alexey Khoroshilov,
Artem Bityutskiy, Ernesto A. Fernández, linux-fsdevel
If no hidden directory exists, the hfsplus_fill_super() function will
create it. A delayed work is then queued to sync the superblock, which
is never canceled in case of failure. Fix this.
Fixes: 9e6c5829b07c ("hfsplus: get rid of write_super")
Reported-by: syzbot+4f2e5f086147d543ab03@syzkaller.appspotmail.com
Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
fs/hfsplus/super.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 513c357c734b..4bc49e3f171d 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -588,6 +588,8 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
return 0;
out_put_hidden_dir:
+ /* Creating an inode queues the sb for synchronization */
+ cancel_delayed_work_sync(&sbi->sync_work);
iput(sbi->hidden_dir);
out_put_root:
dput(sb->s_root);
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/2] hfsplus: always return error if fill_super fails 2018-05-03 22:08 [PATCH 1/2] hfsplus: clean up delayed work if fill_super fails Ernesto A. Fernández @ 2018-05-03 22:10 ` Ernesto A. Fernández 2018-05-03 22:31 ` [PATCH 1/2] hfsplus: clean up delayed work " Al Viro 1 sibling, 0 replies; 5+ messages in thread From: Ernesto A. Fernández @ 2018-05-03 22:10 UTC (permalink / raw) To: syzbot Cc: Andrew Morton, Christoph Hellwig, Alexey Khoroshilov, Artem Bityutskiy, Ernesto A. Fernández, linux-fsdevel If the filesystem is corrupted in such a way that the HFS+ Private Data directory is not of type HFSPLUS_FOLDER, the mount will fail without throwing an error code. The vfs layer is then forced to dereference a NULL root dentry. This bug was found by KASAN while running the reproducer provided by syzbot+4f2e5f086147d543ab03@syzkaller.appspotmail.com for a separate issue. Fixes: 5bd9d99d107c ("hfsplus: add error checking for hfs_find_init()") Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> --- fs/hfsplus/super.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c index 4bc49e3f171d..4f62634c5666 100644 --- a/fs/hfsplus/super.c +++ b/fs/hfsplus/super.c @@ -524,8 +524,10 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) goto out_put_root; if (!hfs_brec_read(&fd, &entry, sizeof(entry))) { hfs_find_exit(&fd); - if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) + if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) { + err = -EINVAL; goto out_put_root; + } inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id)); if (IS_ERR(inode)) { err = PTR_ERR(inode); -- 2.11.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] hfsplus: clean up delayed work if fill_super fails 2018-05-03 22:08 [PATCH 1/2] hfsplus: clean up delayed work if fill_super fails Ernesto A. Fernández 2018-05-03 22:10 ` [PATCH 2/2] hfsplus: always return error " Ernesto A. Fernández @ 2018-05-03 22:31 ` Al Viro 2018-05-08 1:48 ` Ernesto A. Fernández 1 sibling, 1 reply; 5+ messages in thread From: Al Viro @ 2018-05-03 22:31 UTC (permalink / raw) To: Ernesto A. Fernández Cc: syzbot, Andrew Morton, Christoph Hellwig, Alexey Khoroshilov, Artem Bityutskiy, linux-fsdevel On Thu, May 03, 2018 at 07:08:22PM -0300, Ernesto A. Fern�ndez wrote: > If no hidden directory exists, the hfsplus_fill_super() function will > create it. A delayed work is then queued to sync the superblock, which > is never canceled in case of failure. Fix this. Wouldn't it be simpler to avoid all the crap with clearing ->s_root on failure, letting ->put_super() take care of everything? Or, better yet, take cleanups into ->kill_sb(), which is always called on superblock shutdown, ->s_root or no ->s_root... ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] hfsplus: clean up delayed work if fill_super fails 2018-05-03 22:31 ` [PATCH 1/2] hfsplus: clean up delayed work " Al Viro @ 2018-05-08 1:48 ` Ernesto A. Fernández 2018-05-21 2:06 ` [PATCH v3] hfsplus: fix cleanup for hfsplus_fill_super() Ernesto A. Fernández 0 siblings, 1 reply; 5+ messages in thread From: Ernesto A. Fernández @ 2018-05-08 1:48 UTC (permalink / raw) To: Al Viro Cc: syzbot, Andrew Morton, Christoph Hellwig, Alexey Khoroshilov, Artem Bityutskiy, linux-fsdevel, Ernesto A. Fernández On Thu, May 03, 2018 at 11:31:15PM +0100, Al Viro wrote: > On Thu, May 03, 2018 at 07:08:22PM -0300, Ernesto A. Fernández wrote: > > If no hidden directory exists, the hfsplus_fill_super() function will > > create it. A delayed work is then queued to sync the superblock, which > > is never canceled in case of failure. Fix this. > > Wouldn't it be simpler to avoid all the crap with clearing ->s_root > on failure, letting ->put_super() take care of everything? Or, better > yet, take cleanups into ->kill_sb(), which is always called on > superblock shutdown, ->s_root or no ->s_root... If you mean that I move all cleanups from fill_super() to ->kill_sb(), that sounds like a lot of trouble. Is this common in other filesystems? As for letting ->put_super() do the cleanup, the problem is that it will flag the volume as consistent, even if the directory count was increased and the hidden dir was not added to the catalog. So we should correct that before returning from fill_super(). I wouldn't call this simpler, but I guess it's a bit better for consistency. -- >8 -- Subject: [PATCH] hfsplus: fix cleanup for hfsplus_fill_super() If no hidden directory exists, the hfsplus_fill_super() function will create it. A delayed work is then queued to sync the superblock, which is never canceled in case of failure. Also, if the filesystem is corrupted in such a way that the hidden directory is not of type HFSPLUS_FOLDER, the mount will fail without throwing an error code. The vfs layer is then forced to dereference a NULL root dentry. To fix these issues, return an error and allow ->put_super() to take care of most of the cleanup if failure occurs after sb->s_root has been set. Before this patch the volume was simply flagged as inconsistent if the mount failed while creating the hidden directory. Since ->put_super() will now toggle those flags, be sure to correct the folder count before returning, with a call to hfsplus_delete_inode(). Reported-by: syzbot+4f2e5f086147d543ab03@syzkaller.appspotmail.com Fixes: 5bd9d99d107c ("hfsplus: add error checking for hfs_find_init()") Fixes: 9e6c5829b07c ("hfsplus: get rid of write_super") Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> --- fs/hfsplus/super.c | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c index 513c357c734b..cd39f8d34241 100644 --- a/fs/hfsplus/super.c +++ b/fs/hfsplus/super.c @@ -514,22 +514,26 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) goto out_put_alloc_file; } + /* From here on, most of the cleanup is handled by ->put_super */ + str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1; str.name = HFSP_HIDDENDIR_NAME; err = hfs_find_init(sbi->cat_tree, &fd); if (err) - goto out_put_root; + goto out; err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str); if (unlikely(err < 0)) - goto out_put_root; + goto out; if (!hfs_brec_read(&fd, &entry, sizeof(entry))) { hfs_find_exit(&fd); - if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) - goto out_put_root; + if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) { + err = -EINVAL; + goto out; + } inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id)); if (IS_ERR(inode)) { err = PTR_ERR(inode); - goto out_put_root; + goto out; } sbi->hidden_dir = inode; } else @@ -551,16 +555,13 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) mutex_lock(&sbi->vh_mutex); sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR); if (!sbi->hidden_dir) { - mutex_unlock(&sbi->vh_mutex); err = -ENOMEM; - goto out_put_root; + goto out_unlock_vh_mutex; } err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root, &str, sbi->hidden_dir); - if (err) { - mutex_unlock(&sbi->vh_mutex); - goto out_put_hidden_dir; - } + if (err) + goto out_delete_hidden_dir; err = hfsplus_init_inode_security(sbi->hidden_dir, root, &str); @@ -573,8 +574,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) */ hfsplus_delete_cat(sbi->hidden_dir->i_ino, root, &str); - mutex_unlock(&sbi->vh_mutex); - goto out_put_hidden_dir; + goto out_delete_hidden_dir; } mutex_unlock(&sbi->vh_mutex); @@ -587,11 +587,13 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) sbi->nls = nls; return 0; -out_put_hidden_dir: - iput(sbi->hidden_dir); -out_put_root: - dput(sb->s_root); - sb->s_root = NULL; +out_delete_hidden_dir: + clear_nlink(inode); + hfsplus_delete_inode(inode); +out_unlock_vh_mutex: + mutex_unlock(&sbi->vh_mutex); + goto out; + out_put_alloc_file: iput(sbi->alloc_file); out_close_attr_tree: @@ -605,9 +607,9 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) kfree(sbi->s_backup_vhdr_buf); out_unload_nls: unload_nls(sbi->nls); - unload_nls(nls); kfree(sbi); out: + unload_nls(nls); return err; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v3] hfsplus: fix cleanup for hfsplus_fill_super() 2018-05-08 1:48 ` Ernesto A. Fernández @ 2018-05-21 2:06 ` Ernesto A. Fernández 0 siblings, 0 replies; 5+ messages in thread From: Ernesto A. Fernández @ 2018-05-21 2:06 UTC (permalink / raw) To: Al Viro Cc: syzbot, Andrew Morton, Christoph Hellwig, Alexey Khoroshilov, Artem Bityutskiy, linux-fsdevel, Ernesto A. Fernández, Tetsuo Handa If no hidden directory exists, the hfsplus_fill_super() function will create it. A delayed work is then queued to sync the superblock, which is never canceled in case of failure. Also, if the filesystem is corrupted in such a way that the hidden directory is not of type HFSPLUS_FOLDER, the mount will fail without throwing an error code. The vfs layer is then forced to dereference a NULL root dentry. To fix these issues, return an error and allow ->put_super() to take care of most of the cleanup if failure occurs after sb->s_root has been set. Before this patch the volume was simply flagged as inconsistent if the mount failed while creating the hidden directory. Since ->put_super() will now toggle those flags, we need to correct the folder count before returning, with a call to hfsplus_delete_inode(). Simplify this by using a new hfsplus_create_inode() function that can handle its own cleanup. Reported-by: syzbot+4f2e5f086147d543ab03@syzkaller.appspotmail.com Fixes: 5bd9d99d107c ("hfsplus: add error checking for hfs_find_init()") Fixes: 9e6c5829b07c ("hfsplus: get rid of write_super") Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> --- Changes in v3: Move the code that creates the hidden dir into a separate function. I think this is more reasonable. The new function can also be called by hfsplus_mknod(). fs/hfsplus/dir.c | 28 +++----------------------- fs/hfsplus/hfsplus_fs.h | 2 ++ fs/hfsplus/inode.c | 32 +++++++++++++++++++++++++++++ fs/hfsplus/super.c | 53 +++++++++++++------------------------------------ 4 files changed, 51 insertions(+), 64 deletions(-) diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c index 15e06fb552da..2446e5db1dfb 100644 --- a/fs/hfsplus/dir.c +++ b/fs/hfsplus/dir.c @@ -483,37 +483,15 @@ static int hfsplus_mknod(struct inode *dir, struct dentry *dentry, { struct hfsplus_sb_info *sbi = HFSPLUS_SB(dir->i_sb); struct inode *inode; - int res = -ENOMEM; + int res; mutex_lock(&sbi->vh_mutex); - inode = hfsplus_new_inode(dir->i_sb, dir, mode); - if (!inode) - goto out; - - if (S_ISBLK(mode) || S_ISCHR(mode) || S_ISFIFO(mode) || S_ISSOCK(mode)) - init_special_inode(inode, mode, rdev); - - res = hfsplus_create_cat(inode->i_ino, dir, &dentry->d_name, inode); + res = hfsplus_create_inode(dir, &dentry->d_name, mode, rdev, &inode); if (res) - goto failed_mknod; - - res = hfsplus_init_inode_security(inode, dir, &dentry->d_name); - if (res == -EOPNOTSUPP) - res = 0; /* Operation is not supported. */ - else if (res) { - /* Try to delete anyway without error analysis. */ - hfsplus_delete_cat(inode->i_ino, dir, &dentry->d_name); - goto failed_mknod; - } - + goto out; hfsplus_instantiate(dentry, inode, inode->i_ino); mark_inode_dirty(inode); - goto out; -failed_mknod: - clear_nlink(inode); - hfsplus_delete_inode(inode); - iput(inode); out: mutex_unlock(&sbi->vh_mutex); return res; diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h index d9255abafb81..05f30ae788f0 100644 --- a/fs/hfsplus/hfsplus_fs.h +++ b/fs/hfsplus/hfsplus_fs.h @@ -480,6 +480,8 @@ extern const struct dentry_operations hfsplus_dentry_operations; struct inode *hfsplus_new_inode(struct super_block *sb, struct inode *dir, umode_t mode); +int hfsplus_create_inode(struct inode *dir, struct qstr *name, umode_t mode, + dev_t rdev, struct inode **inode); void hfsplus_delete_inode(struct inode *inode); void hfsplus_inode_read_fork(struct inode *inode, struct hfsplus_fork_raw *fork); diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c index c0c8d433864f..b15bb83deda1 100644 --- a/fs/hfsplus/inode.c +++ b/fs/hfsplus/inode.c @@ -413,6 +413,38 @@ struct inode *hfsplus_new_inode(struct super_block *sb, struct inode *dir, return inode; } +int hfsplus_create_inode(struct inode *dir, struct qstr *name, umode_t mode, + dev_t rdev, struct inode **inode) +{ + int res; + + *inode = hfsplus_new_inode(dir->i_sb, dir, mode); + if (!*inode) + return -ENOMEM; + + if (S_ISBLK(mode) || S_ISCHR(mode) || S_ISFIFO(mode) || S_ISSOCK(mode)) + init_special_inode(*inode, mode, rdev); + + res = hfsplus_create_cat((*inode)->i_ino, dir, name, *inode); + if (res) + goto fail; + + res = hfsplus_init_inode_security(*inode, dir, name); + if (res && res != -EOPNOTSUPP) { + /* Try to delete anyway without error analysis. */ + hfsplus_delete_cat((*inode)->i_ino, dir, name); + goto fail; + } + return 0; + +fail: + clear_nlink(*inode); + hfsplus_delete_inode(*inode); + iput(*inode); + *inode = NULL; + return res; +} + void hfsplus_delete_inode(struct inode *inode) { struct super_block *sb = inode->i_sb; diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c index 513c357c734b..3749d9381c56 100644 --- a/fs/hfsplus/super.c +++ b/fs/hfsplus/super.c @@ -514,22 +514,26 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) goto out_put_alloc_file; } + /* From here on, most of the cleanup is handled by ->put_super() */ + str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1; str.name = HFSP_HIDDENDIR_NAME; err = hfs_find_init(sbi->cat_tree, &fd); if (err) - goto out_put_root; + goto out; err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str); if (unlikely(err < 0)) - goto out_put_root; + goto out; if (!hfs_brec_read(&fd, &entry, sizeof(entry))) { hfs_find_exit(&fd); - if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) - goto out_put_root; + if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) { + err = -EINVAL; + goto out; + } inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id)); if (IS_ERR(inode)) { err = PTR_ERR(inode); - goto out_put_root; + goto out; } sbi->hidden_dir = inode; } else @@ -549,35 +553,11 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) if (!sbi->hidden_dir) { mutex_lock(&sbi->vh_mutex); - sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR); - if (!sbi->hidden_dir) { - mutex_unlock(&sbi->vh_mutex); - err = -ENOMEM; - goto out_put_root; - } - err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root, - &str, sbi->hidden_dir); - if (err) { - mutex_unlock(&sbi->vh_mutex); - goto out_put_hidden_dir; - } - - err = hfsplus_init_inode_security(sbi->hidden_dir, - root, &str); - if (err == -EOPNOTSUPP) - err = 0; /* Operation is not supported. */ - else if (err) { - /* - * Try to delete anyway without - * error analysis. - */ - hfsplus_delete_cat(sbi->hidden_dir->i_ino, - root, &str); - mutex_unlock(&sbi->vh_mutex); - goto out_put_hidden_dir; - } - + err = hfsplus_create_inode(root, &str, S_IFDIR, 0, + &sbi->hidden_dir); mutex_unlock(&sbi->vh_mutex); + if (err) + goto out; hfsplus_mark_inode_dirty(sbi->hidden_dir, HFSPLUS_I_CAT_DIRTY); } @@ -587,11 +567,6 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) sbi->nls = nls; return 0; -out_put_hidden_dir: - iput(sbi->hidden_dir); -out_put_root: - dput(sb->s_root); - sb->s_root = NULL; out_put_alloc_file: iput(sbi->alloc_file); out_close_attr_tree: @@ -605,9 +580,9 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) kfree(sbi->s_backup_vhdr_buf); out_unload_nls: unload_nls(sbi->nls); - unload_nls(nls); kfree(sbi); out: + unload_nls(nls); return err; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-05-21 2:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-03 22:08 [PATCH 1/2] hfsplus: clean up delayed work if fill_super fails Ernesto A. Fernández 2018-05-03 22:10 ` [PATCH 2/2] hfsplus: always return error " Ernesto A. Fernández 2018-05-03 22:31 ` [PATCH 1/2] hfsplus: clean up delayed work " Al Viro 2018-05-08 1:48 ` Ernesto A. Fernández 2018-05-21 2:06 ` [PATCH v3] hfsplus: fix cleanup for hfsplus_fill_super() Ernesto A. Fernández
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).