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

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

[Dan Carpenter]

If we iterate through a loop using list_for_each_entry() without
hitting a break, then the iterator points to bogus memory.  The
if (tst->name != test_fw_config->upload_name) { will likely still work
but technically it's an out of bounds read.

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] test_firmware: fix end of loop test in upload_read_show()

[Luis Chamberlain]

On Thu, May 05, 2022 at 01:29:15PM +0300, Dan Carpenter wrote:
> If we iterate through a loop using list_for_each_entry() without
> hitting a break, then the iterator points to bogus memory.  The
> if (tst->name != test_fw_config->upload_name) { will likely still work
> but technically it's an out of bounds read.
> 
> 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);

Note the mutex lock.

> -	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 a lock is held I can't see how the premise of this patch is
correct and we ensure we don't remove entries while holdingg
the lock.

Generalizing this problem seems like a bigger issue, no?

Additionally this patch doesn't apply at all on linux-next.

  Luis

> +		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] test_firmware: fix end of loop test in upload_read_show()

[Dan Carpenter]

The patch applies to today's, May 5, linux-next just fine but I think
I need to re-write the commit message to make the bug more clear.

On Thu, May 05, 2022 at 05:39:35AM -0700, Luis Chamberlain wrote:
> On Thu, May 05, 2022 at 01:29:15PM +0300, Dan Carpenter wrote:
> > If we iterate through a loop using list_for_each_entry() without
> > hitting a break, then the iterator points to bogus memory.  The
> > if (tst->name != test_fw_config->upload_name) { will likely still work
> > but technically it's an out of bounds read.
> > 
> > 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);
> 
> Note the mutex lock.
> 

This lock is fine.

> > -	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 a lock is held I can't see how the premise of this patch is
> correct and we ensure we don't remove entries while holdingg
> the lock.
> 
> Generalizing this problem seems like a bigger issue, no?
> 

It has nothing to do with the look.  The problem is using the list
iterator outside of the loop.


> Additionally this patch doesn't apply at all on linux-next.
> 
>   Luis
> 
> > +		if (tst_iter->name == test_fw_config->upload_name) {
> > +			tst = tst_iter;
> >  			break;
> > +		}
> >  
> > -	if (tst->name != test_fw_config->upload_name) {
> > +	if (!tst) {

This test is reading out of bounds.  Another fix would be to write it
as:

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

But there is a desire to make it impossible to access the list iterator
outside the loop.  Linus was drafting alternative list macros but I
don't know the status of that.

regards,
dan carpenter

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

[Luis Chamberlain]

On Thu, May 05, 2022 at 04:02:51PM +0300, Dan Carpenter wrote:
> The patch applies to today's, May 5, linux-next just fine but I think
> I need to re-write the commit message to make the bug more clear.

Odd not for me.

> On Thu, May 05, 2022 at 05:39:35AM -0700, Luis Chamberlain wrote:
> > On Thu, May 05, 2022 at 01:29:15PM +0300, Dan Carpenter wrote:
> > > If we iterate through a loop using list_for_each_entry() without
> > > hitting a break, then the iterator points to bogus memory.  The
> > > if (tst->name != test_fw_config->upload_name) { will likely still work
> > > but technically it's an out of bounds read.
> > > 
> > > 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);
> > 
> > Note the mutex lock.
> > 
> 
> This lock is fine.
> 
> > > -	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 a lock is held I can't see how the premise of this patch is
> > correct and we ensure we don't remove entries while holdingg
> > the lock.
> > 
> > Generalizing this problem seems like a bigger issue, no?
> > 
> 
> It has nothing to do with the look.  The problem is using the list
> iterator outside of the loop.

Ah the new infamous list iterator violation bug..

> > Additionally this patch doesn't apply at all on linux-next.
> > 
> >   Luis
> > 
> > > +		if (tst_iter->name == test_fw_config->upload_name) {
> > > +			tst = tst_iter;
> > >  			break;
> > > +		}
> > >  
> > > -	if (tst->name != test_fw_config->upload_name) {
> > > +	if (!tst) {
> 
> This test is reading out of bounds.  Another fix would be to write it
> as:
> 
> 	if (list_entry_is_head(tst, &test_upload_list, node)) {
> 
> But there is a desire to make it impossible to access the list iterator
> outside the loop.  Linus was drafting alternative list macros but I
> don't know the status of that.

Fine to get these fixes merged, but it would seem test firmware
would be low on the list of places to fix. Either way I'm happy
for this to go in.

Greg, want to pick it up?

  Luis

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

[Dan Carpenter]

On Mon, May 09, 2022 at 03:36:53PM -0700, Luis Chamberlain wrote:
> > This test is reading out of bounds.  Another fix would be to write it
> > as:
> > 
> > 	if (list_entry_is_head(tst, &test_upload_list, node)) {
> > 
> > But there is a desire to make it impossible to access the list iterator
> > outside the loop.  Linus was drafting alternative list macros but I
> > don't know the status of that.
> 
> Fine to get these fixes merged, but it would seem test firmware
> would be low on the list of places to fix. Either way I'm happy
> for this to go in.
> 

I'm just fixing new static checker bugs as they are added, but you
made me curiuos to see if there were old known bugs still.

The following functions are still buggy:
wd719x_interrupt()
nvkm_clk_ustate_update()

Most of the others are Smatch false positives, but some are a bit tricky
and I'm not sure either way.  I marked them as false positives if the
author seemed confident that no end of loop test was required.

Bugs:
drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c:489 nvkm_clk_ustate_update() warn: iterator used outside loop: 'pstate'
drivers/scsi/wd719x.c:691 wd719x_interrupt() warn: iterator used outside loop: 'scb'
lib/test_firmware.c:1408 upload_read_show() warn: iterator used outside loop: 'tst'

Tricky:
drivers/net/ethernet/mellanox/mlx4/alloc.c:379 __mlx4_alloc_from_zone() warn: iterator used outside loop: 'curr_node'

False positives:
drivers/usb/host/uhci-q.c:466 link_async() warn: iterator used outside loop: 'pqh'
drivers/infiniband/core/mad.c:968 ib_get_rmpp_segment() warn: iterator used outside loop: 'mad_send_wr->cur_seg'
drivers/infiniband/hw/hfi1/tid_rdma.c:1280 kern_alloc_tids() warn: iterator used outside loop: 'group'
drivers/staging/rtl8192e/rtl819x_TSProc.c:260 SearchAdmitTRStream() warn: iterator used outside loop: 'pRet'
drivers/perf/xgene_pmu.c:1487 acpi_get_pmu_hw_inf() warn: iterator used outside loop: 'rentry'
drivers/perf/thunderx2_pmu.c:814 tx2_uncore_pmu_init_dev() warn: iterator used outside loop: 'rentry'
drivers/gpu/drm/vc4/vc4_dsi.c:769 vc4_dsi_encoder_disable() warn: iterator used outside loop: 'iter'
drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c:111 nvkm_control_mthd_pstate_attr() warn: iterator used outside loop: 'pstate'
drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c:283 nvkm_pstate_prog() warn: iterator used outside loop: 'pstate'
drivers/gpu/drm/panfrost/panfrost_mmu.c:203 panfrost_mmu_as_get() warn: iterator used outside loop: 'lru_mmu'
drivers/net/wireless/ath/ath6kl/htc_mbox.c:107 ath6kl_credit_init() warn: iterator used outside loop: 'cur_ep_dist'
net/xfrm/xfrm_ipcomp.c:246 ipcomp_free_tfms() warn: iterator used outside loop: 'pos'

regards,
dan carpenter