evm: zero-initialize the evm_xattrs read buffer

[PATCH] evm: zero-initialize the evm_xattrs read buffer

Posted by Pengpeng Hou 2 months, 1 week 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.

Use kzalloc() so the empty-list case stays a valid empty C string.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 security/integrity/evm/evm_secfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
index acd840461902..03d376fa36c2 100644
--- a/security/integrity/evm/evm_secfs.c
+++ b/security/integrity/evm/evm_secfs.c
@@ -145,7 +145,7 @@ static ssize_t evm_read_xattrs(struct file *filp, char __user *buf,
 		size += strlen(xattr->name) + 1;
 	}
 
-	temp = kmalloc(size + 1, GFP_KERNEL);
+	temp = kzalloc(size + 1, GFP_KERNEL);
 	if (!temp) {
 		mutex_unlock(&xattr_list_mutex);
 		return -ENOMEM;
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] evm: zero-initialize the evm_xattrs read buffer

Posted by Pengpeng Hou 1 month, 4 weeks ago

Hi Roberto,

Thanks, I'll respin this.

I'll add the `Fixes:` tag, switch the formatting site to `snprintf()`,
and rework the empty-list handling so it does not depend on `kzalloc()`
for the terminator.

Thanks,
Pengpeng

Re: [PATCH] evm: zero-initialize the evm_xattrs read buffer

Posted by Roberto Sassu 2 months ago

On Tue, 2026-04-07 at 14:09 +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.
> 
> Use kzalloc() so the empty-list case stays a valid empty C string.

Please also add the Fixes: tag with the relevant commit.

> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  security/integrity/evm/evm_secfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
> index acd840461902..03d376fa36c2 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -145,7 +145,7 @@ static ssize_t evm_read_xattrs(struct file *filp, char __user *buf,
>  		size += strlen(xattr->name) + 1;
>  	}
>  
> -	temp = kmalloc(size + 1, GFP_KERNEL);
> +	temp = kzalloc(size + 1, GFP_KERNEL);

Yes, or just set temp[size] to the terminator so that we don't waste
computation. Can you also change sprintf() to snprintf()?

Thanks

Roberto

>  	if (!temp) {
>  		mutex_unlock(&xattr_list_mutex);
>  		return -ENOMEM;

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