staging: slicoss: fix use-after-free in slic_entry_probe
PATCH 1/1 staging: slicoss: fix use-after-free in slic_entry_probe
Fix a use-after-free bug that can cause a kernel oops. If slic_card_init fails then slic_entry_probe() (the pci probe() function for this device) will return error without cleaning up memory (including the registered netdev struct).
---
This patch was originally sent here https://lkml.org/lkml/2014/5/6/10 with my google.com email address. But due to Google's recent change in DMARC policies, that patchset was silently dropped for at least some users (including my personal gmail account). So I'm sending it out now with my gmail.com account. Let me know if this is an issue. Thanks.
drivers/staging/slicoss/slicoss.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
drivers/staging/slicoss/slicoss.c +5 -11
@@ -3595,7 +3595,6 @@ static int slic_entry_probe(struct pci_dev *pcidev,struct net_device *netdev;struct adapter *adapter;void __iomem *memmapped_ioaddr =NULL ; -u32 status =0 ;ulong mmio_start =0 ;ulong mmio_len =0 ;struct sliccard *card =NULL ;
@@ -3686,16 +3685,11 @@ static int slic_entry_probe(struct pci_dev *pcidev, adapter->allocated =1 ; } - status =slic_card_init (card, adapter); + err =slic_card_init (card, adapter); +if (err) + goto err_out_unmap; -if (status !=0 ) { - card->state =CARD_FAIL ; - adapter->state =ADAPT_FAIL ; - adapter->linkstate =LINK_DOWN ; -dev_err (&pcidev->dev,"FAILED status[%x]\n" , status); - }else { -slic_adapter_set_hwaddr (adapter); - } +slic_adapter_set_hwaddr (adapter); netdev->base_addr = (unsigned long )adapter->memorybase; netdev->irq = adapter->irq;
@@ -3712,7 +3706,7 @@ static int slic_entry_probe(struct pci_dev *pcidev, cards_found++; -return status; +return 0 ; err_out_unmap:iounmap (memmapped_ioaddr);
Signature · 1 line
1.9.2
PATCH 1/1 staging: slicoss: fix free-after-free in slic_entry_remove
Fix two trivial free-after-free bugs in slic_entry_remove.
1. Don't iounmap() the same address twice. adapter->slic_regs (iounmap()'ed in slic_unmap_mmio_space()) and dev->base_addr (iounmap()'ed in slic_entry_remove()) are the same region.
2. Don't call release_mem_region() and pci_release_regions() on the same pci_dev struct. They both free pci memory regions.
---
This patch was originally sent here https://lkml.org/lkml/2014/5/6/3 with my google.com email address. But due to Google's recent change in DMARC policies, that patchset was silently dropped for at least some users (including my personal gmail account). So I'm sending it out now with my gmail.com account. Let me know if this is an issue. Thanks.
drivers/staging/slicoss/slicoss.c | 8 -------- 1 file changed, 8 deletions(-)
drivers/staging/slicoss/slicoss.c +0 -8
@@ -2955,8 +2955,6 @@ static void slic_card_cleanup(struct sliccard *card)static void slic_entry_remove (struct pci_dev *pcidev) {struct net_device *dev =pci_get_drvdata (pcidev); -u32 mmio_start =0 ; -uint mmio_len =0 ;struct adapter *adapter =netdev_priv (dev);struct sliccard *card;struct mcast_address *mcaddr, *mlist;
@@ -2965,12 +2963,6 @@ static void slic_entry_remove(struct pci_dev *pcidev)slic_unmap_mmio_space (adapter);unregister_netdev (dev); - mmio_start =pci_resource_start (pcidev,0 ); - mmio_len =pci_resource_len (pcidev,0 ); - -release_mem_region (mmio_start, mmio_len); - -iounmap ((void __iomem *)dev->base_addr);/* free multicast addresses */ mlist = adapter->mcastaddrs;while (mlist) {
Signature · 1 line
1.9.2
PATCH 1/1 staging: slicoss: fix use-after-free bug in slic_entry_remove
In reply to David MatlackFix a use-after-free bug that causes a null pointer dereference in slic_entry_halt().
Since unregister_netdev() will ultimately call slic_entry_halt() (the net_device ndo_stop() virtual function for this device), we should call it before freeing the memory that is used by slic_entry_halt(). Specifically, slic_unmap_mmio_space() frees adapter->slic_regs, but those registers are used in slic_entry_halt().
---
This patch was originally sent here https://lkml.org/lkml/2014/5/6/5 with my google.com email address. But due to Google's recent change in DMARC policies, that patchset was silently dropped for at least some users (including my personal gmail account). So I'm sending it out now with my gmail.com account. Let me know if this is an issue. Thanks.
drivers/staging/slicoss/slicoss.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/staging/slicoss/slicoss.c +2 -1
@@ -2526,9 +2526,10 @@ static void slic_entry_remove(struct pci_dev *pcidev)struct sliccard *card;struct mcast_address *mcaddr, *mlist; +unregister_netdev (dev); +slic_adapter_freeresources (adapter);slic_unmap_mmio_space (adapter); -unregister_netdev (dev);/* free multicast addresses */ mlist = adapter->mcastaddrs;
Signature · 1 line
1.9.2
PATCH 1/1 staging: slicoss: fix 64-bit isr address bug
In reply to David MatlackFix a bug that only manifests when the physical address of the interrupt status register is >4GB. Specifically, the driver was only telling the device about the lower 32 bits of the ISR. This patch adds the upper 32 bits.
Without this patch, polling the ISR times out whenever the physical address of the ISR is >4GB. This causes the driver to think it failed to fetch config data from the device when it really didn't.
Note: The way 64-bit addresses are handled in this driver needs a rework. There is an entry in the TODO file for this. That will be a rather large patch set which will change a lot of the driver. So for the time being, this patch will at least let the driver run correctly on 64-bit machines with >4GB of physical memory.
---
This patch was originally sent here https://lkml.org/lkml/2014/5/6/6 with my google.com email address. But due to Google's recent change in DMARC policies, that patchset was silently dropped for at least some users (including my personal gmail account). So I'm sending it out now with my gmail.com account. Let me know if this is an issue. Thanks.
drivers/staging/slicoss/slicoss.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/staging/slicoss/slicoss.c +2 -1
@@ -2815,7 +2815,8 @@ static int slic_card_init(struct sliccard *card, struct adapter *adapter)spin_lock_irqsave (&adapter->bit64reglock.lock, adapter->bit64reglock.flags); -slic_reg32_write (&slic_regs->slic_addr_upper,0 ,DONT_FLUSH ); +slic_reg32_write (&slic_regs->slic_addr_upper, +SLIC_GET_ADDR_HIGH (&pshmem->isr),DONT_FLUSH );slic_reg32_write (&slic_regs->slic_isp,SLIC_GET_ADDR_LOW (&pshmem->isr),FLUSH ); spin_unlock_irqrestore(&adapter->bit64reglock.lock,
Signature · 1 line
1.9.2
PATCH 1/1 staging: slicoss: remove gratuitous debug infrastructure
In reply to David MatlackAs per the TODO file, this patch removes the gratuitous debug infrastructure. As an extra incentive for removing this code, the debugfs files are not cleaned up properly. For example, if register_netdev() fails in slic_entry_probe() then all debugfs files get left behind, even after the driver module is unloaded. Touching these files quickly leads to an oops.
The net effect of this patch is that the driver will no longer create files in debugfs.
---
This patch was originally sent here https://lkml.org/lkml/2014/5/6/4 with my google.com email address. But due to Google's recent change in DMARC policies, that patchset was silently dropped for at least some users (including my personal gmail account). So I'm sending it out now with my gmail.com account. Let me know if this is an issue. Thanks.
drivers/staging/slicoss/TODO | 1 - drivers/staging/slicoss/slic.h | 3 - drivers/staging/slicoss/slicoss.c | 450 +------------------------------------- 3 files changed, 3 insertions(+), 451 deletions(-)
drivers/staging/slicoss/TODO +0 -1
@@ -18,7 +18,6 @@ TODO: use ethtool instead - reorder code to elminate use of forward declarations - don't keep private linked list of drivers. - - remove all the gratiutous debug infrastructure - use PCI_DEVICE() - do ethtool correctly using ethtool_ops - NAPI?
drivers/staging/slicoss/slic.h +0 -3
@@ -310,8 +310,6 @@ struct sliccard {u32 loadtimerset;uint config_set;struct slic_config config; -struct dentry *debugfs_dir; -struct dentry *debugfs_cardinfo;struct adapter *master;struct adapter *adapter[SLIC_MAX_PORTS ];struct sliccard *next;
@@ -450,7 +448,6 @@ struct adapter {u32 pingtimerset;struct timer_list loadtimer;u32 loadtimerset; -struct dentry *debugfs_entry;struct slic_spinlock upr_lock;struct slic_spinlock bit64reglock;struct slic_rspqueue rspqueue;
drivers/staging/slicoss/slicoss.c +3 -447
517 lines not on this page · raw message
Signature · 1 line
1.9.2
PATCH 1/1 staging: slicoss: clean up use of dev_err
In reply to David MatlackFirst, don't print pci device information or driver prefixes, this is already printed by dev_err. Next, don't report error messages via dev_err when the failing function already reports all errors via dev_err.
--- drivers/staging/slicoss/slicoss.c | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-)
drivers/staging/slicoss/slicoss.c +9 -19
@@ -395,7 +395,7 @@ static int slic_card_download_gbrcv(struct adapter *adapter) ret =request_firmware (&fw, file, &adapter->pcidev->dev);if (ret) {dev_err (&adapter->pcidev->dev, -"SLICOSS: Failed to load firmware %s\n" , file); +"Failed to load firmware %s\n" , file);return ret; }
@@ -473,7 +473,7 @@ static int slic_card_download(struct adapter *adapter) ret =request_firmware (&fw, file, &adapter->pcidev->dev);if (ret) {dev_err (&adapter->pcidev->dev, -"SLICOSS: Failed to load firmware %s\n" , file); +"Failed to load firmware %s\n" , file);return ret; } numsects = *(u32 *)(fw->data + index);
@@ -2783,13 +2783,8 @@ static int slic_card_init(struct sliccard *card, struct adapter *adapter)/* Download the microcode */ status =slic_card_download (adapter); - -if (status !=0 ) { -dev_err (&adapter->pcidev->dev, -"download failed bus %d slot %d\n" , - adapter->busnumber, adapter->slotnumber); +if (status)return status; - }if (!card->config_set) { peeprom = pci_alloc_consistent(adapter->pcidev,
@@ -2801,8 +2796,7 @@ static int slic_card_init(struct sliccard *card, struct adapter *adapter)if (!peeprom) {dev_err (&adapter->pcidev->dev, -"eeprom read failed to get memory bus %d slot %d\n" , adapter->busnumber, - adapter->slotnumber); +"Failed to allocate DMA memory for EEPROM.\n" );return -ENOMEM ; }else {memset (peeprom,0 ,sizeof (struct slic_eeprom ));
@@ -2846,8 +2840,7 @@ static int slic_card_init(struct sliccard *card, struct adapter *adapter) i++;if (i >5000 ) {dev_err (&adapter->pcidev->dev, -"%d config data fetch timed out!\n" , - adapter->port); +"Fetch of config data timed out.\n" ); slic_reg64_write(adapter, &slic_regs->slic_isp, 0, &slic_regs->slic_addr_upper,
@@ -2931,19 +2924,16 @@ static int slic_card_init(struct sliccard *card, struct adapter *adapter)slic_reg64_write (adapter, &slic_regs->slic_isp,0 , &slic_regs->slic_addr_upper,0 ,FLUSH ); -dev_err (&adapter->pcidev->dev, -"unsupported CONFIGURATION EEPROM invalid\n" ); +dev_err (&adapter->pcidev->dev,"EEPROM invalid.\n" );return -EINVAL ; } card->config_set =1 ; } -if (slic_card_download_gbrcv (adapter)) { -dev_err (&adapter->pcidev->dev, -"unable to download GB receive microcode\n" ); -return -EINVAL ; - } + status =slic_card_download_gbrcv (adapter); +if (status) +return status;if (slic_global.dynamic_intagg)slic_intagg_set (adapter,0 );
Signature · 1 line
1.9.2
Re: [PATCH] staging: slicoss: clean up use of dev_err
Greg, this is the other patch (of two) that got dropped.
Thanks.
PATCH 1/1 staging: slicoss: remove private netdev list
In reply to David MatlackRemove the private linked list of netdev structs. This list isn't being used anyway.
This patch has no noticable effect.
--- drivers/staging/slicoss/slic.h | 1 - drivers/staging/slicoss/slicoss.c | 4 ---- 2 files changed, 5 deletions(-)
drivers/staging/slicoss/slic.h +0 -1
@@ -407,7 +407,6 @@ struct adapter {uint card_size;uint chipid;struct net_device *netdev; -struct net_device *next_netdevice;struct slic_spinlock adapter_lock;struct slic_spinlock reset_lock;struct pci_dev *pcidev;
drivers/staging/slicoss/slicoss.c +0 -4
@@ -104,8 +104,6 @@ static char *slic_banner = "Alacritech SLIC Technology(tm) Server "static char *slic_proc_version ="2.0.351 2006/07/14 12:26:00" ; -static struct net_device *head_netdevice; -static struct base_driver slic_global = { {},0 ,0 ,0 ,1 ,NULL ,NULL };static int intagg_delay =100 ;static u32 dynamic_intagg;
@@ -2993,8 +2991,6 @@ static void slic_init_adapter(struct net_device *netdev, adapter->slic_regs = (__iomem struct slic_regs *)memaddr; adapter->irq = pcidev->irq;/* adapter->netdev = netdev;*/ - adapter->next_netdevice = head_netdevice; - head_netdevice = netdev; adapter->chipid = chip_idx; adapter->port =0 ;/*adapter->functionnumber;*/ adapter->cardindex = adapter->port;
Signature · 1 line
1.9.2
Re: [PATCH] staging: slicoss: remove private netdev list
Greg, this is one of the two patches that got dropped. It should apply cleanly in any order with respect to all my other patches.
Thanks.
Greg KH
maintainerRe: [PATCH] staging: slicoss: remove private netdev list
Quoted 2 lines · David Matlack
Greg, this is one of the two patches that got dropped. It should apply cleanly in any order with respect to all my other patches.
As my patch queue is now flushed out, what is "this" patch? :)
Can you please just resend the two missing patches?
thanks,
greg k-h
PATCH 1/1 staging: slicoss: remove unused members of struct adapter
In reply to David MatlackRemoves two fields from the private "struct adapter".
memorybase duplicate of slic_regs
memorylength written once and never read. This field is trivially
computed with pci_resource_len if it's ever needed in the future.
This patch has no noticable effect.
---
This patch was originally sent here https://lkml.org/lkml/2014/5/6/9 with my google.com email address. But due to Google's recent change in DMARC policies, that patchset was silently dropped for at least some users (including my personal gmail account). So I'm sending it out now with my gmail.com account. Let me know if this is an issue. Thanks.
drivers/staging/slicoss/slic.h | 2 -- drivers/staging/slicoss/slicoss.c | 4 +--- 2 files changed, 1 insertion(+), 5 deletions(-)
drivers/staging/slicoss/slic.h +0 -2
@@ -420,8 +420,6 @@ struct adapter {ushort devid;ushort subsysid;u32 irq; -void __iomem *memorybase; -u32 memorylength;u32 drambase;u32 dramlength;uint queues_initialized;
drivers/staging/slicoss/slicoss.c +1 -3
@@ -3422,7 +3422,6 @@ static void slic_init_adapter(struct net_device *netdev, adapter->busnumber = pcidev->bus->number; adapter->slotnumber = ((pcidev->devfn >>3 ) &0x1F ); adapter->functionnumber = (pcidev->devfn &0x7 ); - adapter->memorylength =pci_resource_len (pcidev,0 ); adapter->slic_regs = (__iomem struct slic_regs *)memaddr; adapter->irq = pcidev->irq;/* adapter->netdev = netdev;*/
@@ -3431,7 +3430,6 @@ static void slic_init_adapter(struct net_device *netdev, adapter->chipid = chip_idx; adapter->port =0 ;/*adapter->functionnumber;*/ adapter->cardindex = adapter->port; - adapter->memorybase = memaddr;spin_lock_init (&adapter->upr_lock.lock);spin_lock_init (&adapter->bit64reglock.lock);spin_lock_init (&adapter->adapter_lock.lock);
@@ -3683,7 +3681,7 @@ static int slic_entry_probe(struct pci_dev *pcidev,slic_adapter_set_hwaddr (adapter); - netdev->base_addr = (unsigned long )adapter->memorybase; + netdev->base_addr = (unsigned long ) memmapped_ioaddr; netdev->irq = adapter->irq; netdev->netdev_ops = &slic_netdev_ops;
Signature · 1 line
1.9.2
Greg KH
maintainerQuoted 5 lines · David Matlack
Fix a use-after-free bug that can cause a kernel oops. If
slic_card_initfails thenslic_entry_probe()(the pciprobe()function for this device) will return error without cleaning up memory (including the registered netdev struct).
Quoted 7 lines
Signed-off-by David Matlack <[email protected]>---
This patch was originally sent here https://lkml.org/lkml/2014/5/6/10 with my google.com email address. But due to Google's recent change in DMARC policies, that patchset was silently dropped for at least some users (including my personal gmail account). So I'm sending it out now with my gmail.com account. Let me know if this is an issue. Thanks.
I think I've already applied all of these, if not, please let me know.
Also, you forgot to number these patches to let me know what order to apply them in :(
thanks,
greg k-h
Quoted 8 lines · Greg KH
I think I've already applied all of these, if not, please let me know.
Also, you forgot to number these patches to let me know what order to apply them in :(
thanks,
greg k-h
Thanks Greg. Sorry for the confusion. I resent these patches without numbers because in retrospect, they were all independent changes and could be applied in any order. In retro-retrospect, that probably just piled onto the confusion :)
Looking at the emails I got from you about staging-next, _almost_ all the patches were applied. I reply to the ones directly that were dropped.
Thanks!