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:   Thu, 5 May 2022 16:02:51 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        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()

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


> 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