Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Yoochan Lee <yoochan1026@gmail.com>
To:     matt.hsiao@hpe.com
Cc:     arnd@arndb.de, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org, Yoochan Lee <yoochan1026@gmail.com>
Subject: [PATCH] misc: hpilo: Fix use-after-free in ilo_open
Date:   Sat, 31 Dec 2022 14:53:10 +0900
Message-Id: <20221231055310.2040648-1-yoochan1026@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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 <yoochan1026@gmail.com>
---
 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