Re: [PATCH] evm: Bound writes in evm_read_protected_xattrs() to buffer_size

[Roberto Sassu]

On Thu, 2026-10-29 at 15:27 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> 
> ima_eventinodexattrs_init_common() calls evm_read_protected_xattrs()
> first with a NULL buffer to obtain the required size, allocates a buffer
> of that size, and then calls it again to fill it. Nothing keeps the
> result stable between the two calls: a protected xattr may be added to
> evm_config_xattrnames through the evm_xattrs securityfs file (with
> CONFIG_EVM_ADD_XATTRS=y), or a protected xattr may be set on, removed
> from, or changed in the measured file in the meantime.
> 
> However, the 'n' (names) and 'l' (lengths) cases write into the buffer
> without checking buffer_size, so if the data grew between the two calls,
> they write past the end of the allocation.
> 
> The 'v' (values) case passes the remaining space to __vfs_getxattr(),
> so the copy itself is bounded, but advances total_size by the length
> returned by the earlier probing __vfs_getxattr() call, rather than by
> the length actually copied. This causes two problems:
> 
> - If the probed size fits in the remaining space, but the value shrinks
>   before the copy, total_size does not exceed buffer_size, but the
>   returned size covers bytes of the buffer that were not written.
> 
> - If the probed size exceeds the remaining space, but the value shrinks
>   enough to fit before the copy, total_size exceeds buffer_size after
>   adding the probed size.
> 
> In addition, once the buffer is full, any further non-empty xattr (e.g.
> one added to the list or set on the file after the buffer size was
> determined) is read with a zero remaining size. Some filesystems, such
> as XFS and btrfs, take a zero size as a request for the xattr length,
> return it without copying, and total_size exceeds buffer_size as well.
> 
> When total_size exceeds buffer_size, either the next iteration passes a
> negative remaining size to __vfs_getxattr(), converted to a huge size_t,
> causing a write past the end of the buffer, or the caller reads past
> the end of the buffer.
> 
> Return -ERANGE in the 'n' and 'l' cases when the data no longer fits in
> the buffer. In the 'v' case, skip the copy if the probed length is zero,
> return -ERANGE if no space is left, so that __vfs_getxattr() is never
> called with a zero size, and advance total_size by the number of bytes
> actually copied. Since __vfs_getxattr() then either copies at most the
> remaining space or fails with -ERANGE, total_size never exceeds
> buffer_size. Also document the -ERANGE return in the kernel-doc.
> 
> On -ERANGE, ima_eventinodexattrs_init_common() records an empty template
> field, as it already does for the other errors.
> 
> Fixes: 8314b6732ae4 ("ima: Define new template fields xattrnames, xattrlengths and xattrvalues")
> Reported-by: Fabrice Derepas <fabrice.derepas@xxxxxxxxxxxxx>

Hi Fabrice

can change to Co-developed-by: Fabrice Derepas <fabrice.derepas@xxxxxxxxxxxxx>

if you provide your Signed-off-by.

Thanks

Roberto

> Closes: https://lore.kernel.org/linux-integrity/CAD2AHCbuQyKwR27hNcYRhU7njCeigmQoL9x9i25zb0N5-8Kx+A@xxxxxxxxxxxxxx/
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> ---
>  security/integrity/evm/evm_main.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 5be9349afcd0..a29b7f80e55d 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -369,6 +369,10 @@ int evm_protected_xattr_if_enabled(const char *req_xattr_name)
>   * given dentry and return the total size of copied data. If buffer is NULL,
>   * just return the total size.
>   *
> + * The set of protected xattrs and their values may change between a call with
> + * a NULL buffer and a subsequent call with a buffer of the returned size. If
> + * the data no longer fits in @buffer_size, -ERANGE is returned.
> + *
>   * Returns the total size on success, a negative value on error.
>   */
>  int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
> @@ -389,6 +393,9 @@ int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
>  		case 'n':
>  			size = strlen(xattr->name) + 1;
>  			if (buffer) {
> +				if (size > buffer_size - total_size)
> +					return -ERANGE;
> +
>  				if (total_size)
>  					*(buffer + total_size - 1) = '|';
>  
> @@ -398,6 +405,9 @@ int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
>  		case 'l':
>  			size = sizeof(u32);
>  			if (buffer) {
> +				if (size > buffer_size - total_size)
> +					return -ERANGE;
> +
>  				if (canonical_fmt)
>  					rc = (__force int)cpu_to_le32(rc);
>  
> @@ -407,12 +417,22 @@ int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
>  		case 'v':
>  			size = rc;
>  			if (buffer) {
> +				/* Nothing to copy. */
> +				if (!size)
> +					break;
> +
> +				/* A zero size would query the length. */
> +				if (buffer_size - total_size == 0)
> +					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;
>  		default: