| Message ID | 15308173-5252-d6a3-ae3b-e96d46cb6f41@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | fs: hfsplus: remove WARN_ON() from hfsplus_cat_{read,write}_inode() | expand |
> On Apr 11, 2023, at 3:57 AM, Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote: > > syzbot is hitting WARN_ON() in hfsplus_cat_{read,write}_inode(), for > crafted filesystem image can contain bogus length. There conditions are > not kernel bugs that can justify kernel to panic. > > Reported-by: syzbot <syzbot+e2787430e752a92b8750@syzkaller.appspotmail.com> > Link: https://syzkaller.appspot.com/bug?extid=e2787430e752a92b8750 > Reported-by: syzbot <syzbot+4913dca2ea6e4d43f3f1@syzkaller.appspotmail.com> > Link: https://syzkaller.appspot.com/bug?extid=4913dca2ea6e4d43f3f1 > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > fs/hfsplus/inode.c | 28 +++++++++++++++++++++++----- > 1 file changed, 23 insertions(+), 5 deletions(-) > > diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c > index abb91f5fae92..b21660475ac1 100644 > --- a/fs/hfsplus/inode.c > +++ b/fs/hfsplus/inode.c > @@ -511,7 +511,11 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd) > if (type == HFSPLUS_FOLDER) { > struct hfsplus_cat_folder *folder = &entry.folder; > > - WARN_ON(fd->entrylength < sizeof(struct hfsplus_cat_folder)); > + if (fd->entrylength < sizeof(struct hfsplus_cat_folder)) { > + pr_err("bad catalog folder entry\n"); > + res = -EIO; > + goto out; > + } > hfs_bnode_read(fd->bnode, &entry, fd->entryoffset, > sizeof(struct hfsplus_cat_folder)); > hfsplus_get_perms(inode, &folder->permissions, 1); > @@ -531,7 +535,11 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd) > } else if (type == HFSPLUS_FILE) { > struct hfsplus_cat_file *file = &entry.file; > > - WARN_ON(fd->entrylength < sizeof(struct hfsplus_cat_file)); > + if (fd->entrylength < sizeof(struct hfsplus_cat_file)) { > + pr_err("bad catalog file entry\n"); > + res = -EIO; > + goto out; > + } > hfs_bnode_read(fd->bnode, &entry, fd->entryoffset, > sizeof(struct hfsplus_cat_file)); > > @@ -562,6 +570,7 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd) > pr_err("bad catalog entry used to create inode\n"); > res = -EIO; > } > +out: > return res; > } > > @@ -570,6 +579,7 @@ int hfsplus_cat_write_inode(struct inode *inode) > struct inode *main_inode = inode; > struct hfs_find_data fd; > hfsplus_cat_entry entry; > + int res = 0; > > if (HFSPLUS_IS_RSRC(inode)) > main_inode = HFSPLUS_I(inode)->rsrc_inode; > @@ -588,7 +598,11 @@ int hfsplus_cat_write_inode(struct inode *inode) > if (S_ISDIR(main_inode->i_mode)) { > struct hfsplus_cat_folder *folder = &entry.folder; > > - WARN_ON(fd.entrylength < sizeof(struct hfsplus_cat_folder)); > + if (fd.entrylength < sizeof(struct hfsplus_cat_folder)) { > + pr_err("bad catalog folder entry\n"); > + res = -EIO; > + goto out; > + } > hfs_bnode_read(fd.bnode, &entry, fd.entryoffset, > sizeof(struct hfsplus_cat_folder)); > /* simple node checks? */ > @@ -613,7 +627,11 @@ int hfsplus_cat_write_inode(struct inode *inode) > } else { > struct hfsplus_cat_file *file = &entry.file; > > - WARN_ON(fd.entrylength < sizeof(struct hfsplus_cat_file)); > + if (fd.entrylength < sizeof(struct hfsplus_cat_file)) { > + pr_err("bad catalog file entry\n"); > + res = -EIO; > + goto out; > + } > hfs_bnode_read(fd.bnode, &entry, fd.entryoffset, > sizeof(struct hfsplus_cat_file)); > hfsplus_inode_write_fork(inode, &file->data_fork); > @@ -634,7 +652,7 @@ int hfsplus_cat_write_inode(struct inode *inode) > set_bit(HFSPLUS_I_CAT_DIRTY, &HFSPLUS_I(inode)->flags); > out: > hfs_find_exit(&fd); > - return 0; > + return res; > } > Looks reasonable to me. Maybe, WARN_ON() provided the opportunity to see the call stack of the issue. It could be useful for the issue’s environment analysis. But returning error from the function(s) makes the execution flow much better. Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com> Thanks, Slava. > int hfsplus_fileattr_get(struct dentry *dentry, struct fileattr *fa) > -- > 2.34.1 >
On Tue, 11 Apr 2023 19:57:33 +0900, Tetsuo Handa wrote: > syzbot is hitting WARN_ON() in hfsplus_cat_{read,write}_inode(), for > crafted filesystem image can contain bogus length. There conditions are > not kernel bugs that can justify kernel to panic. > > Seems fine as a follow-up fix to Arnd's in 55d1cbbbb29e ("hfs/hfsplus: use WARN_ON for sanity check"). Filesystems is orphaned so taking this through one of our vfs trees, branch: fs.misc [1/1] fs: hfsplus: remove WARN_ON() from hfsplus_cat_{read,write}_inode() commit: 81b21c0f0138ff5a499eafc3eb0578ad2a99622c
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c index abb91f5fae92..b21660475ac1 100644 --- a/fs/hfsplus/inode.c +++ b/fs/hfsplus/inode.c @@ -511,7 +511,11 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd) if (type == HFSPLUS_FOLDER) { struct hfsplus_cat_folder *folder = &entry.folder; - WARN_ON(fd->entrylength < sizeof(struct hfsplus_cat_folder)); + if (fd->entrylength < sizeof(struct hfsplus_cat_folder)) { + pr_err("bad catalog folder entry\n"); + res = -EIO; + goto out; + } hfs_bnode_read(fd->bnode, &entry, fd->entryoffset, sizeof(struct hfsplus_cat_folder)); hfsplus_get_perms(inode, &folder->permissions, 1); @@ -531,7 +535,11 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd) } else if (type == HFSPLUS_FILE) { struct hfsplus_cat_file *file = &entry.file; - WARN_ON(fd->entrylength < sizeof(struct hfsplus_cat_file)); + if (fd->entrylength < sizeof(struct hfsplus_cat_file)) { + pr_err("bad catalog file entry\n"); + res = -EIO; + goto out; + } hfs_bnode_read(fd->bnode, &entry, fd->entryoffset, sizeof(struct hfsplus_cat_file)); @@ -562,6 +570,7 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd) pr_err("bad catalog entry used to create inode\n"); res = -EIO; } +out: return res; } @@ -570,6 +579,7 @@ int hfsplus_cat_write_inode(struct inode *inode) struct inode *main_inode = inode; struct hfs_find_data fd; hfsplus_cat_entry entry; + int res = 0; if (HFSPLUS_IS_RSRC(inode)) main_inode = HFSPLUS_I(inode)->rsrc_inode; @@ -588,7 +598,11 @@ int hfsplus_cat_write_inode(struct inode *inode) if (S_ISDIR(main_inode->i_mode)) { struct hfsplus_cat_folder *folder = &entry.folder; - WARN_ON(fd.entrylength < sizeof(struct hfsplus_cat_folder)); + if (fd.entrylength < sizeof(struct hfsplus_cat_folder)) { + pr_err("bad catalog folder entry\n"); + res = -EIO; + goto out; + } hfs_bnode_read(fd.bnode, &entry, fd.entryoffset, sizeof(struct hfsplus_cat_folder)); /* simple node checks? */ @@ -613,7 +627,11 @@ int hfsplus_cat_write_inode(struct inode *inode) } else { struct hfsplus_cat_file *file = &entry.file; - WARN_ON(fd.entrylength < sizeof(struct hfsplus_cat_file)); + if (fd.entrylength < sizeof(struct hfsplus_cat_file)) { + pr_err("bad catalog file entry\n"); + res = -EIO; + goto out; + } hfs_bnode_read(fd.bnode, &entry, fd.entryoffset, sizeof(struct hfsplus_cat_file)); hfsplus_inode_write_fork(inode, &file->data_fork); @@ -634,7 +652,7 @@ int hfsplus_cat_write_inode(struct inode *inode) set_bit(HFSPLUS_I_CAT_DIRTY, &HFSPLUS_I(inode)->flags); out: hfs_find_exit(&fd); - return 0; + return res; } int hfsplus_fileattr_get(struct dentry *dentry, struct fileattr *fa)
syzbot is hitting WARN_ON() in hfsplus_cat_{read,write}_inode(), for crafted filesystem image can contain bogus length. There conditions are not kernel bugs that can justify kernel to panic. Reported-by: syzbot <syzbot+e2787430e752a92b8750@syzkaller.appspotmail.com> Link: https://syzkaller.appspot.com/bug?extid=e2787430e752a92b8750 Reported-by: syzbot <syzbot+4913dca2ea6e4d43f3f1@syzkaller.appspotmail.com> Link: https://syzkaller.appspot.com/bug?extid=4913dca2ea6e4d43f3f1 Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- fs/hfsplus/inode.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)