linux-kernel - Re: [PATCH] test_firmware: fix end of loop test in upload_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 May 2022 15:36:53 -0700
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Dan Carpenter <dan.carpenter@...cle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Russ Weight <russell.h.weight@...el.com>,
        Tianfei zhang <tianfei.zhang@...el.com>,
        Lee Jones <lee.jones@...aro.org>,
        Shawn Guo <shawn.guo@...aro.org>, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] test_firmware: fix end of loop test in upload_read_show()

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@...cle.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