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@xxxxxxxxxx>
> > > ---
> > >  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