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

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

Posted by Pengpeng Hou 1 month, 4 weeks ago

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
simple_read_from_buffer().

Fixes: fa516b66a1bf ("EVM: Allow runtime modification of the set of verified xattrs")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
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;
 	}
+	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;
+		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);
 
-- 
2.50.1 (Apple Git-155)

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

Posted by Roberto Sassu 1 month, 4 weeks ago

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@iscas.ac.cn>
> ---
> 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);
>

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

Posted by Roberto Sassu 1 month, 3 weeks ago

On Fri, 2026-04-17 at 10:30 +0200, Roberto Sassu wrote:
> 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@iscas.ac.cn>
> > ---
> > 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.

Any progress? The changes should be straightforward.

Thanks

Roberto

> 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);
> >

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

Posted by Pengpeng Hou 1 month, 3 weeks ago

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, 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@iscas.ac.cn>
---
Changes since v2:
- adjust the changelog wording to mention why the accumulated length is
  safe
- add the blank line after the allocation error path
- add a comment explaining why snprintf() cannot truncate in the fill loop

diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
index acd840461902..4baf5e23bc97 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)
@@ -151,16 +151,22 @@ static ssize_t evm_read_xattrs(struct file *filp, char __user *buf,
 		return -ENOMEM;
 	}
 
+	temp[size] = '\0';
+
+	/*
+	 * No truncation possible: size is computed over the same enabled
+	 * xattrs under xattr_list_mutex, so offset never exceeds size.
+	 */
 	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;
+		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);
 
-- 
2.50.1 (Apple Git-155)

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

Posted by Roberto Sassu 1 month, 3 weeks ago

On Thu, 2026-04-23 at 23:30 +0800, 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, 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@iscas.ac.cn>

Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com>

Thanks

Roberto

> ---
> Changes since v2:
> - adjust the changelog wording to mention why the accumulated length is
>   safe
> - add the blank line after the allocation error path
> - add a comment explaining why snprintf() cannot truncate in the fill loop
> 
> diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
> index acd840461902..4baf5e23bc97 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)
> @@ -151,16 +151,22 @@ static ssize_t evm_read_xattrs(struct file *filp, char __user *buf,
>  		return -ENOMEM;
>  	}
>  
> +	temp[size] = '\0';
> +
> +	/*
> +	 * No truncation possible: size is computed over the same enabled
> +	 * xattrs under xattr_list_mutex, so offset never exceeds size.
> +	 */
>  	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;
> +		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);
>