[PATCH v2 1/3] platform/x86: dell-smbios-wmi: Fix chardev resource management

When unbinding the WMI driver while a userspace application has
an open file descriptor for the character device, a UAF occurs:

KASAN: slab-use-after-free in _copy_to_user from platform/x86/dell-smbios-wmi

The reason for this is that even after calling misc_deregister(),
userspace appications can still call read() and/or ioctl() on open
file descriptors associated with the already unregistered character
device. This causes a UAF by attempting to access the already freed
state container of the WMI driver.

Fix this by no longer storing the state container inside
filp->private_data. Instead retrieve the state container using
get_first_smbios_priv() and return -ENODEV if the state container
does not exist anymore.

Reported-by: Shuangpeng Bai <[email protected]>
Closes: https://lore.kernel.org/platform-driver-x86/[email protected]/
Signed-off-by: Armin Wolf <[email protected]>
---
 drivers/platform/x86/dell/dell-smbios-wmi.c | 67 +++++++++++----------
 1 file changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-smbios-wmi.c b/drivers/platform/x86/dell/dell-smbios-wmi.c
index 64d0871b706e..231fc90878bc 100644
--- a/drivers/platform/x86/dell/dell-smbios-wmi.c
+++ b/drivers/platform/x86/dell/dell-smbios-wmi.c
@@ -6,6 +6,7 @@
  */
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
 #include <linux/fs.h>
@@ -13,14 +14,13 @@
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/rwsem.h>
 #include <linux/uaccess.h>
 #include <linux/wmi.h>
 #include <uapi/linux/wmi.h>
 #include "dell-smbios.h"
 #include "dell-wmi-descriptor.h"
 
-static DEFINE_MUTEX(call_mutex);
-static DEFINE_MUTEX(list_mutex);
 static int wmi_supported;
 
 struct misc_bios_flags_structure {
@@ -32,6 +32,7 @@ struct misc_bios_flags_structure {
 #define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
 
 struct wmi_smbios_priv {
+	struct mutex call_lock; /* Protects the content of the SMBIOS buffer */
 	struct dell_wmi_smbios_buffer *buf;
 	struct list_head list;
 	struct wmi_device *wdev;
@@ -39,6 +40,8 @@ struct wmi_smbios_priv {
 	u64 req_buf_size;
 	struct miscdevice char_dev;
 };
+
+static DECLARE_RWSEM(list_lock);	/* Protects access to wmi_list */
 static LIST_HEAD(wmi_list);
 
 static inline struct wmi_smbios_priv *get_first_smbios_priv(void)
@@ -87,40 +90,35 @@ static int dell_smbios_wmi_call(struct calling_interface_buffer *buffer)
 	size_t size;
 	int ret;
 
-	mutex_lock(&call_mutex);
+	guard(rwsem_read)(&list_lock);
+
 	priv = get_first_smbios_priv();
-	if (!priv) {
-		ret = -ENODEV;
-		goto out_wmi_call;
-	}
+	if (!priv)
+		return -ENODEV;
 
 	size = sizeof(struct calling_interface_buffer);
 	difference = priv->req_buf_size - sizeof(u64) - size;
 
+	guard(mutex)(&priv->call_lock);
+
 	memset(&priv->buf->ext, 0, difference);
 	memcpy(&priv->buf->std, buffer, size);
 	ret = run_smbios_call(priv->wdev);
 	memcpy(buffer, &priv->buf->std, size);
-out_wmi_call:
-	mutex_unlock(&call_mutex);
 
 	return ret;
 }
 
-static int dell_smbios_wmi_open(struct inode *inode, struct file *filp)
+static ssize_t dell_smbios_wmi_read(struct file *filp, char __user *buffer, size_t length,
+				    loff_t *offset)
 {
 	struct wmi_smbios_priv *priv;
 
-	priv = container_of(filp->private_data, struct wmi_smbios_priv, char_dev);
-	filp->private_data = priv;
+	guard(rwsem_read)(&list_lock);
 
-	return nonseekable_open(inode, filp);
-}
-
-static ssize_t dell_smbios_wmi_read(struct file *filp, char __user *buffer, size_t length,
-				    loff_t *offset)
-{
-	struct wmi_smbios_priv *priv = filp->private_data;
+	priv = get_first_smbios_priv();
+	if (!priv)
+		return -ENODEV;
 
 	return simple_read_from_buffer(buffer, length, offset, &priv->req_buf_size,
 				       sizeof(priv->req_buf_size));
@@ -167,22 +165,24 @@ static long dell_smbios_wmi_do_ioctl(struct wmi_smbios_priv *priv,
 static long dell_smbios_wmi_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct dell_wmi_smbios_buffer __user *input = (struct dell_wmi_smbios_buffer __user *)arg;
-	struct wmi_smbios_priv *priv = filp->private_data;
-	long ret;
+	struct wmi_smbios_priv *priv;
 
 	if (cmd != DELL_WMI_SMBIOS_CMD)
 		return -ENOIOCTLCMD;
 
-	mutex_lock(&call_mutex);
-	ret = dell_smbios_wmi_do_ioctl(priv, input);
-	mutex_unlock(&call_mutex);
+	guard(rwsem_read)(&list_lock);
 
-	return ret;
+	priv = get_first_smbios_priv();
+	if (!priv)
+		return -ENODEV;
+
+	guard(mutex)(&priv->call_lock);
+
+	return dell_smbios_wmi_do_ioctl(priv, input);
 }
 
 static const struct file_operations dell_smbios_wmi_fops = {
 	.owner		= THIS_MODULE,
-	.open		= dell_smbios_wmi_open,
 	.read		= dell_smbios_wmi_read,
 	.unlocked_ioctl	= dell_smbios_wmi_ioctl,
 	.compat_ioctl	= compat_ptr_ioctl,
@@ -254,6 +254,10 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
 	if (!priv->buf)
 		return -ENOMEM;
 
+	ret = devm_mutex_init(&wdev->dev, &priv->call_lock);
+	if (ret)
+		return ret;
+
 	ret = dell_smbios_wmi_register_chardev(priv);
 	if (ret)
 		return ret;
@@ -262,9 +266,8 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
 	if (ret)
 		return ret;
 
-	mutex_lock(&list_mutex);
+	guard(rwsem_write)(&list_lock);
 	list_add_tail(&priv->list, &wmi_list);
-	mutex_unlock(&list_mutex);
 
 	return 0;
 }
@@ -273,12 +276,10 @@ static void dell_smbios_wmi_remove(struct wmi_device *wdev)
 {
 	struct wmi_smbios_priv *priv = dev_get_drvdata(&wdev->dev);
 
-	mutex_lock(&call_mutex);
-	mutex_lock(&list_mutex);
-	list_del(&priv->list);
-	mutex_unlock(&list_mutex);
 	dell_smbios_unregister_device(&wdev->dev);
-	mutex_unlock(&call_mutex);
+
+	guard(rwsem_write)(&list_lock);
+	list_del(&priv->list);
 }
 
 static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
-- 
2.39.5