linux-kernel - Re: Doubts about Patch "ipack/carriers/tpci200: Fix a double free in tpci200_pci

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]

Message-ID: <77df3e1f.22e1.17ac45a1bd9.Coremail.lyl2019@mail.ustc.edu.cn>
Date:   Tue, 20 Jul 2021 22:37:57 +0800 (GMT+08:00)
From:   lyl2019@...l.ustc.edu.cn
To:     "Dongliang Mu" <mudongliangabcd@...il.com>
Cc:     siglesias@...lia.com, jens.taprogge@...rogge.org,
        "Greg KH" <gregkh@...uxfoundation.org>,
        industrypack-devel@...ts.sourceforge.net,
        linux-kernel <linux-kernel@...r.kernel.org>
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