A race condition may occur if the user physically removes the
hpilo device while calling open().
This is a race condition between ilo_open() function and
the ilo_remove() function, which may lead to Use-After-Free.
Therefore, add a refcount check to ilo_remove() function
to free the "ilo_hwinfo" structure after the device is close()d.
---------------CPU 0--------------------CPU 1-----------------
ilo_open() | ilo_remove()
--------------------------------------------------------------
hw = container_of(ip->i_cdev, |
struct ilo_hwinfo, cdev); -(1)|
| struct ilo_hwinfo *ilo_hw = pci_get_drvdata(pdev);
| ...
| kfree(ilo_hw); -(2)
spin_lock(&hw->open_lock);-(3)|
Signed-off-by: Yoochan Lee <[email protected]>
---
drivers/misc/hpilo.c | 53 ++++++++++++++++++++++++++------------------
drivers/misc/hpilo.h | 1 +
2 files changed, 33 insertions(+), 21 deletions(-)
diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
index 8d00df9243c4..b4a99676a642 100644
--- a/drivers/misc/hpilo.c
+++ b/drivers/misc/hpilo.c
@@ -532,6 +532,34 @@ static __poll_t ilo_poll(struct file *fp, poll_table *wait)
return 0;
}
+static void ilo_delete(struct kref *kref)
+{
+ struct ilo_hwinfo *ilo_hw = container_of(kref, struct ilo_hwinfo, refcnt);
+
+ clear_device(ilo_hw);
+
+ minor = MINOR(ilo_hw->cdev.dev);
+ for (i = minor; i < minor + max_ccb; i++)
+ device_destroy(ilo_class, MKDEV(ilo_major, i));
+
+ cdev_del(&ilo_hw->cdev);
+ ilo_disable_interrupts(ilo_hw);
+ free_irq(pdev->irq, ilo_hw);
+ ilo_unmap_device(pdev, ilo_hw);
+ pci_release_regions(pdev);
+ /*
+ * pci_disable_device(pdev) used to be here. But this PCI device has
+ * two functions with interrupt lines connected to a single pin. The
+ * other one is a USB host controller. So when we disable the PIN here
+ * e.g. by rmmod hpilo, the controller stops working. It is because
+ * the interrupt link is disabled in ACPI since it is not refcounted
+ * yet. See acpi_pci_link_free_irq called from acpi_pci_irq_disable.
+ */
+ kfree(ilo_hw);
+ ilo_hwdev[(minor / max_ccb)] = 0;
+
+}
+
static int ilo_close(struct inode *ip, struct file *fp)
{
int slot;
@@ -558,6 +586,7 @@ static int ilo_close(struct inode *ip, struct file *fp)
} else
hw->ccb_alloc[slot]->ccb_cnt--;
+ kref_put(&hw->refcnt, ilo_delete);
spin_unlock(&hw->open_lock);
return 0;
@@ -629,6 +658,7 @@ static int ilo_open(struct inode *ip, struct file *fp)
}
}
out:
+ kref_get(&hw->refcnt);
spin_unlock(&hw->open_lock);
if (!error)
@@ -748,27 +778,7 @@ static void ilo_remove(struct pci_dev *pdev)
if (!ilo_hw)
return;
- clear_device(ilo_hw);
-
- minor = MINOR(ilo_hw->cdev.dev);
- for (i = minor; i < minor + max_ccb; i++)
- device_destroy(ilo_class, MKDEV(ilo_major, i));
-
- cdev_del(&ilo_hw->cdev);
- ilo_disable_interrupts(ilo_hw);
- free_irq(pdev->irq, ilo_hw);
- ilo_unmap_device(pdev, ilo_hw);
- pci_release_regions(pdev);
- /*
- * pci_disable_device(pdev) used to be here. But this PCI device has
- * two functions with interrupt lines connected to a single pin. The
- * other one is a USB host controller. So when we disable the PIN here
- * e.g. by rmmod hpilo, the controller stops working. It is because
- * the interrupt link is disabled in ACPI since it is not refcounted
- * yet. See acpi_pci_link_free_irq called from acpi_pci_irq_disable.
- */
- kfree(ilo_hw);
- ilo_hwdev[(minor / max_ccb)] = 0;
+ kref_put(&hw->refcnt, ilo_delete);
}
static int ilo_probe(struct pci_dev *pdev,
@@ -810,6 +820,7 @@ static int ilo_probe(struct pci_dev *pdev,
spin_lock_init(&ilo_hw->alloc_lock);
spin_lock_init(&ilo_hw->fifo_lock);
spin_lock_init(&ilo_hw->open_lock);
+ kref_init(&iol_hw->refcnt);
error = pci_enable_device(pdev);
if (error)
diff --git a/drivers/misc/hpilo.h b/drivers/misc/hpilo.h
index d57c34680b09..ebc677eb45ae 100644
--- a/drivers/misc/hpilo.h
+++ b/drivers/misc/hpilo.h
@@ -62,6 +62,7 @@ struct ilo_hwinfo {
spinlock_t fifo_lock;
struct cdev cdev;
+ struct kref refcnt;
};
/* offset from mmio_vaddr for enabling doorbell interrupts */
--
2.39.0
Hi, there is some typos in my patch.
Therefore, here are additional patches.
diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
index b4a99676a642..ea5bfbb231bd 100644
--- a/drivers/misc/hpilo.c
+++ b/drivers/misc/hpilo.c
@@ -778,7 +778,7 @@ static void ilo_remove(struct pci_dev *pdev)
if (!ilo_hw)
return;
- kref_put(&hw->refcnt, ilo_delete);
+ kref_put(&ilo_hw->refcnt, ilo_delete);
}
static int ilo_probe(struct pci_dev *pdev,
@@ -820,7 +820,7 @@ static int ilo_probe(struct pci_dev *pdev,
spin_lock_init(&ilo_hw->alloc_lock);
spin_lock_init(&ilo_hw->fifo_lock);
spin_lock_init(&ilo_hw->open_lock);
- kref_init(&iol_hw->refcnt);
+ kref_init(&ilo_hw->refcnt);
error = pci_enable_device(pdev);
if (error)
Sincerely,
Yoocahn
2022년 12월 31일 (토) 오후 5:51, kernel test robot <[email protected]>님이 작성:
>
> Hi Yoochan,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on char-misc/char-misc-testing]
> [also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus soc/for-next linus/master v6.2-rc1 next-20221226]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Yoochan-Lee/misc-hpilo-Fix-use-after-free-in-ilo_open/20221231-135458
> patch link: https://lore.kernel.org/r/20221231055310.2040648-1-yoochan1026%40gmail.com
> patch subject: [PATCH] misc: hpilo: Fix use-after-free in ilo_open
> config: x86_64-rhel-8.3-bpf
> compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
> reproduce (this is a W=1 build):
> # https://github.com/intel-lab-lkp/linux/commit/aca13e7e71e5c2b68742270a834fd67929850ef9
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Yoochan-Lee/misc-hpilo-Fix-use-after-free-in-ilo_open/20221231-135458
> git checkout aca13e7e71e5c2b68742270a834fd67929850ef9
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> make W=1 O=build_dir ARCH=x86_64 olddefconfig
> make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/misc/
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
>
> All warnings (new ones prefixed by >>):
>
> drivers/misc/hpilo.c: In function 'ilo_delete':
> drivers/misc/hpilo.c:541:9: error: 'minor' undeclared (first use in this function); did you mean 'iminor'?
> 541 | minor = MINOR(ilo_hw->cdev.dev);
> | ^~~~~
> | iminor
> drivers/misc/hpilo.c:541:9: note: each undeclared identifier is reported only once for each function it appears in
> drivers/misc/hpilo.c:542:14: error: 'i' undeclared (first use in this function)
> 542 | for (i = minor; i < minor + max_ccb; i++)
> | ^
> drivers/misc/hpilo.c:547:18: error: 'pdev' undeclared (first use in this function); did you mean 'cdev'?
> 547 | free_irq(pdev->irq, ilo_hw);
> | ^~~~
> | cdev
> drivers/misc/hpilo.c:548:9: error: implicit declaration of function 'ilo_unmap_device' [-Werror=implicit-function-declaration]
> 548 | ilo_unmap_device(pdev, ilo_hw);
> | ^~~~~~~~~~~~~~~~
> drivers/misc/hpilo.c: At top level:
> drivers/misc/hpilo.c:715:13: warning: conflicting types for 'ilo_unmap_device'; have 'void(struct pci_dev *, struct ilo_hwinfo *)'
> 715 | static void ilo_unmap_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
> | ^~~~~~~~~~~~~~~~
> drivers/misc/hpilo.c:715:13: error: static declaration of 'ilo_unmap_device' follows non-static declaration
> drivers/misc/hpilo.c:548:9: note: previous implicit declaration of 'ilo_unmap_device' with type 'void(struct pci_dev *, struct ilo_hwinfo *)'
> 548 | ilo_unmap_device(pdev, ilo_hw);
> | ^~~~~~~~~~~~~~~~
> drivers/misc/hpilo.c: In function 'ilo_remove':
> drivers/misc/hpilo.c:781:19: error: 'hw' undeclared (first use in this function)
> 781 | kref_put(&hw->refcnt, ilo_delete);
> | ^~
> >> drivers/misc/hpilo.c:775:16: warning: unused variable 'minor' [-Wunused-variable]
> 775 | int i, minor;
> | ^~~~~
> >> drivers/misc/hpilo.c:775:13: warning: unused variable 'i' [-Wunused-variable]
> 775 | int i, minor;
> | ^
> drivers/misc/hpilo.c: In function 'ilo_probe':
> drivers/misc/hpilo.c:823:20: error: 'iol_hw' undeclared (first use in this function); did you mean 'ilo_hw'?
> 823 | kref_init(&iol_hw->refcnt);
> | ^~~~~~
> | ilo_hw
> cc1: some warnings being treated as errors
>
>
> vim +/minor +775 drivers/misc/hpilo.c
>
> 9f7048412163d8 David Altobelli 2009-08-17 714
> 89bcb05d9bbf8b David Altobelli 2008-07-02 @715 static void ilo_unmap_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
> 89bcb05d9bbf8b David Altobelli 2008-07-02 716 {
> 89bcb05d9bbf8b David Altobelli 2008-07-02 717 pci_iounmap(pdev, hw->db_vaddr);
> 89bcb05d9bbf8b David Altobelli 2008-07-02 718 pci_iounmap(pdev, hw->ram_vaddr);
> 89bcb05d9bbf8b David Altobelli 2008-07-02 719 pci_iounmap(pdev, hw->mmio_vaddr);
> 89bcb05d9bbf8b David Altobelli 2008-07-02 720 }
> 89bcb05d9bbf8b David Altobelli 2008-07-02 721
> 80c8ae28926652 Bill Pemberton 2012-11-19 722 static int ilo_map_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
> 89bcb05d9bbf8b David Altobelli 2008-07-02 723 {
> c9fef1cc3dd367 Rusk, Mark 2016-09-19 724 int bar;
> c9fef1cc3dd367 Rusk, Mark 2016-09-19 725 unsigned long off;
> 23d51b81815127 Matt Hsiao 2021-05-31 726 u8 pci_rev_id;
> 23d51b81815127 Matt Hsiao 2021-05-31 727 int rc;
> 89bcb05d9bbf8b David Altobelli 2008-07-02 728
> 89bcb05d9bbf8b David Altobelli 2008-07-02 729 /* map the memory mapped i/o registers */
> 89bcb05d9bbf8b David Altobelli 2008-07-02 730 hw->mmio_vaddr = pci_iomap(pdev, 1, 0);
> 89bcb05d9bbf8b David Altobelli 2008-07-02 731 if (hw->mmio_vaddr == NULL) {
> 89bcb05d9bbf8b David Altobelli 2008-07-02 732 dev_err(&pdev->dev, "Error mapping mmio\n");
> 89bcb05d9bbf8b David Altobelli 2008-07-02 733 goto out;
> 89bcb05d9bbf8b David Altobelli 2008-07-02 734 }
> 89bcb05d9bbf8b David Altobelli 2008-07-02 735
> 89bcb05d9bbf8b David Altobelli 2008-07-02 736 /* map the adapter shared memory region */
> 23d51b81815127 Matt Hsiao 2021-05-31 737 rc = pci_read_config_byte(pdev, PCI_REVISION_ID, &pci_rev_id);
> 23d51b81815127 Matt Hsiao 2021-05-31 738 if (rc != 0) {
> 23d51b81815127 Matt Hsiao 2021-05-31 739 dev_err(&pdev->dev, "Error reading PCI rev id: %d\n", rc);
> 23d51b81815127 Matt Hsiao 2021-05-31 740 goto out;
> 23d51b81815127 Matt Hsiao 2021-05-31 741 }
> 23d51b81815127 Matt Hsiao 2021-05-31 742
> 23d51b81815127 Matt Hsiao 2021-05-31 743 if (pci_rev_id >= PCI_REV_ID_NECHES) {
> c9fef1cc3dd367 Rusk, Mark 2016-09-19 744 bar = 5;
> c9fef1cc3dd367 Rusk, Mark 2016-09-19 745 /* Last 8k is reserved for CCBs */
> c9fef1cc3dd367 Rusk, Mark 2016-09-19 746 off = pci_resource_len(pdev, bar) - 0x2000;
> c9fef1cc3dd367 Rusk, Mark 2016-09-19 747 } else {
> c9fef1cc3dd367 Rusk, Mark 2016-09-19 748 bar = 2;
> c9fef1cc3dd367 Rusk, Mark 2016-09-19 749 off = 0;
> c9fef1cc3dd367 Rusk, Mark 2016-09-19 750 }
> c9fef1cc3dd367 Rusk, Mark 2016-09-19 751 hw->ram_vaddr = pci_iomap_range(pdev, bar, off, max_ccb * ILOHW_CCB_SZ);
> 89bcb05d9bbf8b David Altobelli 2008-07-02 752 if (hw->ram_vaddr == NULL) {
> 89bcb05d9bbf8b David Altobelli 2008-07-02 753 dev_err(&pdev->dev, "Error mapping shared mem\n");
> 89bcb05d9bbf8b David Altobelli 2008-07-02 754 goto mmio_free;
> 89bcb05d9bbf8b David Altobelli 2008-07-02 755 }
> 89bcb05d9bbf8b David Altobelli 2008-07-02 756
> 89bcb05d9bbf8b David Altobelli 2008-07-02 757 /* map the doorbell aperture */
> 98dcd59dd063dd Camuso, Tony 2012-06-10 758 hw->db_vaddr = pci_iomap(pdev, 3, max_ccb * ONE_DB_SIZE);
> 89bcb05d9bbf8b David Altobelli 2008-07-02 759 if (hw->db_vaddr == NULL) {
> 89bcb05d9bbf8b David Altobelli 2008-07-02 760 dev_err(&pdev->dev, "Error mapping doorbell\n");
> 89bcb05d9bbf8b David Altobelli 2008-07-02 761 goto ram_free;
> 89bcb05d9bbf8b David Altobelli 2008-07-02 762 }
> 89bcb05d9bbf8b David Altobelli 2008-07-02 763
> 89bcb05d9bbf8b David Altobelli 2008-07-02 764 return 0;
> 89bcb05d9bbf8b David Altobelli 2008-07-02 765 ram_free:
> 89bcb05d9bbf8b David Altobelli 2008-07-02 766 pci_iounmap(pdev, hw->ram_vaddr);
> 89bcb05d9bbf8b David Altobelli 2008-07-02 767 mmio_free:
> 89bcb05d9bbf8b David Altobelli 2008-07-02 768 pci_iounmap(pdev, hw->mmio_vaddr);
> 89bcb05d9bbf8b David Altobelli 2008-07-02 769 out:
> c9fef1cc3dd367 Rusk, Mark 2016-09-19 770 return -ENOMEM;
> 89bcb05d9bbf8b David Altobelli 2008-07-02 771 }
> 89bcb05d9bbf8b David Altobelli 2008-07-02 772
> 89bcb05d9bbf8b David Altobelli 2008-07-02 773 static void ilo_remove(struct pci_dev *pdev)
> 89bcb05d9bbf8b David Altobelli 2008-07-02 774 {
> 89bcb05d9bbf8b David Altobelli 2008-07-02 @775 int i, minor;
> 89bcb05d9bbf8b David Altobelli 2008-07-02 776 struct ilo_hwinfo *ilo_hw = pci_get_drvdata(pdev);
> 89bcb05d9bbf8b David Altobelli 2008-07-02 777
> ebf1b764aa5cb3 Mark Rusk 2012-11-06 778 if (!ilo_hw)
> ebf1b764aa5cb3 Mark Rusk 2012-11-06 779 return;
> ebf1b764aa5cb3 Mark Rusk 2012-11-06 780
> aca13e7e71e5c2 Yoochan Lee 2022-12-31 @781 kref_put(&hw->refcnt, ilo_delete);
> 89bcb05d9bbf8b David Altobelli 2008-07-02 782 }
> 89bcb05d9bbf8b David Altobelli 2008-07-02 783
>
> --
> 0-DAY CI Kernel Test Service
> https://01.org/lkp
Hi Yoochan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus soc/for-next linus/master v6.2-rc1 next-20221226]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yoochan-Lee/misc-hpilo-Fix-use-after-free-in-ilo_open/20221231-135458
patch link: https://lore.kernel.org/r/20221231055310.2040648-1-yoochan1026%40gmail.com
patch subject: [PATCH] misc: hpilo: Fix use-after-free in ilo_open
config: x86_64-rhel-8.3-bpf
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/aca13e7e71e5c2b68742270a834fd67929850ef9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yoochan-Lee/misc-hpilo-Fix-use-after-free-in-ilo_open/20221231-135458
git checkout aca13e7e71e5c2b68742270a834fd67929850ef9
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/misc/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/misc/hpilo.c: In function 'ilo_delete':
drivers/misc/hpilo.c:541:9: error: 'minor' undeclared (first use in this function); did you mean 'iminor'?
541 | minor = MINOR(ilo_hw->cdev.dev);
| ^~~~~
| iminor
drivers/misc/hpilo.c:541:9: note: each undeclared identifier is reported only once for each function it appears in
drivers/misc/hpilo.c:542:14: error: 'i' undeclared (first use in this function)
542 | for (i = minor; i < minor + max_ccb; i++)
| ^
drivers/misc/hpilo.c:547:18: error: 'pdev' undeclared (first use in this function); did you mean 'cdev'?
547 | free_irq(pdev->irq, ilo_hw);
| ^~~~
| cdev
drivers/misc/hpilo.c:548:9: error: implicit declaration of function 'ilo_unmap_device' [-Werror=implicit-function-declaration]
548 | ilo_unmap_device(pdev, ilo_hw);
| ^~~~~~~~~~~~~~~~
drivers/misc/hpilo.c: At top level:
drivers/misc/hpilo.c:715:13: warning: conflicting types for 'ilo_unmap_device'; have 'void(struct pci_dev *, struct ilo_hwinfo *)'
715 | static void ilo_unmap_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
| ^~~~~~~~~~~~~~~~
drivers/misc/hpilo.c:715:13: error: static declaration of 'ilo_unmap_device' follows non-static declaration
drivers/misc/hpilo.c:548:9: note: previous implicit declaration of 'ilo_unmap_device' with type 'void(struct pci_dev *, struct ilo_hwinfo *)'
548 | ilo_unmap_device(pdev, ilo_hw);
| ^~~~~~~~~~~~~~~~
drivers/misc/hpilo.c: In function 'ilo_remove':
drivers/misc/hpilo.c:781:19: error: 'hw' undeclared (first use in this function)
781 | kref_put(&hw->refcnt, ilo_delete);
| ^~
>> drivers/misc/hpilo.c:775:16: warning: unused variable 'minor' [-Wunused-variable]
775 | int i, minor;
| ^~~~~
>> drivers/misc/hpilo.c:775:13: warning: unused variable 'i' [-Wunused-variable]
775 | int i, minor;
| ^
drivers/misc/hpilo.c: In function 'ilo_probe':
drivers/misc/hpilo.c:823:20: error: 'iol_hw' undeclared (first use in this function); did you mean 'ilo_hw'?
823 | kref_init(&iol_hw->refcnt);
| ^~~~~~
| ilo_hw
cc1: some warnings being treated as errors
vim +/minor +775 drivers/misc/hpilo.c
9f7048412163d8 David Altobelli 2009-08-17 714
89bcb05d9bbf8b David Altobelli 2008-07-02 @715 static void ilo_unmap_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
89bcb05d9bbf8b David Altobelli 2008-07-02 716 {
89bcb05d9bbf8b David Altobelli 2008-07-02 717 pci_iounmap(pdev, hw->db_vaddr);
89bcb05d9bbf8b David Altobelli 2008-07-02 718 pci_iounmap(pdev, hw->ram_vaddr);
89bcb05d9bbf8b David Altobelli 2008-07-02 719 pci_iounmap(pdev, hw->mmio_vaddr);
89bcb05d9bbf8b David Altobelli 2008-07-02 720 }
89bcb05d9bbf8b David Altobelli 2008-07-02 721
80c8ae28926652 Bill Pemberton 2012-11-19 722 static int ilo_map_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
89bcb05d9bbf8b David Altobelli 2008-07-02 723 {
c9fef1cc3dd367 Rusk, Mark 2016-09-19 724 int bar;
c9fef1cc3dd367 Rusk, Mark 2016-09-19 725 unsigned long off;
23d51b81815127 Matt Hsiao 2021-05-31 726 u8 pci_rev_id;
23d51b81815127 Matt Hsiao 2021-05-31 727 int rc;
89bcb05d9bbf8b David Altobelli 2008-07-02 728
89bcb05d9bbf8b David Altobelli 2008-07-02 729 /* map the memory mapped i/o registers */
89bcb05d9bbf8b David Altobelli 2008-07-02 730 hw->mmio_vaddr = pci_iomap(pdev, 1, 0);
89bcb05d9bbf8b David Altobelli 2008-07-02 731 if (hw->mmio_vaddr == NULL) {
89bcb05d9bbf8b David Altobelli 2008-07-02 732 dev_err(&pdev->dev, "Error mapping mmio\n");
89bcb05d9bbf8b David Altobelli 2008-07-02 733 goto out;
89bcb05d9bbf8b David Altobelli 2008-07-02 734 }
89bcb05d9bbf8b David Altobelli 2008-07-02 735
89bcb05d9bbf8b David Altobelli 2008-07-02 736 /* map the adapter shared memory region */
23d51b81815127 Matt Hsiao 2021-05-31 737 rc = pci_read_config_byte(pdev, PCI_REVISION_ID, &pci_rev_id);
23d51b81815127 Matt Hsiao 2021-05-31 738 if (rc != 0) {
23d51b81815127 Matt Hsiao 2021-05-31 739 dev_err(&pdev->dev, "Error reading PCI rev id: %d\n", rc);
23d51b81815127 Matt Hsiao 2021-05-31 740 goto out;
23d51b81815127 Matt Hsiao 2021-05-31 741 }
23d51b81815127 Matt Hsiao 2021-05-31 742
23d51b81815127 Matt Hsiao 2021-05-31 743 if (pci_rev_id >= PCI_REV_ID_NECHES) {
c9fef1cc3dd367 Rusk, Mark 2016-09-19 744 bar = 5;
c9fef1cc3dd367 Rusk, Mark 2016-09-19 745 /* Last 8k is reserved for CCBs */
c9fef1cc3dd367 Rusk, Mark 2016-09-19 746 off = pci_resource_len(pdev, bar) - 0x2000;
c9fef1cc3dd367 Rusk, Mark 2016-09-19 747 } else {
c9fef1cc3dd367 Rusk, Mark 2016-09-19 748 bar = 2;
c9fef1cc3dd367 Rusk, Mark 2016-09-19 749 off = 0;
c9fef1cc3dd367 Rusk, Mark 2016-09-19 750 }
c9fef1cc3dd367 Rusk, Mark 2016-09-19 751 hw->ram_vaddr = pci_iomap_range(pdev, bar, off, max_ccb * ILOHW_CCB_SZ);
89bcb05d9bbf8b David Altobelli 2008-07-02 752 if (hw->ram_vaddr == NULL) {
89bcb05d9bbf8b David Altobelli 2008-07-02 753 dev_err(&pdev->dev, "Error mapping shared mem\n");
89bcb05d9bbf8b David Altobelli 2008-07-02 754 goto mmio_free;
89bcb05d9bbf8b David Altobelli 2008-07-02 755 }
89bcb05d9bbf8b David Altobelli 2008-07-02 756
89bcb05d9bbf8b David Altobelli 2008-07-02 757 /* map the doorbell aperture */
98dcd59dd063dd Camuso, Tony 2012-06-10 758 hw->db_vaddr = pci_iomap(pdev, 3, max_ccb * ONE_DB_SIZE);
89bcb05d9bbf8b David Altobelli 2008-07-02 759 if (hw->db_vaddr == NULL) {
89bcb05d9bbf8b David Altobelli 2008-07-02 760 dev_err(&pdev->dev, "Error mapping doorbell\n");
89bcb05d9bbf8b David Altobelli 2008-07-02 761 goto ram_free;
89bcb05d9bbf8b David Altobelli 2008-07-02 762 }
89bcb05d9bbf8b David Altobelli 2008-07-02 763
89bcb05d9bbf8b David Altobelli 2008-07-02 764 return 0;
89bcb05d9bbf8b David Altobelli 2008-07-02 765 ram_free:
89bcb05d9bbf8b David Altobelli 2008-07-02 766 pci_iounmap(pdev, hw->ram_vaddr);
89bcb05d9bbf8b David Altobelli 2008-07-02 767 mmio_free:
89bcb05d9bbf8b David Altobelli 2008-07-02 768 pci_iounmap(pdev, hw->mmio_vaddr);
89bcb05d9bbf8b David Altobelli 2008-07-02 769 out:
c9fef1cc3dd367 Rusk, Mark 2016-09-19 770 return -ENOMEM;
89bcb05d9bbf8b David Altobelli 2008-07-02 771 }
89bcb05d9bbf8b David Altobelli 2008-07-02 772
89bcb05d9bbf8b David Altobelli 2008-07-02 773 static void ilo_remove(struct pci_dev *pdev)
89bcb05d9bbf8b David Altobelli 2008-07-02 774 {
89bcb05d9bbf8b David Altobelli 2008-07-02 @775 int i, minor;
89bcb05d9bbf8b David Altobelli 2008-07-02 776 struct ilo_hwinfo *ilo_hw = pci_get_drvdata(pdev);
89bcb05d9bbf8b David Altobelli 2008-07-02 777
ebf1b764aa5cb3 Mark Rusk 2012-11-06 778 if (!ilo_hw)
ebf1b764aa5cb3 Mark Rusk 2012-11-06 779 return;
ebf1b764aa5cb3 Mark Rusk 2012-11-06 780
aca13e7e71e5c2 Yoochan Lee 2022-12-31 @781 kref_put(&hw->refcnt, ilo_delete);
89bcb05d9bbf8b David Altobelli 2008-07-02 782 }
89bcb05d9bbf8b David Altobelli 2008-07-02 783
--
0-DAY CI Kernel Test Service
https://01.org/lkp
I missed the error in my patches.
Here is fixed version of patches.
From 8e0501f6c9dd3925cacbb5fdc74548daf276fb04 Mon Sep 17 00:00:00 2001
From: Yoochan Lee <[email protected]>
Date: Sat, 31 Dec 2022 18:22:23 +0900
Subject: [PATCH 2/2] fix error in previous patches
---
drivers/misc/hpilo.c | 68 ++++++++++++++++++++++++--------------------
drivers/misc/hpilo.h | 1 +
2 files changed, 38 insertions(+), 31 deletions(-)
diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
index b4a99676a642..d6691cbedb25 100644
--- a/drivers/misc/hpilo.c
+++ b/drivers/misc/hpilo.c
@@ -37,6 +37,7 @@ static const struct pci_device_id ilo_blacklist[] = {
{}
};
+static void ilo_delete(struct kref *kref);
static inline int get_entry_id(int entry)
{
return (entry & ENTRY_MASK_DESCRIPTOR) >> ENTRY_BITPOS_DESCRIPTOR;
@@ -532,34 +533,6 @@ static __poll_t ilo_poll(struct file *fp, poll_table *wait)
return 0;
}
-static void ilo_delete(struct kref *kref)
-{
- struct ilo_hwinfo *ilo_hw = container_of(kref, struct ilo_hwinfo, refcnt);
-
- clear_device(ilo_hw);
-
- minor = MINOR(ilo_hw->cdev.dev);
- for (i = minor; i < minor + max_ccb; i++)
- device_destroy(ilo_class, MKDEV(ilo_major, i));
-
- cdev_del(&ilo_hw->cdev);
- ilo_disable_interrupts(ilo_hw);
- free_irq(pdev->irq, ilo_hw);
- ilo_unmap_device(pdev, ilo_hw);
- pci_release_regions(pdev);
- /*
- * pci_disable_device(pdev) used to be here. But this PCI device has
- * two functions with interrupt lines connected to a single pin. The
- * other one is a USB host controller. So when we disable the PIN here
- * e.g. by rmmod hpilo, the controller stops working. It is because
- * the interrupt link is disabled in ACPI since it is not refcounted
- * yet. See acpi_pci_link_free_irq called from acpi_pci_irq_disable.
- */
- kfree(ilo_hw);
- ilo_hwdev[(minor / max_ccb)] = 0;
-
-}
-
static int ilo_close(struct inode *ip, struct file *fp)
{
int slot;
@@ -719,6 +692,39 @@ static void ilo_unmap_device(struct pci_dev
*pdev, struct ilo_hwinfo *hw)
pci_iounmap(pdev, hw->mmio_vaddr);
}
+static void ilo_delete(struct kref *kref)
+{
+ int i, minor;
+ struct ilo_hwinfo *ilo_hw = container_of(kref, struct ilo_hwinfo, refcnt);
+ struct pci_dev *pdev = ilo_hw->pdev;
+
+ if(!ilo_hw)
+ return;
+
+ clear_device(ilo_hw);
+
+ minor = MINOR(ilo_hw->cdev.dev);
+ for (i = minor; i < minor + max_ccb; i++)
+ device_destroy(ilo_class, MKDEV(ilo_major, i));
+
+ cdev_del(&ilo_hw->cdev);
+ ilo_disable_interrupts(ilo_hw);
+ free_irq(pdev->irq, ilo_hw);
+ ilo_unmap_device(pdev, ilo_hw);
+ pci_release_regions(pdev);
+ /*
+ * pci_disable_device(pdev) used to be here. But this PCI device has
+ * two functions with interrupt lines connected to a single pin. The
+ * other one is a USB host controller. So when we disable the PIN here
+ * e.g. by rmmod hpilo, the controller stops working. It is because
+ * the interrupt link is disabled in ACPI since it is not refcounted
+ * yet. See acpi_pci_link_free_irq called from acpi_pci_irq_disable.
+ */
+ kfree(ilo_hw);
+ ilo_hwdev[(minor / max_ccb)] = 0;
+
+}
+
static int ilo_map_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
{
int bar;
@@ -772,13 +778,12 @@ static int ilo_map_device(struct pci_dev *pdev,
struct ilo_hwinfo *hw)
static void ilo_remove(struct pci_dev *pdev)
{
- int i, minor;
struct ilo_hwinfo *ilo_hw = pci_get_drvdata(pdev);
if (!ilo_hw)
return;
- kref_put(&hw->refcnt, ilo_delete);
+ kref_put(&ilo_hw->refcnt, ilo_delete);
}
static int ilo_probe(struct pci_dev *pdev,
@@ -820,7 +825,8 @@ static int ilo_probe(struct pci_dev *pdev,
spin_lock_init(&ilo_hw->alloc_lock);
spin_lock_init(&ilo_hw->fifo_lock);
spin_lock_init(&ilo_hw->open_lock);
- kref_init(&iol_hw->refcnt);
+ kref_init(&ilo_hw->refcnt);
+ ilo_hw->pdev = pdev;
error = pci_enable_device(pdev);
if (error)
diff --git a/drivers/misc/hpilo.h b/drivers/misc/hpilo.h
index ebc677eb45ae..f8a857fb93eb 100644
--- a/drivers/misc/hpilo.h
+++ b/drivers/misc/hpilo.h
@@ -62,6 +62,7 @@ struct ilo_hwinfo {
spinlock_t fifo_lock;
struct cdev cdev;
+ struct pci_dev *pdev;
struct kref refcnt;
};
--
2.39.0
2022년 12월 31일 (토) 오후 5:56, Yoochan Lee <[email protected]>님이 작성:
>
> Hi, there is some typos in my patch.
>
> Therefore, here are additional patches.
>
> diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
> index b4a99676a642..ea5bfbb231bd 100644
> --- a/drivers/misc/hpilo.c
> +++ b/drivers/misc/hpilo.c
> @@ -778,7 +778,7 @@ static void ilo_remove(struct pci_dev *pdev)
> if (!ilo_hw)
> return;
>
> - kref_put(&hw->refcnt, ilo_delete);
> + kref_put(&ilo_hw->refcnt, ilo_delete);
> }
>
> static int ilo_probe(struct pci_dev *pdev,
> @@ -820,7 +820,7 @@ static int ilo_probe(struct pci_dev *pdev,
> spin_lock_init(&ilo_hw->alloc_lock);
> spin_lock_init(&ilo_hw->fifo_lock);
> spin_lock_init(&ilo_hw->open_lock);
> - kref_init(&iol_hw->refcnt);
> + kref_init(&ilo_hw->refcnt);
>
> error = pci_enable_device(pdev);
> if (error)
>
> Sincerely,
> Yoocahn
>
>
> 2022년 12월 31일 (토) 오후 5:51, kernel test robot <[email protected]>님이 작성:
> >
> > Hi Yoochan,
> >
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on char-misc/char-misc-testing]
> > [also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus soc/for-next linus/master v6.2-rc1 next-20221226]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url: https://github.com/intel-lab-lkp/linux/commits/Yoochan-Lee/misc-hpilo-Fix-use-after-free-in-ilo_open/20221231-135458
> > patch link: https://lore.kernel.org/r/20221231055310.2040648-1-yoochan1026%40gmail.com
> > patch subject: [PATCH] misc: hpilo: Fix use-after-free in ilo_open
> > config: x86_64-rhel-8.3-bpf
> > compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
> > reproduce (this is a W=1 build):
> > # https://github.com/intel-lab-lkp/linux/commit/aca13e7e71e5c2b68742270a834fd67929850ef9
> > git remote add linux-review https://github.com/intel-lab-lkp/linux
> > git fetch --no-tags linux-review Yoochan-Lee/misc-hpilo-Fix-use-after-free-in-ilo_open/20221231-135458
> > git checkout aca13e7e71e5c2b68742270a834fd67929850ef9
> > # save the config file
> > mkdir build_dir && cp config build_dir/.config
> > make W=1 O=build_dir ARCH=x86_64 olddefconfig
> > make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/misc/
> >
> > If you fix the issue, kindly add following tag where applicable
> > | Reported-by: kernel test robot <[email protected]>
> >
> > All warnings (new ones prefixed by >>):
> >
> > drivers/misc/hpilo.c: In function 'ilo_delete':
> > drivers/misc/hpilo.c:541:9: error: 'minor' undeclared (first use in this function); did you mean 'iminor'?
> > 541 | minor = MINOR(ilo_hw->cdev.dev);
> > | ^~~~~
> > | iminor
> > drivers/misc/hpilo.c:541:9: note: each undeclared identifier is reported only once for each function it appears in
> > drivers/misc/hpilo.c:542:14: error: 'i' undeclared (first use in this function)
> > 542 | for (i = minor; i < minor + max_ccb; i++)
> > | ^
> > drivers/misc/hpilo.c:547:18: error: 'pdev' undeclared (first use in this function); did you mean 'cdev'?
> > 547 | free_irq(pdev->irq, ilo_hw);
> > | ^~~~
> > | cdev
> > drivers/misc/hpilo.c:548:9: error: implicit declaration of function 'ilo_unmap_device' [-Werror=implicit-function-declaration]
> > 548 | ilo_unmap_device(pdev, ilo_hw);
> > | ^~~~~~~~~~~~~~~~
> > drivers/misc/hpilo.c: At top level:
> > drivers/misc/hpilo.c:715:13: warning: conflicting types for 'ilo_unmap_device'; have 'void(struct pci_dev *, struct ilo_hwinfo *)'
> > 715 | static void ilo_unmap_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
> > | ^~~~~~~~~~~~~~~~
> > drivers/misc/hpilo.c:715:13: error: static declaration of 'ilo_unmap_device' follows non-static declaration
> > drivers/misc/hpilo.c:548:9: note: previous implicit declaration of 'ilo_unmap_device' with type 'void(struct pci_dev *, struct ilo_hwinfo *)'
> > 548 | ilo_unmap_device(pdev, ilo_hw);
> > | ^~~~~~~~~~~~~~~~
> > drivers/misc/hpilo.c: In function 'ilo_remove':
> > drivers/misc/hpilo.c:781:19: error: 'hw' undeclared (first use in this function)
> > 781 | kref_put(&hw->refcnt, ilo_delete);
> > | ^~
> > >> drivers/misc/hpilo.c:775:16: warning: unused variable 'minor' [-Wunused-variable]
> > 775 | int i, minor;
> > | ^~~~~
> > >> drivers/misc/hpilo.c:775:13: warning: unused variable 'i' [-Wunused-variable]
> > 775 | int i, minor;
> > | ^
> > drivers/misc/hpilo.c: In function 'ilo_probe':
> > drivers/misc/hpilo.c:823:20: error: 'iol_hw' undeclared (first use in this function); did you mean 'ilo_hw'?
> > 823 | kref_init(&iol_hw->refcnt);
> > | ^~~~~~
> > | ilo_hw
> > cc1: some warnings being treated as errors
> >
> >
> > vim +/minor +775 drivers/misc/hpilo.c
> >
> > 9f7048412163d8 David Altobelli 2009-08-17 714
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 @715 static void ilo_unmap_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 716 {
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 717 pci_iounmap(pdev, hw->db_vaddr);
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 718 pci_iounmap(pdev, hw->ram_vaddr);
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 719 pci_iounmap(pdev, hw->mmio_vaddr);
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 720 }
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 721
> > 80c8ae28926652 Bill Pemberton 2012-11-19 722 static int ilo_map_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 723 {
> > c9fef1cc3dd367 Rusk, Mark 2016-09-19 724 int bar;
> > c9fef1cc3dd367 Rusk, Mark 2016-09-19 725 unsigned long off;
> > 23d51b81815127 Matt Hsiao 2021-05-31 726 u8 pci_rev_id;
> > 23d51b81815127 Matt Hsiao 2021-05-31 727 int rc;
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 728
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 729 /* map the memory mapped i/o registers */
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 730 hw->mmio_vaddr = pci_iomap(pdev, 1, 0);
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 731 if (hw->mmio_vaddr == NULL) {
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 732 dev_err(&pdev->dev, "Error mapping mmio\n");
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 733 goto out;
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 734 }
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 735
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 736 /* map the adapter shared memory region */
> > 23d51b81815127 Matt Hsiao 2021-05-31 737 rc = pci_read_config_byte(pdev, PCI_REVISION_ID, &pci_rev_id);
> > 23d51b81815127 Matt Hsiao 2021-05-31 738 if (rc != 0) {
> > 23d51b81815127 Matt Hsiao 2021-05-31 739 dev_err(&pdev->dev, "Error reading PCI rev id: %d\n", rc);
> > 23d51b81815127 Matt Hsiao 2021-05-31 740 goto out;
> > 23d51b81815127 Matt Hsiao 2021-05-31 741 }
> > 23d51b81815127 Matt Hsiao 2021-05-31 742
> > 23d51b81815127 Matt Hsiao 2021-05-31 743 if (pci_rev_id >= PCI_REV_ID_NECHES) {
> > c9fef1cc3dd367 Rusk, Mark 2016-09-19 744 bar = 5;
> > c9fef1cc3dd367 Rusk, Mark 2016-09-19 745 /* Last 8k is reserved for CCBs */
> > c9fef1cc3dd367 Rusk, Mark 2016-09-19 746 off = pci_resource_len(pdev, bar) - 0x2000;
> > c9fef1cc3dd367 Rusk, Mark 2016-09-19 747 } else {
> > c9fef1cc3dd367 Rusk, Mark 2016-09-19 748 bar = 2;
> > c9fef1cc3dd367 Rusk, Mark 2016-09-19 749 off = 0;
> > c9fef1cc3dd367 Rusk, Mark 2016-09-19 750 }
> > c9fef1cc3dd367 Rusk, Mark 2016-09-19 751 hw->ram_vaddr = pci_iomap_range(pdev, bar, off, max_ccb * ILOHW_CCB_SZ);
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 752 if (hw->ram_vaddr == NULL) {
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 753 dev_err(&pdev->dev, "Error mapping shared mem\n");
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 754 goto mmio_free;
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 755 }
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 756
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 757 /* map the doorbell aperture */
> > 98dcd59dd063dd Camuso, Tony 2012-06-10 758 hw->db_vaddr = pci_iomap(pdev, 3, max_ccb * ONE_DB_SIZE);
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 759 if (hw->db_vaddr == NULL) {
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 760 dev_err(&pdev->dev, "Error mapping doorbell\n");
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 761 goto ram_free;
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 762 }
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 763
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 764 return 0;
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 765 ram_free:
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 766 pci_iounmap(pdev, hw->ram_vaddr);
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 767 mmio_free:
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 768 pci_iounmap(pdev, hw->mmio_vaddr);
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 769 out:
> > c9fef1cc3dd367 Rusk, Mark 2016-09-19 770 return -ENOMEM;
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 771 }
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 772
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 773 static void ilo_remove(struct pci_dev *pdev)
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 774 {
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 @775 int i, minor;
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 776 struct ilo_hwinfo *ilo_hw = pci_get_drvdata(pdev);
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 777
> > ebf1b764aa5cb3 Mark Rusk 2012-11-06 778 if (!ilo_hw)
> > ebf1b764aa5cb3 Mark Rusk 2012-11-06 779 return;
> > ebf1b764aa5cb3 Mark Rusk 2012-11-06 780
> > aca13e7e71e5c2 Yoochan Lee 2022-12-31 @781 kref_put(&hw->refcnt, ilo_delete);
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 782 }
> > 89bcb05d9bbf8b David Altobelli 2008-07-02 783
> >
> > --
> > 0-DAY CI Kernel Test Service
> > https://01.org/lkp
On Sat, Dec 31, 2022 at 02:53:10PM +0900, Yoochan Lee wrote:
> A race condition may occur if the user physically removes the
> hpilo device while calling open().
How did you test any of this, and how can you remove one of these
devices like this?
thanks,
greg k-h
On Sat, Dec 31, 2022 at 02:53:10PM +0900, Yoochan Lee wrote:
> --- a/drivers/misc/hpilo.h
> +++ b/drivers/misc/hpilo.h
> @@ -62,6 +62,7 @@ struct ilo_hwinfo {
> spinlock_t fifo_lock;
>
> struct cdev cdev;
> + struct kref refcnt;
This is obviously incorrect, please never have 2 reference counts for
the same structure.
greg k-h
Hi Yoochan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus soc/for-next linus/master v6.2-rc1 next-20221226]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yoochan-Lee/misc-hpilo-Fix-use-after-free-in-ilo_open/20221231-135458
patch link: https://lore.kernel.org/r/20221231055310.2040648-1-yoochan1026%40gmail.com
patch subject: [PATCH] misc: hpilo: Fix use-after-free in ilo_open
config: x86_64-rhel-8.3-kselftests
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/aca13e7e71e5c2b68742270a834fd67929850ef9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yoochan-Lee/misc-hpilo-Fix-use-after-free-in-ilo_open/20221231-135458
git checkout aca13e7e71e5c2b68742270a834fd67929850ef9
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/misc/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All error/warnings (new ones prefixed by >>):
drivers/misc/hpilo.c: In function 'ilo_delete':
>> drivers/misc/hpilo.c:541:9: error: 'minor' undeclared (first use in this function); did you mean 'iminor'?
541 | minor = MINOR(ilo_hw->cdev.dev);
| ^~~~~
| iminor
drivers/misc/hpilo.c:541:9: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/misc/hpilo.c:542:14: error: 'i' undeclared (first use in this function)
542 | for (i = minor; i < minor + max_ccb; i++)
| ^
>> drivers/misc/hpilo.c:547:18: error: 'pdev' undeclared (first use in this function); did you mean 'cdev'?
547 | free_irq(pdev->irq, ilo_hw);
| ^~~~
| cdev
>> drivers/misc/hpilo.c:548:9: error: implicit declaration of function 'ilo_unmap_device' [-Werror=implicit-function-declaration]
548 | ilo_unmap_device(pdev, ilo_hw);
| ^~~~~~~~~~~~~~~~
drivers/misc/hpilo.c: At top level:
>> drivers/misc/hpilo.c:715:13: warning: conflicting types for 'ilo_unmap_device'; have 'void(struct pci_dev *, struct ilo_hwinfo *)'
715 | static void ilo_unmap_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
| ^~~~~~~~~~~~~~~~
>> drivers/misc/hpilo.c:715:13: error: static declaration of 'ilo_unmap_device' follows non-static declaration
drivers/misc/hpilo.c:548:9: note: previous implicit declaration of 'ilo_unmap_device' with type 'void(struct pci_dev *, struct ilo_hwinfo *)'
548 | ilo_unmap_device(pdev, ilo_hw);
| ^~~~~~~~~~~~~~~~
drivers/misc/hpilo.c: In function 'ilo_remove':
>> drivers/misc/hpilo.c:781:19: error: 'hw' undeclared (first use in this function)
781 | kref_put(&hw->refcnt, ilo_delete);
| ^~
>> drivers/misc/hpilo.c:775:16: warning: unused variable 'minor' [-Wunused-variable]
775 | int i, minor;
| ^~~~~
>> drivers/misc/hpilo.c:775:13: warning: unused variable 'i' [-Wunused-variable]
775 | int i, minor;
| ^
drivers/misc/hpilo.c: In function 'ilo_probe':
>> drivers/misc/hpilo.c:823:20: error: 'iol_hw' undeclared (first use in this function); did you mean 'ilo_hw'?
823 | kref_init(&iol_hw->refcnt);
| ^~~~~~
| ilo_hw
cc1: some warnings being treated as errors
vim +541 drivers/misc/hpilo.c
534
535 static void ilo_delete(struct kref *kref)
536 {
537 struct ilo_hwinfo *ilo_hw = container_of(kref, struct ilo_hwinfo, refcnt);
538
539 clear_device(ilo_hw);
540
> 541 minor = MINOR(ilo_hw->cdev.dev);
> 542 for (i = minor; i < minor + max_ccb; i++)
543 device_destroy(ilo_class, MKDEV(ilo_major, i));
544
545 cdev_del(&ilo_hw->cdev);
546 ilo_disable_interrupts(ilo_hw);
> 547 free_irq(pdev->irq, ilo_hw);
> 548 ilo_unmap_device(pdev, ilo_hw);
549 pci_release_regions(pdev);
550 /*
551 * pci_disable_device(pdev) used to be here. But this PCI device has
552 * two functions with interrupt lines connected to a single pin. The
553 * other one is a USB host controller. So when we disable the PIN here
554 * e.g. by rmmod hpilo, the controller stops working. It is because
555 * the interrupt link is disabled in ACPI since it is not refcounted
556 * yet. See acpi_pci_link_free_irq called from acpi_pci_irq_disable.
557 */
558 kfree(ilo_hw);
559 ilo_hwdev[(minor / max_ccb)] = 0;
560
561 }
562
563 static int ilo_close(struct inode *ip, struct file *fp)
564 {
565 int slot;
566 struct ccb_data *data;
567 struct ilo_hwinfo *hw;
568 unsigned long flags;
569
570 slot = iminor(ip) % max_ccb;
571 hw = container_of(ip->i_cdev, struct ilo_hwinfo, cdev);
572
573 spin_lock(&hw->open_lock);
574
575 if (hw->ccb_alloc[slot]->ccb_cnt == 1) {
576
577 data = fp->private_data;
578
579 spin_lock_irqsave(&hw->alloc_lock, flags);
580 hw->ccb_alloc[slot] = NULL;
581 spin_unlock_irqrestore(&hw->alloc_lock, flags);
582
583 ilo_ccb_close(hw->ilo_dev, data);
584
585 kfree(data);
586 } else
587 hw->ccb_alloc[slot]->ccb_cnt--;
588
589 kref_put(&hw->refcnt, ilo_delete);
590 spin_unlock(&hw->open_lock);
591
592 return 0;
593 }
594
595 static int ilo_open(struct inode *ip, struct file *fp)
596 {
597 int slot, error;
598 struct ccb_data *data;
599 struct ilo_hwinfo *hw;
600 unsigned long flags;
601
602 slot = iminor(ip) % max_ccb;
603 hw = container_of(ip->i_cdev, struct ilo_hwinfo, cdev);
604
605 /* new ccb allocation */
606 data = kzalloc(sizeof(*data), GFP_KERNEL);
607 if (!data)
608 return -ENOMEM;
609
610 spin_lock(&hw->open_lock);
611
612 /* each fd private_data holds sw/hw view of ccb */
613 if (hw->ccb_alloc[slot] == NULL) {
614 /* create a channel control block for this minor */
615 error = ilo_ccb_setup(hw, data, slot);
616 if (error) {
617 kfree(data);
618 goto out;
619 }
620
621 data->ccb_cnt = 1;
622 data->ccb_excl = fp->f_flags & O_EXCL;
623 data->ilo_hw = hw;
624 init_waitqueue_head(&data->ccb_waitq);
625
626 /* write the ccb to hw */
627 spin_lock_irqsave(&hw->alloc_lock, flags);
628 ilo_ccb_open(hw, data, slot);
629 hw->ccb_alloc[slot] = data;
630 spin_unlock_irqrestore(&hw->alloc_lock, flags);
631
632 /* make sure the channel is functional */
633 error = ilo_ccb_verify(hw, data);
634 if (error) {
635
636 spin_lock_irqsave(&hw->alloc_lock, flags);
637 hw->ccb_alloc[slot] = NULL;
638 spin_unlock_irqrestore(&hw->alloc_lock, flags);
639
640 ilo_ccb_close(hw->ilo_dev, data);
641
642 kfree(data);
643 goto out;
644 }
645
646 } else {
647 kfree(data);
648 if (fp->f_flags & O_EXCL || hw->ccb_alloc[slot]->ccb_excl) {
649 /*
650 * The channel exists, and either this open
651 * or a previous open of this channel wants
652 * exclusive access.
653 */
654 error = -EBUSY;
655 } else {
656 hw->ccb_alloc[slot]->ccb_cnt++;
657 error = 0;
658 }
659 }
660 out:
661 kref_get(&hw->refcnt);
662 spin_unlock(&hw->open_lock);
663
664 if (!error)
665 fp->private_data = hw->ccb_alloc[slot];
666
667 return error;
668 }
669
670 static const struct file_operations ilo_fops = {
671 .owner = THIS_MODULE,
672 .read = ilo_read,
673 .write = ilo_write,
674 .poll = ilo_poll,
675 .open = ilo_open,
676 .release = ilo_close,
677 .llseek = noop_llseek,
678 };
679
680 static irqreturn_t ilo_isr(int irq, void *data)
681 {
682 struct ilo_hwinfo *hw = data;
683 int pending, i;
684
685 spin_lock(&hw->alloc_lock);
686
687 /* check for ccbs which have data */
688 pending = get_device_outbound(hw);
689 if (!pending) {
690 spin_unlock(&hw->alloc_lock);
691 return IRQ_NONE;
692 }
693
694 if (is_db_reset(pending)) {
695 /* wake up all ccbs if the device was reset */
696 pending = -1;
697 ilo_set_reset(hw);
698 }
699
700 for (i = 0; i < max_ccb; i++) {
701 if (!hw->ccb_alloc[i])
702 continue;
703 if (pending & (1 << i))
704 wake_up_interruptible(&hw->ccb_alloc[i]->ccb_waitq);
705 }
706
707 /* clear the device of the channels that have been handled */
708 clear_pending_db(hw, pending);
709
710 spin_unlock(&hw->alloc_lock);
711
712 return IRQ_HANDLED;
713 }
714
> 715 static void ilo_unmap_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
716 {
717 pci_iounmap(pdev, hw->db_vaddr);
718 pci_iounmap(pdev, hw->ram_vaddr);
719 pci_iounmap(pdev, hw->mmio_vaddr);
720 }
721
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi,
I haven't tested with a physical device cause I don't have a real device.
I found this bug through static analysis.
This type of bug is similar to [1] and [2].
And I'm sorry that my patch is incorrect.
It's my first time patching a Linux kernel myself.
So I'm not familiar with how to patch it well (For this reason, my
patches are referred to [1] and [2]).
Then, how should I patch it?
[1] https://lore.kernel.org/lkml/20220919040457.GA302681@ubuntu/
[2] https://lore.kernel.org/lkml/20220919101825.GA313940@ubuntu/
2022년 12월 31일 (토) 오후 6:58, Greg KH <[email protected]>님이 작성:
>
> On Sat, Dec 31, 2022 at 02:53:10PM +0900, Yoochan Lee wrote:
> > --- a/drivers/misc/hpilo.h
> > +++ b/drivers/misc/hpilo.h
> > @@ -62,6 +62,7 @@ struct ilo_hwinfo {
> > spinlock_t fifo_lock;
> >
> > struct cdev cdev;
> > + struct kref refcnt;
>
> This is obviously incorrect, please never have 2 reference counts for
> the same structure.
>
> greg k-h
Hi Yoochan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus soc/for-next linus/master v6.2-rc1 next-20221226]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yoochan-Lee/misc-hpilo-Fix-use-after-free-in-ilo_open/20221231-135458
patch link: https://lore.kernel.org/r/20221231055310.2040648-1-yoochan1026%40gmail.com
patch subject: [PATCH] misc: hpilo: Fix use-after-free in ilo_open
config: i386-randconfig-a001-20221226
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/aca13e7e71e5c2b68742270a834fd67929850ef9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yoochan-Lee/misc-hpilo-Fix-use-after-free-in-ilo_open/20221231-135458
git checkout aca13e7e71e5c2b68742270a834fd67929850ef9
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/misc/hpilo.c:541:2: error: use of undeclared identifier 'minor'
minor = MINOR(ilo_hw->cdev.dev);
^
>> drivers/misc/hpilo.c:542:7: error: use of undeclared identifier 'i'
for (i = minor; i < minor + max_ccb; i++)
^
drivers/misc/hpilo.c:542:11: error: use of undeclared identifier 'minor'
for (i = minor; i < minor + max_ccb; i++)
^
drivers/misc/hpilo.c:542:18: error: use of undeclared identifier 'i'
for (i = minor; i < minor + max_ccb; i++)
^
drivers/misc/hpilo.c:542:22: error: use of undeclared identifier 'minor'
for (i = minor; i < minor + max_ccb; i++)
^
drivers/misc/hpilo.c:542:39: error: use of undeclared identifier 'i'
for (i = minor; i < minor + max_ccb; i++)
^
drivers/misc/hpilo.c:543:46: error: use of undeclared identifier 'i'
device_destroy(ilo_class, MKDEV(ilo_major, i));
^
>> drivers/misc/hpilo.c:547:11: error: use of undeclared identifier 'pdev'
free_irq(pdev->irq, ilo_hw);
^
>> drivers/misc/hpilo.c:548:2: error: implicit declaration of function 'ilo_unmap_device' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
ilo_unmap_device(pdev, ilo_hw);
^
drivers/misc/hpilo.c:548:19: error: use of undeclared identifier 'pdev'
ilo_unmap_device(pdev, ilo_hw);
^
drivers/misc/hpilo.c:549:22: error: use of undeclared identifier 'pdev'
pci_release_regions(pdev);
^
drivers/misc/hpilo.c:559:13: error: use of undeclared identifier 'minor'
ilo_hwdev[(minor / max_ccb)] = 0;
^
drivers/misc/hpilo.c:715:13: error: static declaration of 'ilo_unmap_device' follows non-static declaration
static void ilo_unmap_device(struct pci_dev *pdev, struct ilo_hwinfo *hw)
^
drivers/misc/hpilo.c:548:2: note: previous implicit declaration is here
ilo_unmap_device(pdev, ilo_hw);
^
>> drivers/misc/hpilo.c:781:12: error: use of undeclared identifier 'hw'
kref_put(&hw->refcnt, ilo_delete);
^
>> drivers/misc/hpilo.c:823:13: error: use of undeclared identifier 'iol_hw'; did you mean 'ilo_hw'?
kref_init(&iol_hw->refcnt);
^~~~~~
ilo_hw
drivers/misc/hpilo.c:788:21: note: 'ilo_hw' declared here
struct ilo_hwinfo *ilo_hw;
^
15 errors generated.
vim +/minor +541 drivers/misc/hpilo.c
534
535 static void ilo_delete(struct kref *kref)
536 {
537 struct ilo_hwinfo *ilo_hw = container_of(kref, struct ilo_hwinfo, refcnt);
538
539 clear_device(ilo_hw);
540
> 541 minor = MINOR(ilo_hw->cdev.dev);
> 542 for (i = minor; i < minor + max_ccb; i++)
543 device_destroy(ilo_class, MKDEV(ilo_major, i));
544
545 cdev_del(&ilo_hw->cdev);
546 ilo_disable_interrupts(ilo_hw);
> 547 free_irq(pdev->irq, ilo_hw);
> 548 ilo_unmap_device(pdev, ilo_hw);
549 pci_release_regions(pdev);
550 /*
551 * pci_disable_device(pdev) used to be here. But this PCI device has
552 * two functions with interrupt lines connected to a single pin. The
553 * other one is a USB host controller. So when we disable the PIN here
554 * e.g. by rmmod hpilo, the controller stops working. It is because
555 * the interrupt link is disabled in ACPI since it is not refcounted
556 * yet. See acpi_pci_link_free_irq called from acpi_pci_irq_disable.
557 */
558 kfree(ilo_hw);
559 ilo_hwdev[(minor / max_ccb)] = 0;
560
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Sat, Dec 31, 2022 at 07:05:36PM +0900, Yoochan Lee wrote:
> Hi,
>
> I haven't tested with a physical device cause I don't have a real device.
> I found this bug through static analysis.
Then please verify that it actually is a bug and that you have fixed it
properly. To send patches that are broken wastes everyone's time :(
Also, you did not properly describe how the static analysis happened or
what tools reported it as is required.
> This type of bug is similar to [1] and [2].
>
> And I'm sorry that my patch is incorrect.
> It's my first time patching a Linux kernel myself.
I suggest taking the tutorial on kernelnewbies.org and working in the
drivers/staging/* portion of the kernel first, so that you can learn how
this all works. Do not dive in and assume that fixing issues that a
random tool spits out is even correct to do.
good luck!
greg k-h
Thanks.
Since I don't have a real device, it is difficult to verify the bug dynamically.
However, this type of race condition (i.e., b/w remove device and
fops) is prevalently founded recently[1-3].
Therefore, I think this bug can be triggered if a real device exists.
The main reason for this race condition (i.e., b/w detach and fops) is
there is no proper lock mechanism.
I think the detach device function is delayed until the other
operations (e.g., fops) is finished.
To this end, I use kref to wait for the other operations.
The tool I am making is currently under development, and it can find
the race condition between detach function and fops.
[1] https://lore.kernel.org/lkml/20220919040701.GA302806@ubuntu/
[2] https://lore.kernel.org/lkml/20220919040457.GA302681@ubuntu/
[3] https://lore.kernel.org/lkml/20220919101825.GA313940@ubuntu/
2022년 12월 31일 (토) 오후 8:46, Greg KH <[email protected]>님이 작성:
>
> On Sat, Dec 31, 2022 at 07:05:36PM +0900, Yoochan Lee wrote:
> > Hi,
> >
> > I haven't tested with a physical device cause I don't have a real device.
> > I found this bug through static analysis.
>
> Then please verify that it actually is a bug and that you have fixed it
> properly. To send patches that are broken wastes everyone's time :(
>
> Also, you did not properly describe how the static analysis happened or
> what tools reported it as is required.
>
> > This type of bug is similar to [1] and [2].
> >
> > And I'm sorry that my patch is incorrect.
> > It's my first time patching a Linux kernel myself.
>
> I suggest taking the tutorial on kernelnewbies.org and working in the
> drivers/staging/* portion of the kernel first, so that you can learn how
> this all works. Do not dive in and assume that fixing issues that a
> random tool spits out is even correct to do.
>
> good luck!
>
> greg k-h
A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
A: No.
Q: Should I include quotations after my reply?
http://daringfireball.net/2007/07/on_top
On Sat, Dec 31, 2022 at 10:06:19PM +0900, Yoochan Lee wrote:
> Thanks.
>
> Since I don't have a real device, it is difficult to verify the bug dynamically.
> However, this type of race condition (i.e., b/w remove device and
> fops) is prevalently founded recently[1-3].
> Therefore, I think this bug can be triggered if a real device exists.
And how can this device actually be removed from the system? Is that
possible with this hardware?
> The main reason for this race condition (i.e., b/w detach and fops) is
> there is no proper lock mechanism.
> I think the detach device function is delayed until the other
> operations (e.g., fops) is finished.
> To this end, I use kref to wait for the other operations.
And again, this is not the correct solution as you have way too many
reference counts happening here. Please become more familiar with how
these all work before adding another one and causing more problems like
this patch did :(
> The tool I am making is currently under development, and it can find
> the race condition between detach function and fops.
Then you MUST document this as it looks like your tool needs work.
Please read Documentation/process/researcher-guidelines.rst for what you
MUST do if you use a tool to find "issues" and send out random patches.
good luck!
greg k-h
> And how can this device actually be removed from the system? Is that
> possible with this hardware?
HP ILO device is connected by using LAN cable.
Therefore, the detach function is triggered when the attacker
physically detaches the LAN cable connected to the HP ILO device.
> And again, this is not the correct solution as you have way too many
> reference counts happening here. Please become more familiar with how
> these all work before adding another one and causing more problems like
> this patch did :(
Okay, I'll find a better way to patch this bug.
Sincerely,
Yoochan
2022년 12월 31일 (토) 오후 10:48, Greg KH <[email protected]>님이 작성:
>
> A: http://en.wikipedia.org/wiki/Top_post
> Q: Were do I find info about this thing called top-posting?
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
>
> A: No.
> Q: Should I include quotations after my reply?
>
> http://daringfireball.net/2007/07/on_top
>
> On Sat, Dec 31, 2022 at 10:06:19PM +0900, Yoochan Lee wrote:
> > Thanks.
> >
> > Since I don't have a real device, it is difficult to verify the bug dynamically.
> > However, this type of race condition (i.e., b/w remove device and
> > fops) is prevalently founded recently[1-3].
> > Therefore, I think this bug can be triggered if a real device exists.
>
> And how can this device actually be removed from the system? Is that
> possible with this hardware?
>
> > The main reason for this race condition (i.e., b/w detach and fops) is
> > there is no proper lock mechanism.
> > I think the detach device function is delayed until the other
> > operations (e.g., fops) is finished.
> > To this end, I use kref to wait for the other operations.
>
> And again, this is not the correct solution as you have way too many
> reference counts happening here. Please become more familiar with how
> these all work before adding another one and causing more problems like
> this patch did :(
>
> > The tool I am making is currently under development, and it can find
> > the race condition between detach function and fops.
>
> Then you MUST document this as it looks like your tool needs work.
> Please read Documentation/process/researcher-guidelines.rst for what you
> MUST do if you use a tool to find "issues" and send out random patches.
>
> good luck!
>
> greg k-h
On Sun, Jan 01, 2023 at 10:19:14AM +0900, Yoochan Lee wrote:
> > And how can this device actually be removed from the system? Is that
> > possible with this hardware?
> HP ILO device is connected by using LAN cable.
> Therefore, the detach function is triggered when the attacker
> physically detaches the LAN cable connected to the HP ILO device.
This is incorrect.
The iLO device is an SoC attached directly on the PCB board.
It cannot be removed like a PCMCIA card that you referred so the
patch cannot apply the same to iLO. This iLO SoC does have a NIC, but
detach the LAN cable connected to it does not remove the SoC itself from
the host CPU.
>
> > And again, this is not the correct solution as you have way too many
> > reference counts happening here. Please become more familiar with how
> > these all work before adding another one and causing more problems like
> > this patch did :(
> Okay, I'll find a better way to patch this bug.
>
> Sincerely,
> Yoochan
>
> 2022년 12월 31일 (토) 오후 10:48, Greg KH <[email protected]>님이 작성:
> >
> > A: http://en.wikipedia.org/wiki/Top_post
> > Q: Were do I find info about this thing called top-posting?
> > A: Because it messes up the order in which people normally read text.
> > Q: Why is top-posting such a bad thing?
> > A: Top-posting.
> > Q: What is the most annoying thing in e-mail?
> >
> > A: No.
> > Q: Should I include quotations after my reply?
> >
> > http://daringfireball.net/2007/07/on_top
> >
> > On Sat, Dec 31, 2022 at 10:06:19PM +0900, Yoochan Lee wrote:
> > > Thanks.
> > >
> > > Since I don't have a real device, it is difficult to verify the bug dynamically.
> > > However, this type of race condition (i.e., b/w remove device and
> > > fops) is prevalently founded recently[1-3].
> > > Therefore, I think this bug can be triggered if a real device exists.
> >
> > And how can this device actually be removed from the system? Is that
> > possible with this hardware?
> >
> > > The main reason for this race condition (i.e., b/w detach and fops) is
> > > there is no proper lock mechanism.
> > > I think the detach device function is delayed until the other
> > > operations (e.g., fops) is finished.
> > > To this end, I use kref to wait for the other operations.
> >
> > And again, this is not the correct solution as you have way too many
> > reference counts happening here. Please become more familiar with how
> > these all work before adding another one and causing more problems like
> > this patch did :(
> >
> > > The tool I am making is currently under development, and it can find
> > > the race condition between detach function and fops.
> >
> > Then you MUST document this as it looks like your tool needs work.
> > Please read Documentation/process/researcher-guidelines.rst for what you
> > MUST do if you use a tool to find "issues" and send out random patches.
> >
> > good luck!
> >
> > greg k-h