[PATCH 0/3] hfs: fix various errors

[PATCH 0/3] hfs: fix various errors

[Desmond Cheong Zhi Xi]

Hi,

This series ultimately aims to address a lockdep warning in hfs_find_init reported by Syzbot:
https://syzkaller.appspot.com/bug?id=f007ef1d7a31a469e3be7aeb0fde0769b18585db

The work done for this led to the discovery of another bug, and the Syzkaller repro test also reveals an invalid memory access error after clearing the lockdep warning. Hence, this series is broken up into three patches:

1. Add a missing call to hfs_find_exit an error path in hfs_fill_super

2. Fix memory mapping in hfs_bnode_read by fixing calls to kmap

3. Add lock nesting notation to tell lockdep that the observed locking hierarchy is safe

Desmond Cheong Zhi Xi (3):
  hfs: add missing clean-up in hfs_fill_super
  hfs: fix high memory mapping in hfs_bnode_read
  hfs: add lock nesting notation to hfs_find_init

 fs/hfs/bfind.c | 14 +++++++++++++-
 fs/hfs/bnode.c | 18 ++++++++++++++----
 fs/hfs/btree.h |  7 +++++++
 fs/hfs/super.c |  1 +
 4 files changed, 35 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/3] hfs: add missing clean-up in hfs_fill_super

[Desmond Cheong Zhi Xi]

On exiting hfs_fill_super, the file descriptor used in hfs_find_init
should be passed to hfs_find_exit to be cleaned up, and to release the
lock held on the btree.

The call to hfs_find_exit is missing from this error path, so we add
it in to release resources.

Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 fs/hfs/super.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 44d07c9e3a7f..48340b77eb36 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -419,6 +419,7 @@ static int hfs_fill_super(struct super_block *sb, void *data, int silent)
 	res = hfs_cat_find_brec(sb, HFS_ROOT_CNID, &fd);
 	if (!res) {
 		if (fd.entrylength > sizeof(rec) || fd.entrylength < 0) {
+			hfs_find_exit(&fd);
 			res =  -EIO;
 			goto bail;
 		}
-- 
2.25.1

[PATCH 2/3] hfs: fix high memory mapping in hfs_bnode_read

[Desmond Cheong Zhi Xi]

Pages that we read in hfs_bnode_read need to be kmapped into kernel
address space. However, currently only the 0th page is kmapped. If the
given offset + length exceeds this 0th page, then we have an invalid
memory access.

To fix this, we use the same logic used  in hfsplus' version of
hfs_bnode_read to kmap each page of relevant data that we copy.

An example of invalid memory access occurring without this fix can be
seen in the following crash report:

==================================================================
BUG: KASAN: use-after-free in memcpy include/linux/fortify-string.h:191 [inline]
BUG: KASAN: use-after-free in hfs_bnode_read+0xc4/0xe0 fs/hfs/bnode.c:26
Read of size 2 at addr ffff888125fdcffe by task syz-executor5/4634

CPU: 0 PID: 4634 Comm: syz-executor5 Not tainted 5.13.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:79 [inline]
 dump_stack+0x195/0x1f8 lib/dump_stack.c:120
 print_address_description.constprop.0+0x1d/0x110 mm/kasan/report.c:233
 __kasan_report mm/kasan/report.c:419 [inline]
 kasan_report.cold+0x7b/0xd4 mm/kasan/report.c:436
 check_region_inline mm/kasan/generic.c:180 [inline]
 kasan_check_range+0x154/0x1b0 mm/kasan/generic.c:186
 memcpy+0x24/0x60 mm/kasan/shadow.c:65
 memcpy include/linux/fortify-string.h:191 [inline]
 hfs_bnode_read+0xc4/0xe0 fs/hfs/bnode.c:26
 hfs_bnode_read_u16 fs/hfs/bnode.c:34 [inline]
 hfs_bnode_find+0x880/0xcc0 fs/hfs/bnode.c:365
 hfs_brec_find+0x2d8/0x540 fs/hfs/bfind.c:126
 hfs_brec_read+0x27/0x120 fs/hfs/bfind.c:165
 hfs_cat_find_brec+0x19a/0x3b0 fs/hfs/catalog.c:194
 hfs_fill_super+0xc13/0x1460 fs/hfs/super.c:419
 mount_bdev+0x331/0x3f0 fs/super.c:1368
 hfs_mount+0x35/0x40 fs/hfs/super.c:457
 legacy_get_tree+0x10c/0x220 fs/fs_context.c:592
 vfs_get_tree+0x93/0x300 fs/super.c:1498
 do_new_mount fs/namespace.c:2905 [inline]
 path_mount+0x13f5/0x20e0 fs/namespace.c:3235
 do_mount fs/namespace.c:3248 [inline]
 __do_sys_mount fs/namespace.c:3456 [inline]
 __se_sys_mount fs/namespace.c:3433 [inline]
 __x64_sys_mount+0x2b8/0x340 fs/namespace.c:3433
 do_syscall_64+0x37/0xc0 arch/x86/entry/common.c:47
 entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x45e63a
Code: 48 c7 c2 bc ff ff ff f7 d8 64 89 02 b8 ff ff ff ff eb d2 e8 88 04 00 00 0f 1f 84 00 00 00 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f9404d410d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 0000000020000248 RCX: 000000000045e63a
RDX: 0000000020000000 RSI: 0000000020000100 RDI: 00007f9404d41120
RBP: 00007f9404d41120 R08: 00000000200002c0 R09: 0000000020000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000003
R13: 0000000000000003 R14: 00000000004ad5d8 R15: 0000000000000000

The buggy address belongs to the page:
page:00000000dadbcf3e refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x125fdc
flags: 0x2fffc0000000000(node=0|zone=2|lastcpupid=0x3fff)
raw: 02fffc0000000000 ffffea000497f748 ffffea000497f6c8 0000000000000000
raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff888125fdce80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
 ffff888125fdcf00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>ffff888125fdcf80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
                                                                ^
 ffff888125fdd000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
 ffff888125fdd080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
==================================================================

Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 fs/hfs/bnode.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
index b63a4df7327b..936cfa763224 100644
--- a/fs/hfs/bnode.c
+++ b/fs/hfs/bnode.c
@@ -18,13 +18,23 @@
 void hfs_bnode_read(struct hfs_bnode *node, void *buf,
 		int off, int len)
 {
-	struct page *page;
+	struct page **pagep;
+	int l;
 
 	off += node->page_offset;
-	page = node->page[0];
+	pagep = node->page + (off >> PAGE_SHIFT);
+	off &= ~PAGE_MASK;
 
-	memcpy(buf, kmap(page) + off, len);
-	kunmap(page);
+	l = min_t(int, len, PAGE_SIZE - off);
+	memcpy(buf, kmap(*pagep) + off, l);
+	kunmap(*pagep);
+
+	while ((len -= l) != 0) {
+		buf += l;
+		l = min_t(int, len, PAGE_SIZE);
+		memcpy(buf, kmap(*++pagep), l);
+		kunmap(*pagep);
+	}
 }
 
 u16 hfs_bnode_read_u16(struct hfs_bnode *node, int off)
-- 
2.25.1

[PATCH 3/3] hfs: add lock nesting notation to hfs_find_init

[Desmond Cheong Zhi Xi]

Syzbot reports a possible recursive lock:
https://syzkaller.appspot.com/bug?id=f007ef1d7a31a469e3be7aeb0fde0769b18585db

This happens due to missing lock nesting information. From the logs,
we see that a call to hfs_fill_super is made to mount the hfs
filesystem. While searching for the root inode, the lock on the
catalog btree is grabbed. Then, when the parent of the root isn't
found, a call to __hfs_bnode_create is made to create the parent of
the root. This eventually leads to a call to hfs_ext_read_extent which
grabs a lock on the extents btree.

Since the order of locking is catalog btree -> extents btree, this
lock hierarchy does not lead to a deadlock.

To tell lockdep that this locking is safe, we add nesting notation to
distinguish between catalog btrees, extents btrees, and attributes
btrees (for HFS+). This has already been done in hfsplus.

Reported-and-tested-by: syzbot+b718ec84a87b7e73ade4@syzkaller.appspotmail.com
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 fs/hfs/bfind.c | 14 +++++++++++++-
 fs/hfs/btree.h |  7 +++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c
index 4af318fbda77..ef9498a6e88a 100644
--- a/fs/hfs/bfind.c
+++ b/fs/hfs/bfind.c
@@ -25,7 +25,19 @@ int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd)
 	fd->key = ptr + tree->max_key_len + 2;
 	hfs_dbg(BNODE_REFS, "find_init: %d (%p)\n",
 		tree->cnid, __builtin_return_address(0));
-	mutex_lock(&tree->tree_lock);
+	switch (tree->cnid) {
+	case HFS_CAT_CNID:
+		mutex_lock_nested(&tree->tree_lock, CATALOG_BTREE_MUTEX);
+		break;
+	case HFS_EXT_CNID:
+		mutex_lock_nested(&tree->tree_lock, EXTENTS_BTREE_MUTEX);
+		break;
+	case HFS_ATTR_CNID:
+		mutex_lock_nested(&tree->tree_lock, ATTR_BTREE_MUTEX);
+		break;
+	default:
+		return -EINVAL;
+	}
 	return 0;
 }
 
diff --git a/fs/hfs/btree.h b/fs/hfs/btree.h
index 4ba45caf5939..0e6baee93245 100644
--- a/fs/hfs/btree.h
+++ b/fs/hfs/btree.h
@@ -13,6 +13,13 @@ typedef int (*btree_keycmp)(const btree_key *, const btree_key *);
 
 #define NODE_HASH_SIZE  256
 
+/* B-tree mutex nested subclasses */
+enum hfs_btree_mutex_classes {
+	CATALOG_BTREE_MUTEX,
+	EXTENTS_BTREE_MUTEX,
+	ATTR_BTREE_MUTEX,
+};
+
 /* A HFS BTree held in memory */
 struct hfs_btree {
 	struct super_block *sb;
-- 
2.25.1

Re: [PATCH 1/3] hfs: add missing clean-up in hfs_fill_super

[Viacheslav Dubeyko]

> On Jun 29, 2021, at 7:48 AM, Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> wrote:
> 
> On exiting hfs_fill_super, the file descriptor used in hfs_find_init
> should be passed to hfs_find_exit to be cleaned up, and to release the
> lock held on the btree.
> 
> The call to hfs_find_exit is missing from this error path, so we add
> it in to release resources.
> 
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> ---
> fs/hfs/super.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index 44d07c9e3a7f..48340b77eb36 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -419,6 +419,7 @@ static int hfs_fill_super(struct super_block *sb, void *data, int silent)
> 	res = hfs_cat_find_brec(sb, HFS_ROOT_CNID, &fd);
> 	if (!res) {
> 		if (fd.entrylength > sizeof(rec) || fd.entrylength < 0) {
> +			hfs_find_exit(&fd);

I see that there are several places of hfs_find_exit() calls in hfs_fill_super(). Maybe, it makes sense to move the hfs_find_exit() call to the end of the hfs_fill_super()? In this case we could process this activity of resources freeing into one place. I mean line 449 in the source code (failure case).

Thanks,
Slava.

> 			res =  -EIO;
> 			goto bail;
> 		}
> -- 
> 2.25.1
> 

Re: [PATCH 2/3] hfs: fix high memory mapping in hfs_bnode_read

[Viacheslav Dubeyko]

> On Jun 29, 2021, at 7:48 AM, Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> wrote:
> 
> Pages that we read in hfs_bnode_read need to be kmapped into kernel
> address space. However, currently only the 0th page is kmapped. If the
> given offset + length exceeds this 0th page, then we have an invalid
> memory access.
> 
> To fix this, we use the same logic used  in hfsplus' version of
> hfs_bnode_read to kmap each page of relevant data that we copy.
> 
> An example of invalid memory access occurring without this fix can be
> seen in the following crash report:
> 
> ==================================================================
> BUG: KASAN: use-after-free in memcpy include/linux/fortify-string.h:191 [inline]
> BUG: KASAN: use-after-free in hfs_bnode_read+0xc4/0xe0 fs/hfs/bnode.c:26
> Read of size 2 at addr ffff888125fdcffe by task syz-executor5/4634
> 
> CPU: 0 PID: 4634 Comm: syz-executor5 Not tainted 5.13.0-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:79 [inline]
> dump_stack+0x195/0x1f8 lib/dump_stack.c:120
> print_address_description.constprop.0+0x1d/0x110 mm/kasan/report.c:233
> __kasan_report mm/kasan/report.c:419 [inline]
> kasan_report.cold+0x7b/0xd4 mm/kasan/report.c:436
> check_region_inline mm/kasan/generic.c:180 [inline]
> kasan_check_range+0x154/0x1b0 mm/kasan/generic.c:186
> memcpy+0x24/0x60 mm/kasan/shadow.c:65
> memcpy include/linux/fortify-string.h:191 [inline]
> hfs_bnode_read+0xc4/0xe0 fs/hfs/bnode.c:26
> hfs_bnode_read_u16 fs/hfs/bnode.c:34 [inline]
> hfs_bnode_find+0x880/0xcc0 fs/hfs/bnode.c:365
> hfs_brec_find+0x2d8/0x540 fs/hfs/bfind.c:126
> hfs_brec_read+0x27/0x120 fs/hfs/bfind.c:165
> hfs_cat_find_brec+0x19a/0x3b0 fs/hfs/catalog.c:194
> hfs_fill_super+0xc13/0x1460 fs/hfs/super.c:419
> mount_bdev+0x331/0x3f0 fs/super.c:1368
> hfs_mount+0x35/0x40 fs/hfs/super.c:457
> legacy_get_tree+0x10c/0x220 fs/fs_context.c:592
> vfs_get_tree+0x93/0x300 fs/super.c:1498
> do_new_mount fs/namespace.c:2905 [inline]
> path_mount+0x13f5/0x20e0 fs/namespace.c:3235
> do_mount fs/namespace.c:3248 [inline]
> __do_sys_mount fs/namespace.c:3456 [inline]
> __se_sys_mount fs/namespace.c:3433 [inline]
> __x64_sys_mount+0x2b8/0x340 fs/namespace.c:3433
> do_syscall_64+0x37/0xc0 arch/x86/entry/common.c:47
> entry_SYSCALL_64_after_hwframe+0x44/0xae
> RIP: 0033:0x45e63a
> Code: 48 c7 c2 bc ff ff ff f7 d8 64 89 02 b8 ff ff ff ff eb d2 e8 88 04 00 00 0f 1f 84 00 00 00 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007f9404d410d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
> RAX: ffffffffffffffda RBX: 0000000020000248 RCX: 000000000045e63a
> RDX: 0000000020000000 RSI: 0000000020000100 RDI: 00007f9404d41120
> RBP: 00007f9404d41120 R08: 00000000200002c0 R09: 0000000020000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000003
> R13: 0000000000000003 R14: 00000000004ad5d8 R15: 0000000000000000
> 
> The buggy address belongs to the page:
> page:00000000dadbcf3e refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x125fdc
> flags: 0x2fffc0000000000(node=0|zone=2|lastcpupid=0x3fff)
> raw: 02fffc0000000000 ffffea000497f748 ffffea000497f6c8 0000000000000000
> raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
> 
> Memory state around the buggy address:
> ffff888125fdce80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ffff888125fdcf00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>> ffff888125fdcf80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>                                                                ^
> ffff888125fdd000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ffff888125fdd080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ==================================================================
> 
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> ---
> fs/hfs/bnode.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
> index b63a4df7327b..936cfa763224 100644
> --- a/fs/hfs/bnode.c
> +++ b/fs/hfs/bnode.c
> @@ -18,13 +18,23 @@
> void hfs_bnode_read(struct hfs_bnode *node, void *buf,
> 		int off, int len)
> {
> -	struct page *page;
> +	struct page **pagep;
> +	int l;
> 
> 	off += node->page_offset;
> -	page = node->page[0];
> +	pagep = node->page + (off >> PAGE_SHIFT);

I would like to have a check here that we are not out of the page array. Could you add this check?

Also, maybe, node->page[index] could look much safer here. What do you think?

> +	off &= ~PAGE_MASK;
> 
> -	memcpy(buf, kmap(page) + off, len);
> -	kunmap(page);
> +	l = min_t(int, len, PAGE_SIZE - off);

Maybe, it makes sense to use more informative name of the variable instead of “l”?

> +	memcpy(buf, kmap(*pagep) + off, l);

I suppose that it could be good to have a check that we do not overflow the buffer. How do you feel about it?

> +	kunmap(*pagep);

What’s about kmap_atomic/kunmap_atomic in this function?

Thanks,
Slava.

> +
> +	while ((len -= l) != 0) {
> +		buf += l;
> +		l = min_t(int, len, PAGE_SIZE);
> +		memcpy(buf, kmap(*++pagep), l);
> +		kunmap(*pagep);
> +	}
> }
> 
> u16 hfs_bnode_read_u16(struct hfs_bnode *node, int off)
> -- 
> 2.25.1
> 

Re: [PATCH 3/3] hfs: add lock nesting notation to hfs_find_init

[Viacheslav Dubeyko]

> On Jun 29, 2021, at 7:48 AM, Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> wrote:
> 
> Syzbot reports a possible recursive lock:
> https://syzkaller.appspot.com/bug?id=f007ef1d7a31a469e3be7aeb0fde0769b18585db
> 
> This happens due to missing lock nesting information. From the logs,
> we see that a call to hfs_fill_super is made to mount the hfs
> filesystem. While searching for the root inode, the lock on the
> catalog btree is grabbed. Then, when the parent of the root isn't
> found, a call to __hfs_bnode_create is made to create the parent of
> the root. This eventually leads to a call to hfs_ext_read_extent which
> grabs a lock on the extents btree.
> 
> Since the order of locking is catalog btree -> extents btree, this
> lock hierarchy does not lead to a deadlock.
> 
> To tell lockdep that this locking is safe, we add nesting notation to
> distinguish between catalog btrees, extents btrees, and attributes
> btrees (for HFS+). This has already been done in hfsplus.
> 
> Reported-and-tested-by: syzbot+b718ec84a87b7e73ade4@syzkaller.appspotmail.com
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> ---
> fs/hfs/bfind.c | 14 +++++++++++++-
> fs/hfs/btree.h |  7 +++++++
> 2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c
> index 4af318fbda77..ef9498a6e88a 100644
> --- a/fs/hfs/bfind.c
> +++ b/fs/hfs/bfind.c
> @@ -25,7 +25,19 @@ int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd)
> 	fd->key = ptr + tree->max_key_len + 2;
> 	hfs_dbg(BNODE_REFS, "find_init: %d (%p)\n",
> 		tree->cnid, __builtin_return_address(0));
> -	mutex_lock(&tree->tree_lock);
> +	switch (tree->cnid) {
> +	case HFS_CAT_CNID:
> +		mutex_lock_nested(&tree->tree_lock, CATALOG_BTREE_MUTEX);
> +		break;
> +	case HFS_EXT_CNID:
> +		mutex_lock_nested(&tree->tree_lock, EXTENTS_BTREE_MUTEX);
> +		break;
> +	case HFS_ATTR_CNID:
> +		mutex_lock_nested(&tree->tree_lock, ATTR_BTREE_MUTEX);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> 	return 0;
> }
> 
> diff --git a/fs/hfs/btree.h b/fs/hfs/btree.h
> index 4ba45caf5939..0e6baee93245 100644
> --- a/fs/hfs/btree.h
> +++ b/fs/hfs/btree.h
> @@ -13,6 +13,13 @@ typedef int (*btree_keycmp)(const btree_key *, const btree_key *);
> 
> #define NODE_HASH_SIZE  256
> 
> +/* B-tree mutex nested subclasses */
> +enum hfs_btree_mutex_classes {
> +	CATALOG_BTREE_MUTEX,
> +	EXTENTS_BTREE_MUTEX,
> +	ATTR_BTREE_MUTEX,
> +};
> +
> /* A HFS BTree held in memory */
> struct hfs_btree {
> 	struct super_block *sb;
> -- 
> 2.25.1
> 

Looks good to me.

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.

Re: [PATCH 2/3] hfs: fix high memory mapping in hfs_bnode_read

[Desmond Cheong Zhi Xi]

On 30/6/21 3:31 am, Viacheslav Dubeyko wrote:
> 
> 
>> On Jun 29, 2021, at 7:48 AM, Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> wrote:
>>
>> Pages that we read in hfs_bnode_read need to be kmapped into kernel
>> address space. However, currently only the 0th page is kmapped. If the
>> given offset + length exceeds this 0th page, then we have an invalid
>> memory access.
>>
>> To fix this, we use the same logic used  in hfsplus' version of
>> hfs_bnode_read to kmap each page of relevant data that we copy.
>>
>> An example of invalid memory access occurring without this fix can be
>> seen in the following crash report:
>>
>> ==================================================================
>> BUG: KASAN: use-after-free in memcpy include/linux/fortify-string.h:191 [inline]
>> BUG: KASAN: use-after-free in hfs_bnode_read+0xc4/0xe0 fs/hfs/bnode.c:26
>> Read of size 2 at addr ffff888125fdcffe by task syz-executor5/4634
>>
>> CPU: 0 PID: 4634 Comm: syz-executor5 Not tainted 5.13.0-syzkaller #0
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>> Call Trace:
>> __dump_stack lib/dump_stack.c:79 [inline]
>> dump_stack+0x195/0x1f8 lib/dump_stack.c:120
>> print_address_description.constprop.0+0x1d/0x110 mm/kasan/report.c:233
>> __kasan_report mm/kasan/report.c:419 [inline]
>> kasan_report.cold+0x7b/0xd4 mm/kasan/report.c:436
>> check_region_inline mm/kasan/generic.c:180 [inline]
>> kasan_check_range+0x154/0x1b0 mm/kasan/generic.c:186
>> memcpy+0x24/0x60 mm/kasan/shadow.c:65
>> memcpy include/linux/fortify-string.h:191 [inline]
>> hfs_bnode_read+0xc4/0xe0 fs/hfs/bnode.c:26
>> hfs_bnode_read_u16 fs/hfs/bnode.c:34 [inline]
>> hfs_bnode_find+0x880/0xcc0 fs/hfs/bnode.c:365
>> hfs_brec_find+0x2d8/0x540 fs/hfs/bfind.c:126
>> hfs_brec_read+0x27/0x120 fs/hfs/bfind.c:165
>> hfs_cat_find_brec+0x19a/0x3b0 fs/hfs/catalog.c:194
>> hfs_fill_super+0xc13/0x1460 fs/hfs/super.c:419
>> mount_bdev+0x331/0x3f0 fs/super.c:1368
>> hfs_mount+0x35/0x40 fs/hfs/super.c:457
>> legacy_get_tree+0x10c/0x220 fs/fs_context.c:592
>> vfs_get_tree+0x93/0x300 fs/super.c:1498
>> do_new_mount fs/namespace.c:2905 [inline]
>> path_mount+0x13f5/0x20e0 fs/namespace.c:3235
>> do_mount fs/namespace.c:3248 [inline]
>> __do_sys_mount fs/namespace.c:3456 [inline]
>> __se_sys_mount fs/namespace.c:3433 [inline]
>> __x64_sys_mount+0x2b8/0x340 fs/namespace.c:3433
>> do_syscall_64+0x37/0xc0 arch/x86/entry/common.c:47
>> entry_SYSCALL_64_after_hwframe+0x44/0xae
>> RIP: 0033:0x45e63a
>> Code: 48 c7 c2 bc ff ff ff f7 d8 64 89 02 b8 ff ff ff ff eb d2 e8 88 04 00 00 0f 1f 84 00 00 00 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
>> RSP: 002b:00007f9404d410d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
>> RAX: ffffffffffffffda RBX: 0000000020000248 RCX: 000000000045e63a
>> RDX: 0000000020000000 RSI: 0000000020000100 RDI: 00007f9404d41120
>> RBP: 00007f9404d41120 R08: 00000000200002c0 R09: 0000000020000000
>> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000003
>> R13: 0000000000000003 R14: 00000000004ad5d8 R15: 0000000000000000
>>
>> The buggy address belongs to the page:
>> page:00000000dadbcf3e refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x125fdc
>> flags: 0x2fffc0000000000(node=0|zone=2|lastcpupid=0x3fff)
>> raw: 02fffc0000000000 ffffea000497f748 ffffea000497f6c8 0000000000000000
>> raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
>> page dumped because: kasan: bad access detected
>>
>> Memory state around the buggy address:
>> ffff888125fdce80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>> ffff888125fdcf00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>>> ffff888125fdcf80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>>                                                                 ^
>> ffff888125fdd000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>> ffff888125fdd080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>> ==================================================================
>>
>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
>> ---
>> fs/hfs/bnode.c | 18 ++++++++++++++----
>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
>> index b63a4df7327b..936cfa763224 100644
>> --- a/fs/hfs/bnode.c
>> +++ b/fs/hfs/bnode.c
>> @@ -18,13 +18,23 @@
>> void hfs_bnode_read(struct hfs_bnode *node, void *buf,
>> 		int off, int len)
>> {
>> -	struct page *page;
>> +	struct page **pagep;
>> +	int l;
>>
>> 	off += node->page_offset;
>> -	page = node->page[0];
>> +	pagep = node->page + (off >> PAGE_SHIFT);
> 
> I would like to have a check here that we are not out of the page array. Could you add this check?
> 
> Also, maybe, node->page[index] could look much safer here. What do you think?
> 

Hi Slava,

Thanks for the review.

Checking that we don't exceed the page array sounds good. Also agree 
that node->page[index] looks more readable.

>> +	off &= ~PAGE_MASK;
>>
>> -	memcpy(buf, kmap(page) + off, len);
>> -	kunmap(page);
>> +	l = min_t(int, len, PAGE_SIZE - off);
> 
> Maybe, it makes sense to use more informative name of the variable instead of “l”?
> 
>> +	memcpy(buf, kmap(*pagep) + off, l);
> 
> I suppose that it could be good to have a check that we do not overflow the buffer. How do you feel about it?
> 

The computation of `l = min_t(int, len, PAGE_SIZE - off);` ensures that 
we don't overflow the buffer because the number of bytes read won't 
exceed the given len. But I think you're right that this can be made 
more explicit, alongside giving "l" a more informative name.

>> +	kunmap(*pagep);
> 
> What’s about kmap_atomic/kunmap_atomic in this function?
> 
> Thanks,
> Slava.
> 

kmap_atomic/kunmap_atomic sounds good to me. I'll followup with a v2 
series. Thanks again!

Best wishes,
Desmond

>> +
>> +	while ((len -= l) != 0) {
>> +		buf += l;
>> +		l = min_t(int, len, PAGE_SIZE);
>> +		memcpy(buf, kmap(*++pagep), l);
>> +		kunmap(*pagep);
>> +	}
>> }
>>
>> u16 hfs_bnode_read_u16(struct hfs_bnode *node, int off)
>> -- 
>> 2.25.1
>>
> 

Re: [PATCH 1/3] hfs: add missing clean-up in hfs_fill_super

[Desmond Cheong Zhi Xi]

On 30/6/21 3:13 am, Viacheslav Dubeyko wrote:
> 
> 
>> On Jun 29, 2021, at 7:48 AM, Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> wrote:
>>
>> On exiting hfs_fill_super, the file descriptor used in hfs_find_init
>> should be passed to hfs_find_exit to be cleaned up, and to release the
>> lock held on the btree.
>>
>> The call to hfs_find_exit is missing from this error path, so we add
>> it in to release resources.
>>
>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
>> ---
>> fs/hfs/super.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
>> index 44d07c9e3a7f..48340b77eb36 100644
>> --- a/fs/hfs/super.c
>> +++ b/fs/hfs/super.c
>> @@ -419,6 +419,7 @@ static int hfs_fill_super(struct super_block *sb, void *data, int silent)
>> 	res = hfs_cat_find_brec(sb, HFS_ROOT_CNID, &fd);
>> 	if (!res) {
>> 		if (fd.entrylength > sizeof(rec) || fd.entrylength < 0) {
>> +			hfs_find_exit(&fd);
> 
> I see that there are several places of hfs_find_exit() calls in hfs_fill_super(). Maybe, it makes sense to move the hfs_find_exit() call to the end of the hfs_fill_super()? In this case we could process this activity of resources freeing into one place. I mean line 449 in the source code (failure case).
> 
> Thanks,
> Slava.
> 
>> 			res =  -EIO;
>> 			goto bail;
>> 		}
>> -- 
>> 2.25.1
>>
> 

Thanks for the suggestion. Since the bail and bail_no_root error paths 
are used before hfs_find_init and after hfs_find_exit are called in the 
normal execution case, moving hfs_find_exit under the bail label 
wouldn't work.

Perhaps this can be done by introducing another goto label. Any thoughts 
on the following?

diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 44d07c9e3a7f..12d9bae39363 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -420,14 +420,12 @@ static int hfs_fill_super(struct super_block *sb, 
void *data, int silent)
         if (!res) {
                 if (fd.entrylength > sizeof(rec) || fd.entrylength < 0) {
                         res =  -EIO;
-                       goto bail;
+                       goto bail_hfs_find;
                 }
                 hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, 
fd.entrylength);
         }
-       if (res) {
-               hfs_find_exit(&fd);
-               goto bail_no_root;
-       }
+       if (res)
+               goto bail_hfs_find;
         res = -EINVAL;
         root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
         hfs_find_exit(&fd);
@@ -443,6 +441,8 @@ static int hfs_fill_super(struct super_block *sb, 
void *data, int silent)
         /* everything's okay */
         return 0;

+bail_hfs_find:
+       hfs_find_exit(&fd);
  bail_no_root:
         pr_err("get root inode failed\n");
  bail:

Re: [PATCH 1/3] hfs: add missing clean-up in hfs_fill_super

[Viacheslav Dubeyko]

> On Jun 29, 2021, at 9:50 PM, Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> wrote:

<skipped>

> Thanks for the suggestion. Since the bail and bail_no_root error paths are used before hfs_find_init and after hfs_find_exit are called in the normal execution case, moving hfs_find_exit under the bail label wouldn't work.
> 
> Perhaps this can be done by introducing another goto label. Any thoughts on the following?
> 
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index 44d07c9e3a7f..12d9bae39363 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -420,14 +420,12 @@ static int hfs_fill_super(struct super_block *sb, void *data, int silent)
>        if (!res) {
>                if (fd.entrylength > sizeof(rec) || fd.entrylength < 0) {
>                        res =  -EIO;
> -                       goto bail;
> +                       goto bail_hfs_find;
>                }
>                hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
>        }
> -       if (res) {
> -               hfs_find_exit(&fd);
> -               goto bail_no_root;
> -       }
> +       if (res)
> +               goto bail_hfs_find;
>        res = -EINVAL;
>        root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
>        hfs_find_exit(&fd);
> @@ -443,6 +441,8 @@ static int hfs_fill_super(struct super_block *sb, void *data, int silent)
>        /* everything's okay */
>        return 0;
> 
> +bail_hfs_find:
> +       hfs_find_exit(&fd);
> bail_no_root:
>        pr_err("get root inode failed\n");
> bail:

Makes sense. Looks good.

Thanks,
Slava.