Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\))
Subject: Re: [PATCH] hfs: Fix OOB Read in __hfs_brec_find
From:   Viacheslav Dubeyko <slava@dubeyko.com>
In-Reply-To: <20221130065959.2168236-1-zhangpeng362@huawei.com>
Date:   Wed, 30 Nov 2022 11:23:56 -0800
Cc:     "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Damien Le Moal <damien.lemoal@opensource.wdc.com>,
        Jeff Layton <jlayton@kernel.org>,
        Ira Weiny <ira.weiny@intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux FS Devel <linux-fsdevel@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, sunnanyong@huawei.com,
        wangkefeng.wang@huawei.com,
        syzbot+e836ff7133ac02be825f@syzkaller.appspotmail.com
Content-Transfer-Encoding: quoted-printable
Message-Id: <AF1D5323-061B-4B7E-83E4-90BF1275DB30@dubeyko.com>
References: <20221130065959.2168236-1-zhangpeng362@huawei.com>
To:     Peng Zhang <zhangpeng362@huawei.com>
Precedence: bulk


> On Nov 29, 2022, at 10:59 PM, Peng Zhang <zhangpeng362@huawei.com> =
wrote:
>=20
> From: ZhangPeng <zhangpeng362@huawei.com>
>=20
> Syzbot reported a OOB read bug:
>=20
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> 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]
>=20
> 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
>=20
> 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
>=20
> Fix this by adding a Check on len in hfs_write_inode() before calling
> hfs_brec_find().
>=20
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+e836ff7133ac02be825f@syzkaller.appspotmail.com
> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> ---
> fs/hfs/inode.c | 2 ++
> 1 file changed, 2 insertions(+)
>=20
> 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;
>=20
> +	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.=20

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