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 <[email protected]> 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 <[email protected]> --- 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
