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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3257 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]