Messages in this thread | | | Date | Mon, 4 Jul 2022 16:28:12 +0200 | Subject | Re: [PATCH 2/2] drm: Prevent drm_copy_field() to attempt copying a NULL pointer | From | Thomas Zimmermann <> |
| |
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@redhat.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@redhat.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);
-- 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 [unhandled content-type:application/pgp-signature] | |