fscache: fix OOB Read in __fscache_acquire_volume

From: ZhangPeng <[email protected]>

Syzbot reported slab-out-of-bounds Read in __fscache_acquire_volume.

==================================================================

BUG: KASAN: slab-out-of-bounds in memcmp+0x16f/0x1c0 lib/string.c:757
Read of size 8 at addr ffff888016f3aa90 by task syz-executor344/3613

CPU: 0 PID: 3613 Comm: syz-executor344 Not tainted
6.0.0-rc2-syzkaller-00327-g8379c0b31fbc #0
Hardware name: Google Compute Engine/Google Compute Engine, BIOS
Google 07/22/2022
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
 print_address_description mm/kasan/report.c:317 [inline]
 print_report.cold+0x2ba/0x719 mm/kasan/report.c:433
 kasan_report+0xb1/0x1e0 mm/kasan/report.c:495
 memcmp+0x16f/0x1c0 lib/string.c:757
 memcmp include/linux/fortify-string.h:420 [inline]
 fscache_volume_same fs/fscache/volume.c:133 [inline]
 fscache_hash_volume fs/fscache/volume.c:171 [inline]
 __fscache_acquire_volume+0x76c/0x1080 fs/fscache/volume.c:328
 fscache_acquire_volume include/linux/fscache.h:204 [inline]
 v9fs_cache_session_get_cookie+0x143/0x240 fs/9p/cache.c:34
 v9fs_session_init+0x1166/0x1810 fs/9p/v9fs.c:473
 v9fs_mount+0xba/0xc90 fs/9p/vfs_super.c:126
 legacy_get_tree+0x105/0x220 fs/fs_context.c:610
 vfs_get_tree+0x89/0x2f0 fs/super.c:1530
 do_new_mount fs/namespace.c:3040 [inline]
 path_mount+0x1326/0x1e20 fs/namespace.c:3370
 do_mount fs/namespace.c:3383 [inline]
 __do_sys_mount fs/namespace.c:3591 [inline]
 __se_sys_mount fs/namespace.c:3568 [inline]
 __x64_sys_mount+0x27f/0x300 fs/namespace.c:3568
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f7d5064b1d9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 e1 14 00 00 90 48 89 f8 48 89

f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffd1700c028 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007ffd1700c060 RCX: 00007f7d5064b1d9
RDX: 0000000020000040 RSI: 0000000020000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000020000200 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000000f4240
R13: 0000000000000000 R14: 00007ffd1700c04c R15: 00007ffd1700c050
==================================================================

The type of a->key[0] is char. If the length of cache volume key is greater than 127, the value of a->key[0] is less than 0. In this case, klen becomes much larger than 255 after type conversion, because the type of klen is size_t. As a result, memcmp() is read out of bounds. Fix this by adding a check on the length of the key in v9fs_cache_session_get_cookie().

---
 fs/9p/cache.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index cebba4eaa0b5..2688840b734e 100644
@@ -21,12 +21,20 @@ int v9fs_cache_session_get_cookie(struct v9fs_session_info *v9ses,
 {
 	struct fscache_volume *vcookie;
 	char *name, *p;
+	size_t nlen;
 
 	name = kasprintf(GFP_KERNEL, "9p,%s,%s",
 			 dev_name, v9ses->cachetag ?: v9ses->aname);
 	if (!name)
 		return -ENOMEM;
 
+	nlen = strlen(name);
+	if (nlen > 127) {
+		pr_err("Invalid cache volume key length: %d\n", nlen);
+		kfree(name);
+		return -EINVAL;
+	}
+
 	for (p = name; *p; p++)
 		if (*p == '/')
 			*p = ';';

2.25.1

Peng Zhang wrote on Tue, Nov 15, 2022 at 12:27:01PM +0000:

Thanks for the analysis. (it took me a while to understand what a->key was about, this is referring to the code in fscache_volume_same...)

It feels like that's another problem that could be avoided by using unsigned... but I don't know enough about fscache to comment seriously about whether that'd be viable or not, and it'd just punt the limit from 127 to 255 anyway.

Rather than this patch, I've had a quick look at afs/cifs/ceph and it doesen't look like any of these check the name length before calling fscache_acquire_volume either -- I'd say it's worth moving that check there.
Perhaps in fscahce_alloc_volume() they already compute
klen = strlen(volume_key) to store it in key[0] -- making sure it fits a signed char before writing key[0] sounds like a good idea that'd benefit everyone?

Please test this (feel free to resend that):
---

index a058e0136bfe..cc206d5e4cc7 100644
@@ -230,6 +230,8 @@ static struct fscache_volume *fscache_alloc_volume(const char *volume_key,
 	 * hashing easier.
 	 */
 	klen = strlen(volume_key);
+	if (klen > 127)
+		goto err_cache;
 	hlen = round_up(1 + klen + 1, sizeof(__le32));
 	key = kzalloc(hlen, GFP_KERNEL);
 	if (!key)

---

David, comments welcome :)

--
Dominique