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@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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
