Hi,to summarize our discussion on IRC, I'd return an empty string and do a drm_warn_once() if there's a NULL pointer.
Best regards Thomas Am 04.07.22 um 14:55 schrieb Javier Martinez Canillas:
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);
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
