Re: [PATCH 131/190] Revert "net: 8390: fix potential NULL pointer dereferences"

[Greg Kroah-Hartman]

On Wed, Apr 28, 2021 at 11:08:47AM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Apr 27, 2021 at 08:34:26PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Apr 21, 2021 at 03:00:06PM +0200, Greg Kroah-Hartman wrote:
> > > This reverts commit c7cbc3e937b885c9394bf9d0ca21ceb75c2ac262.
> > > 
> > > Commits from @umn.edu addresses have been found to be submitted in "bad
> > > faith" to try to test the kernel community's ability to review "known
> > > malicious" changes.  The result of these submissions can be found in a
> > > paper published at the 42nd IEEE Symposium on Security and Privacy
> > > entitled, "Open Source Insecurity: Stealthily Introducing
> > > Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University
> > > of Minnesota) and Kangjie Lu (University of Minnesota).
> > > 
> > > Because of this, all submissions from this group must be reverted from
> > > the kernel tree and will need to be re-reviewed again to determine if
> > > they actually are a valid fix.  Until that work is complete, remove this
> > > change to ensure that no problems are being introduced into the
> > > codebase.
> > > 
> > > Cc: Kangjie Lu <kjlu@xxxxxxx>
> > > Cc: David S. Miller <davem@xxxxxxxxxxxxx>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/net/ethernet/8390/pcnet_cs.c | 10 ----------
> > >  1 file changed, 10 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/8390/pcnet_cs.c b/drivers/net/ethernet/8390/pcnet_cs.c
> > > index 9d3b1e0e425c..0a408fa2b7a4 100644
> > > --- a/drivers/net/ethernet/8390/pcnet_cs.c
> > > +++ b/drivers/net/ethernet/8390/pcnet_cs.c
> > > @@ -289,11 +289,6 @@ static struct hw_info *get_hwinfo(struct pcmcia_device *link)
> > >  
> > >      virt = ioremap(link->resource[2]->start,
> > >  	    resource_size(link->resource[2]));
> > > -    if (unlikely(!virt)) {
> > > -	    pcmcia_release_window(link, link->resource[2]);
> > > -	    return NULL;
> > > -    }
> > > -
> > >      for (i = 0; i < NR_INFO; i++) {
> > >  	pcmcia_map_mem_page(link, link->resource[2],
> > >  		hw_info[i].offset & ~(resource_size(link->resource[2])-1));
> > > @@ -1430,11 +1425,6 @@ static int setup_shmem_window(struct pcmcia_device *link, int start_pg,
> > >      /* Try scribbling on the buffer */
> > >      info->base = ioremap(link->resource[3]->start,
> > >  			resource_size(link->resource[3]));
> > > -    if (unlikely(!info->base)) {
> > > -	    ret = -ENOMEM;
> > > -	    goto failed;
> > > -    }
> > > -
> > >      for (i = 0; i < (TX_PAGES<<8); i += 2)
> > >  	__raw_writew((i>>1), info->base+offset+i);
> > >      udelay(100);
> > > -- 
> > > 2.31.1
> > > 
> > 
> > This change causes a memory leak on the error path, so I will keep the
> > revert.
> > 
> > But really, a pcmcia card with a failed ioremap() call?  That can never
> > happen here, so I'll just keep it reverted, it's not worth touching
> > again.
> 
> I'm afraid that I have to disagree with your analysis.
> 
> However, first thing is, it would help immensely if you actually said
> where the memory leak is, rather than using this boiler plate and
> rushing through the changes.
> 
> On IRC, you mentioned it was due to the change in setup_shmem_window(),
> specifically "no call to pcmcia_release_window()". This seems to imply
> that you have no problem with the change to get_hwinfo(), and indeed
> that part of the original change looks fully correct to me. So,
> concentrating on the setup_shmem_window() change in this patch:

<snip>

Fair enough, I'll drop this revert from my tree as you feel the original
commit was correct.

thanks for the review.

greg k-h