> On Apr 11, 2023, at 3:57 AM, Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Link: https://syzkaller.appspot.com/bug?extid=e2787430e752a92b8750
> Reported-by: syzbot <syzbot+4913dca2ea6e4d43f3f1@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Link: https://syzkaller.appspot.com/bug?extid=4913dca2ea6e4d43f3f1
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxx>
Thanks,
Slava.
> int hfsplus_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> --
> 2.34.1
>
