Messages in this thread | | | Date | Tue, 20 Jul 2021 22:37:57 +0800 (GMT+08:00) | From | lyl2019@mail ... | Subject | Re: Doubts about Patch "ipack/carriers/tpci200: Fix a double free in tpci200_pci_probe" |
| |
Hello Dongliang Mu,
> Depending on if CONFIG_PCI defines, the "tpci200->info->cfg_regs" may > not be freed. > > #ifdef CONFIG_PCI > /* Destroy a virtual mapping cookie for a PCI BAR (memory or IO) */ > struct pci_dev; > extern void pci_iounmap(struct pci_dev *dev, void __iomem *); > #elif defined(CONFIG_GENERIC_IOMAP) > struct pci_dev; > static inline void pci_iounmap(struct pci_dev *dev, void __iomem *addr) > { } > #endif
I think only `CONFIG_PCI=n` and `CONFIG_GENERIC_IOMAP=y` cause pci_iounmap an empty implementation. Actually, `CONFIG_PCI` is a default option when run `make defconfig`, pci_iounmap() usually is acted as an extern function.
> Even if CONFIG_PCI is undefined, it is possible that > tpci200->info->cfg_regs is not freed at all. Therefore, this patch > would cause memory leak. Take a look at the following code: > > void pci_iounmap(struct pci_dev *dev, void __iomem * addr) > { > IO_COND(addr, /* nothing */, iounmap(addr)); > }
Here i am not sure this is the final implementation of pci_iounmap(), because pci_iounmap() is re-implementated in many architectures. Even so, i observed there still many call-sites of pci_iounmap() have reset `the addr = NULL` after calling. Can you have some ways to determine the actual implementation of pci_iounmap in our cases?
> #define IO_COND(addr, is_pio, is_mmio) do { \ > unsigned long port = (unsigned long __force)addr; \ > if (port >= PIO_RESERVED) { \ > is_mmio; \ > } else if (port > PIO_OFFSET) { \ > port &= PIO_MASK; \ > is_pio; \ > } else \ > bad_io_access(port, #is_pio ); \ > } while (0) >
Although the above codes is actually called, the addr might be freed if `port >= PIO_RESERVED` is true. The double free still existed.
If I make any mistakes, please tell me. Thanks your report. --- Lv Yunlong
| |