linux-kernel - Re: [PATCH 2/2] drm: Prevent drm_copy_field() to attempt copying a NULL pointer

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]

Date:   Mon, 4 Jul 2022 14:55:53 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Thomas Zimmermann <tzimmermann@...e.de>,
        linux-kernel@...r.kernel.org
Cc:     David Airlie <airlied@...ux.ie>, dri-devel@...ts.freedesktop.org,
        Peter Robinson <pbrobinson@...il.com>
Subject: Re: [PATCH 2/2] drm: Prevent drm_copy_field() to attempt copying a
 NULL pointer

On 7/4/22 14:36, Javier Martinez Canillas wrote:
> Hello Thomas,
> 
> Thanks for your feedback.
> 

[snip]

>>> +	/* don't attempt to copy a NULL pointer */
>>> +	if (WARN_ONCE(!value, "BUG: the value to copy was not set!"))
>>> +		return -EINVAL;
>>> +
>>
>> We usually assume that the caller passes the correct arguments. This is 
>> different for no reasons. I'd rather not take this patch unless there's 
>> a security implication to the ioctl interface (e.g., leaking information 
>> because of this NULL ptr).
>>
> 
> This can lead from an oops (soft panic) to a kernel crash for a buggy driver.
> 
> I see from where you are coming from but then I think we should sanitize the
> filled struct drm_driver fields in drm_dev_register() and make it fail early.
> 
> Would you agree with such a patch? But what I think that we shouldn't allow
> is to attempt copying a NULL pointer, if we can easily prevent it.
>

I mean something like the following patch (didn't add a commit message
for brevity):

>From 4c13400c54e0e29918a8eb248013f54cd2660f4f Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@...hat.com>
Date: Mon, 4 Jul 2022 14:53:48 +0200
Subject: [PATCH] drm: Check in drm_dev_register() that required DRM driver
 fields were set

Signed-off-by: Javier Martinez Canillas <javierm@...hat.com>
---
 drivers/gpu/drm/drm_drv.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..d4eebaf37e23 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -842,6 +842,12 @@ static void remove_compat_control_link(struct drm_device *dev)
 	kfree(name);
 }
 
+static inline bool check_drm_driver_fields(const struct drm_driver *driver)
+{
+	/* required since are copied to user-space by DRM_IOCTL_VERSION */
+	return driver->name && driver->date && driver->desc;
+}
+
 /**
  * drm_dev_register - Register DRM device
  * @dev: Device to register
@@ -865,7 +871,11 @@ static void remove_compat_control_link(struct drm_device *dev)
 int drm_dev_register(struct drm_device *dev, unsigned long flags)
 {
 	const struct drm_driver *driver = dev->driver;
-	int ret;
+	int ret = -EINVAL;
+
+	if (drm_WARN(dev, !check_drm_driver_fields(driver),
+		     "Required DRM drivers fields not set.\n"))
+			goto out_err;
 
 	if (!driver->load)
 		drm_mode_config_validate(dev);
@@ -913,6 +923,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 out_unlock:
 	if (drm_dev_needs_global_mutex(dev))
 		mutex_unlock(&drm_global_mutex);
+out_err:
 	return ret;
 }
 EXPORT_SYMBOL(drm_dev_register);
-- 
2.36.1

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat