Re: [PATCH] fsverity: reject short BPF digest buffers

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds write (TOCTOU) due to use of shared memory in pointer arithmetic.
--

--- Patch [1]: [PATCH] fsverity: reject short BPF digest buffers ---
commit f40b6203fcacca3140ba489474dd88b289d0ffc4
Author: Yichong Chen <[email protected]>

fsverity: reject short BPF digest buffers

bpf_get_fsverity_digest() reports the digest size as the full hash digest
size. However, when the provided dynptr only has room for part of the
digest, the helper currently copies the truncated digest and still returns
success. This changes it to return -EOVERFLOW instead to match the ioctl
measurement path.

> diff --git a/fs/verity/measure.c b/fs/verity/measure.c
> index cfe2d5e535f96..4cfadba95488b 100644
> --- a/fs/verity/measure.c
> +++ b/fs/verity/measure.c
> @@ -148,9 +148,11 @@ __bpf_kfunc int bpf_get_fsverity_digest(struct file *file, const struct bpf_dynp
>  	arg->digest_size = hash_alg->digest_size;
>  
>  	out_digest_sz = dynptr_sz - sizeof(struct fsverity_digest);
> +	if (out_digest_sz < hash_alg->digest_size)
> +		return -EOVERFLOW;
>  
>  	/* copy digest */
> -	memcpy(arg->digest, vi->file_digest,  min_t(int, hash_alg->digest_size, out_digest_sz));
> +	memcpy(arg->digest, vi->file_digest, hash_alg->digest_size);
>  
>  	/* fill the extra buffer with zeros */
>  	if (out_digest_sz > hash_alg->digest_size)

[Severity: High]
This is a pre-existing issue, but can the arg->digest_size field be modified
concurrently after it is initialized?

Looking at bpf_get_fsverity_digest(), the memset() operation directly following
this hunk uses arg->digest_size to compute the destination pointer:

    if (out_digest_sz > hash_alg->digest_size)
        memset(arg->digest + arg->digest_size, 0,
               out_digest_sz - hash_alg->digest_size);

Since arg is a pointer into a dynptr that might be backed by shared memory,
if an attacker overwrites arg->digest_size with a large value before the
memset() executes, could this cause an out-of-bounds write of zeros into
kernel memory?

Would it be safer to use hash_alg->digest_size instead of re-reading
arg->digest_size when calculating the pointer offset for the memset?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1