Re: [PATCH] hfs: Fix OOB Read in __hfs_brec_find

[Viacheslav Dubeyko]

> On Nov 29, 2022, at 10:59 PM, Peng Zhang <zhangpeng362@xxxxxxxxxx> wrote:
> 
> From: ZhangPeng <zhangpeng362@xxxxxxxxxx>
> 
> Syzbot reported a OOB read bug:
> 
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in hfs_strcmp+0x117/0x190
> fs/hfs/string.c:84
> Read of size 1 at addr ffff88807eb62c4e by task kworker/u4:1/11
> CPU: 1 PID: 11 Comm: kworker/u4:1 Not tainted
> 6.1.0-rc6-syzkaller-00308-g644e9524388a #0
> Workqueue: writeback wb_workfn (flush-7:0)
> Call Trace:
> <TASK>
> __dump_stack lib/dump_stack.c:88 [inline]
> dump_stack_lvl+0x1b1/0x28e lib/dump_stack.c:106
> print_address_description+0x74/0x340 mm/kasan/report.c:284
> print_report+0x107/0x1f0 mm/kasan/report.c:395
> kasan_report+0xcd/0x100 mm/kasan/report.c:495
> hfs_strcmp+0x117/0x190 fs/hfs/string.c:84
> __hfs_brec_find+0x213/0x5c0 fs/hfs/bfind.c:75
> hfs_brec_find+0x276/0x520 fs/hfs/bfind.c:138
> hfs_write_inode+0x34c/0xb40 fs/hfs/inode.c:462
> write_inode fs/fs-writeback.c:1440 [inline]
> 
> If the input inode of hfs_write_inode() is incorrect:
> struct inode
>  struct hfs_inode_info
>    struct hfs_cat_key
>      struct hfs_name
>        u8 len # len is greater than HFS_NAMELEN(31) which is the
> maximum length of an HFS filename
> 
> OOB read occurred:
> hfs_write_inode()
>  hfs_brec_find()
>    __hfs_brec_find()
>      hfs_cat_keycmp()
>        hfs_strcmp() # OOB read occurred due to len is too large
> 
> Fix this by adding a Check on len in hfs_write_inode() before calling
> hfs_brec_find().
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+e836ff7133ac02be825f@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: ZhangPeng <zhangpeng362@xxxxxxxxxx>
> ---
> fs/hfs/inode.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index c4526f16355d..a0746be3c1de 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -458,6 +458,8 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
> 		/* panic? */
> 		return -EIO;
> 
> +	if (HFS_I(main_inode)->cat_key.CName.len > HFS_NAMELEN)
> +		return -EIO;

If I understood correctly, we have corrupted struct hfs_cat_key instance. But what is the initial place
of this corruption? What function could introduce such corruption? Maybe, it needs to find a place(s)
where we can add some additional check and potentially exclude the incorrect input into
hfs_write_inode()?

I think it is not only place where it makes sense to check the correctness of struct hfs_cat_key
instance. Could we introduce a special function that check struct hfs_cat_key on corrupted
state and to use this function to check the state of the key in functions that operates by
keys?

Thanks,
Slava. 

> 	fd.search_key->cat = HFS_I(main_inode)->cat_key;
> 	if (hfs_brec_find(&fd))
> 		/* panic? */
> -- 
> 2.25.1
>