Re: [PATCH v2] evm: terminate and bound the evm_xattrs read buffer

[Roberto Sassu]

On 4/17/2026 2:44 PM, Pengpeng Hou wrote:
> evm_read_xattrs() allocates size + 1 bytes, fills them from the list of
> enabled xattrs, and then passes strlen(temp) to
> simple_read_from_buffer(). When no configured xattrs are enabled, the
> fill loop stores nothing and temp[0] remains uninitialized, so strlen()
> reads beyond initialized memory.
>
> Explicitly terminate the buffer after allocation, use snprintf() for
> each formatted line, and pass the accumulated length to

pass the accumulate length (without risk of truncation) to ...

> simple_read_from_buffer().
>
> Fixes: fa516b66a1bf ("EVM: Allow runtime modification of the set of verified xattrs")
> Signed-off-by: Pengpeng Hou <pengpeng@xxxxxxxxxxx>
> ---
> Changes since v1:
> - add the Fixes tag
> - replace sprintf() with snprintf()
> - explicitly terminate the buffer instead of switching to kzalloc()
>
>   security/integrity/evm/evm_secfs.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
> index acd840461902..b7882a4ce9d0 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -127,8 +127,8 @@ static ssize_t evm_read_xattrs(struct file *filp, char __user *buf,
>   			       size_t count, loff_t *ppos)
>   {
>   	char *temp;
> -	int offset = 0;
> -	ssize_t rc, size = 0;
> +	size_t offset = 0, size = 0;
> +	ssize_t rc;
>   	struct xattr_list *xattr;
>   
>   	if (*ppos != 0)
> @@ -150,17 +150,18 @@ static ssize_t evm_read_xattrs(struct file *filp, char __user *buf,
>   		mutex_unlock(&xattr_list_mutex);
>   		return -ENOMEM;
>   	}

Please add a newline here.

> +	temp[size] = '\0';
>   
>   	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
>   		if (!xattr->enabled)
>   			continue;
>   
> -		sprintf(temp + offset, "%s\n", xattr->name);
> -		offset += strlen(xattr->name) + 1;

Also a comment like:

/*
 * No truncation possible: size is computed over the same
 * enabled xattrs under xattr_list_mutex, so offset never exceeds size.
 */

to motivate why it is fine to increment offset without checking.

Thanks

Roberto

> +		offset += snprintf(temp + offset, size + 1 - offset, "%s\n",
> +				   xattr->name);
>   	}
>   
>   	mutex_unlock(&xattr_list_mutex);
> -	rc = simple_read_from_buffer(buf, count, ppos, temp, strlen(temp));
> +	rc = simple_read_from_buffer(buf, count, ppos, temp, offset);
>   
>   	kfree(temp);
>