[PATCH v2] test_firmware: fix end of loop test in upload_read_show()

[PATCH v2] test_firmware: fix end of loop test in upload_read_show()

[Dan Carpenter]

If a list_for_each_entry() loop exits without hitting a break statement
then the iterator points to invalid memory.  So in this code the
"tst->name" dereference is an out bounds read.  It's an offset from the
&test_upload_list pointer and it will likely work fine most of the time
but it's not correct.

One alternative is to fix this this by changing the test to:

	if (list_entry_is_head(tst, &test_upload_list, node)) {

But the simpler, trendy new way is just create a new variable and test
for NULL.

Fixes: a31ad463b72d ("test_firmware: Add test support for firmware upload")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 lib/test_firmware.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 76115c1a2629..c82b65947ce6 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -1392,7 +1392,8 @@ static ssize_t upload_read_show(struct device *dev,
 				struct device_attribute *attr,
 				char *buf)
 {
-	struct test_firmware_upload *tst;
+	struct test_firmware_upload *tst = NULL;
+	struct test_firmware_upload *tst_iter;
 	int ret = -EINVAL;
 
 	if (!test_fw_config->upload_name) {
@@ -1401,11 +1402,13 @@ static ssize_t upload_read_show(struct device *dev,
 	}
 
 	mutex_lock(&test_fw_mutex);
-	list_for_each_entry(tst, &test_upload_list, node)
-		if (tst->name == test_fw_config->upload_name)
+	list_for_each_entry(tst_iter, &test_upload_list, node)
+		if (tst_iter->name == test_fw_config->upload_name) {
+			tst = tst_iter;
 			break;
+		}
 
-	if (tst->name != test_fw_config->upload_name) {
+	if (!tst) {
 		pr_err("Firmware name not found: %s\n",
 		       test_fw_config->upload_name);
 		goto out;
-- 
2.35.1

Re: [PATCH v2] test_firmware: fix end of loop test in upload_read_show()

[Greg Kroah-Hartman]

On Thu, May 05, 2022 at 04:10:15PM +0300, Dan Carpenter wrote:
> If a list_for_each_entry() loop exits without hitting a break statement
> then the iterator points to invalid memory.  So in this code the
> "tst->name" dereference is an out bounds read.  It's an offset from the
> &test_upload_list pointer and it will likely work fine most of the time
> but it's not correct.
> 
> One alternative is to fix this this by changing the test to:
> 
> 	if (list_entry_is_head(tst, &test_upload_list, node)) {
> 
> But the simpler, trendy new way is just create a new variable and test
> for NULL.
> 
> Fixes: a31ad463b72d ("test_firmware: Add test support for firmware upload")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  lib/test_firmware.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/test_firmware.c b/lib/test_firmware.c
> index 76115c1a2629..c82b65947ce6 100644
> --- a/lib/test_firmware.c
> +++ b/lib/test_firmware.c
> @@ -1392,7 +1392,8 @@ static ssize_t upload_read_show(struct device *dev,
>  				struct device_attribute *attr,
>  				char *buf)
>  {
> -	struct test_firmware_upload *tst;
> +	struct test_firmware_upload *tst = NULL;
> +	struct test_firmware_upload *tst_iter;
>  	int ret = -EINVAL;
>  
>  	if (!test_fw_config->upload_name) {
> @@ -1401,11 +1402,13 @@ static ssize_t upload_read_show(struct device *dev,
>  	}
>  
>  	mutex_lock(&test_fw_mutex);
> -	list_for_each_entry(tst, &test_upload_list, node)
> -		if (tst->name == test_fw_config->upload_name)
> +	list_for_each_entry(tst_iter, &test_upload_list, node)
> +		if (tst_iter->name == test_fw_config->upload_name) {
> +			tst = tst_iter;
>  			break;
> +		}
>  
> -	if (tst->name != test_fw_config->upload_name) {
> +	if (!tst) {
>  		pr_err("Firmware name not found: %s\n",
>  		       test_fw_config->upload_name);
>  		goto out;
> -- 
> 2.35.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH v2] test_firmware: fix end of loop test in upload_read_show()

[Russ Weight]

On 5/5/22 06:10, Dan Carpenter wrote:
> If a list_for_each_entry() loop exits without hitting a break statement
> then the iterator points to invalid memory.  So in this code the
> "tst->name" dereference is an out bounds read.  It's an offset from the
> &test_upload_list pointer and it will likely work fine most of the time
> but it's not correct.
>
> One alternative is to fix this this by changing the test to:
>
> 	if (list_entry_is_head(tst, &test_upload_list, node)) {
>
> But the simpler, trendy new way is just create a new variable and test
> for NULL.
>
> Fixes: a31ad463b72d ("test_firmware: Add test support for firmware upload")
Reviewed-by: Russ Weight <russell.h.weight@intel.com>

Thanks,
- Russ

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  lib/test_firmware.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/lib/test_firmware.c b/lib/test_firmware.c
> index 76115c1a2629..c82b65947ce6 100644
> --- a/lib/test_firmware.c
> +++ b/lib/test_firmware.c
> @@ -1392,7 +1392,8 @@ static ssize_t upload_read_show(struct device *dev,
>  				struct device_attribute *attr,
>  				char *buf)
>  {
> -	struct test_firmware_upload *tst;
> +	struct test_firmware_upload *tst = NULL;
> +	struct test_firmware_upload *tst_iter;
>  	int ret = -EINVAL;
>  
>  	if (!test_fw_config->upload_name) {
> @@ -1401,11 +1402,13 @@ static ssize_t upload_read_show(struct device *dev,
>  	}
>  
>  	mutex_lock(&test_fw_mutex);
> -	list_for_each_entry(tst, &test_upload_list, node)
> -		if (tst->name == test_fw_config->upload_name)
> +	list_for_each_entry(tst_iter, &test_upload_list, node)
> +		if (tst_iter->name == test_fw_config->upload_name) {
> +			tst = tst_iter;
>  			break;
> +		}
>  
> -	if (tst->name != test_fw_config->upload_name) {
> +	if (!tst) {
>  		pr_err("Firmware name not found: %s\n",
>  		       test_fw_config->upload_name);
>  		goto out;