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

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

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

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@canonical.com>
Closes: https://lore.kernel.org/linux-integrity/CAD2AHCbuQyKwR27hNcYRhU7njCeigmQoL9x9i25zb0N5-8Kx+A@mail.gmail.com/
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 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:
-- 
2.43.0

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

[sashiko-bot]

> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> 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(),
> [ ... ]
> 
> Fixes: 8314b6732ae4 ("ima: Define new template fields xattrnames, xattrlengths and xattrvalues")
> Reported-by: Fabrice Derepas <fabrice.derepas@canonical.com>
> Closes: https://lore.kernel.org/linux-integrity/CAD2AHCbuQyKwR27hNcYRhU7njCeigmQoL9x9i25zb0N5-8Kx+A@mail.gmail.com/
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029142707.2602008-1-roberto.sassu@huaweicloud.com?part=1

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@huawei.com>
> 
> 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@canonical.com>

Hi Fabrice

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

if you provide your Signed-off-by.

Thanks

Roberto

> Closes: https://lore.kernel.org/linux-integrity/CAD2AHCbuQyKwR27hNcYRhU7njCeigmQoL9x9i25zb0N5-8Kx+A@mail.gmail.com/
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  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: