linux-kernel - Re: [PATCH] fscache: fix OOB Read in __fscache_acquire

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 15 Nov 2022 21:37:12 +0900
From:   asmadeus@...ewreck.org
To:     Peng Zhang <zhangpeng362@...wei.com>, dhowells@...hat.com
Cc:     ericvh@...il.com, lucho@...kov.net, linux_oss@...debyte.com,
        jlayton@...nel.org, v9fs-developer@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org,
        syzbot+a76f6a6e524cf2080aa3@...kaller.appspotmail.com
Subject: Re: [PATCH] fscache: fix OOB Read in __fscache_acquire_volume

Peng Zhang wrote on Tue, Nov 15, 2022 at 12:27:01PM +0000:
> 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().

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):
---
diff --git a/fs/fscache/volume.c b/fs/fscache/volume.c
index a058e0136bfe..cc206d5e4cc7 100644
--- a/fs/fscache/volume.c
+++ b/fs/fscache/volume.c
@@ -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