Re: [PATCH] fscache: fix OOB Read in __fscache_acquire_volume

[asmadeus@codewreck.org]

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