Messages in this thread | | | Date | Wed, 28 Apr 2021 11:08:47 +0100 | From | Russell King - ARM Linux admin <> | Subject | Re: [PATCH 131/190] Revert "net: 8390: fix potential NULL pointer dereferences" |
| |
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@umn.edu> > > Cc: David S. Miller <davem@davemloft.net> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > --- > > 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:
First, let's analyse pcmcia_request_window() and pcmcia_release_window(). pcmcia_request_window() allocates some memory for a struct resource, which pcmcia_release_window() releases. So, on the face of it, calling the former without a subsequent call to the latter looks like a memory leak, unless one gets a deeper understanding first.
When pcmcia_request_window() is called and is successful, the resource is marked with the window number that has been allocated to it, which is always non-zero:
#define WIN_FLAGS_REQ 0x1c
/* Configure the socket controller */ win->map = w+1; ... res->flags &= ~WIN_FLAGS_REQ; res->flags |= (win->map << 2) | IORESOURCE_MEM;
When a window is released, this is checked:
w = ((res->flags & IORESOURCE_BITS & WIN_FLAGS_REQ) >> 2) - 1; if (w >= MAX_WIN) return -EINVAL;
However, there is another path where a driver's resources are released, which is if the driver makes a call to pcmcia_disable_device():
for (i = 0; i < MAX_WIN; i++) { struct resource *res = p_dev->resource[MAX_IO_WIN + i]; if (res->flags & WIN_FLAGS_REQ) pcmcia_release_window(p_dev, res); }
This walks all the memory resources for the card, releasing any that are currently requested (have non-zero WIN_FLAGS_REQ bits) at the time of this call.
pcnet_cs makes a call to pcmcia_disable_device() from pcnet_release() which is called in two cases: firstly on any probe failure, and secondly on device removal in pcnet_detach().
Consequently, the best that can be said about the setup_shmem_window() memory leak is that it is a temporary leak - the allocated memory will be freed if either the probe eventually fails, or the driver is unbound.
Second point. setup_shmem_window() "failure" itself does not lead to probe failure. The function returns a value that indicates whether shmem can be used by the driver or not - it needs a subsequent call to fail. setup_shmem_window() merely returns a value to indicate whether shmem should be configured or not.
Third point. setup_shmem_window() can already "leak" this window allocation. If pcmcia_map_mem_page() fails, then the window is not released in a timely fashion. So, the "leak" is pre-existing in this driver, and the "leak" introduced by this patch already exists in other paths.
Fourth point. If setup_shmem_window() is successful, which means we use shmem, and we then fail to register the netdev in the subsequent register_netdev() call, we are reliant on pcmcia_disable_device() to clean up that allocation. We are also reliant on pcmcia_disable_device() to clean that up when the driver is unbound. So, this is an entirely normal state of affairs to rely on the action of this call to clean up these resources.
Fifth point. The effect of reverting the original patch will be a kernel oops should this ioremap() fail:
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); ^^^^^^^^^^^^^^^^^^^^
So, is the patch of net benefit or net harm?
1) it does not introduce any new issue that doesn't already exist. The driver has a short-term memory leak in certain circumstances already that is cleaned up via the normal action of other pcmcia_* calls.
2) it removes an opportunity for a NULL or close-to-NULL pointer dereference.
Therefore, the original patch is of net _benefit_ and should not be reverted.
It would have been nice to have had a patch that addresses the temporary leak in the driver, but I really don't see that as any justification for reverting this patch. Therefore, I NAK your revert of this change as that will result in more harm than good.
You said on IRC early this morning when you thought there was still a leak in the probe path: "the leak happens from probe, that is where it needs to be cleaned up properly, the patch was incorrect. You owe me a bottle of wine for even debating this crap..."
I think you now owe me more than just a bottle of wine... and I think you need to realise that your attitude also needs to be altered. Yes, I fully understand _why_ you made that comment - it's because you're swamped. However, we *need* to review and debate your reverts, and that is something you should DEFINITELY NOT be discouraging with such comments. If we discourage review and debate in the way you seem to be doing when someone raises a concern, we're yet again proving that our approach to review is broken.
It really concerns me that there seems to be a slap-dash attitude here, and push-back when one of your fellow kernel developers disagrees with your abbreviated unexplained analysis.
Are you sure you aren't causing harm?
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |