Re: [PATCH] evm: fix TOCTOU heap OOB in protected xattr reads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] evm: incomplete TOCTOU bounds check in
  evm_read_protected_xattrs()
--

commit 94961f480ee0e6be15409d6ea100ede9af939446
Author: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>

evm: fix TOCTOU heap OOB in protected xattr reads

This commit adds bounds checks in evm_read_protected_xattrs() to reject
protected xattr names, lengths, and values that no longer fit in the
caller's buffer if an xattr changes between the size query and the read.

> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index b59e3f121b8a8..d00b050edb811 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
[ ... ]
> @@ -407,12 +415,17 @@ int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
>  		case 'v':
>  			size = rc;
>  			if (buffer) {
> +				if (total_size > buffer_size ||
> +				    size > buffer_size - total_size)
> +					return -ERANGE;
> +
>  				rc = __vfs_getxattr(dentry,
>  					d_backing_inode(dentry), xattr->name,
>  					buffer + total_size,
>  					buffer_size - total_size);
>  				if (rc < 0)
>  					return rc;
> +				size = rc;
>  			}
>  			break;

[Severity: Medium]
Can total_size still exceed buffer_size when total_size == buffer_size and
an xattr grows from 0 bytes between the two __vfs_getxattr() calls?

When the first __vfs_getxattr() returns 0 for an empty xattr and
total_size == buffer_size, the bounds check passes and the second
__vfs_getxattr() is called with a size argument of
buffer_size - total_size == 0. Many filesystem xattr handlers (such as
btrfs_getxattr(), xfs_attr_copy_value(), and fuse_getxattr()) treat a zero
size argument as a length query rather than an empty buffer, so if the
xattr concurrently grew to N > 0 bytes, __vfs_getxattr() returns N instead
of -ERANGE.

Assigning size = rc then advances total_size to buffer_size + N, causing
ima_eventinodexattrs_init_common() -> ima_write_template_field_data() to
memcpy() past the end of buffer (or dereference ZERO_SIZE_PTR when
buffer_size == 0). Skipping the second __vfs_getxattr() call when size == 0
or checking that rc <= buffer_size - total_size would avoid this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107194909.1061154-2-Jeremy.Jean@oss.cyber.gouv.fr?part=1