[PATCH 0/2] drm: A couple of fixes for drm_copy_field() helper function

[PATCH 0/2] drm: A couple of fixes for drm_copy_field() helper function

[Javier Martinez Canillas]

Hello,

Peter Robinson reported me a kernel bug in one of his aarch64 test boards
and even though I was not able to reproduce it, I think that figured out
what the problem was. It seems the cause is that a DRM driver doesn't set
some of the struct drm fields copied to userspace via DRM_IOCTL_VERSION.

Even though this is a driver bug, we can make drm_copy_field() more robust
and warn about it instead of leading to an attempt to copy a NULL pointer.

While looking at this, I also found that a variable in drm_copy_field() is
not using the correct type. So I included that change in the patch-set too.

Best regards,
Javier


Javier Martinez Canillas (2):
  drm: Use size_t type for len variable in drm_copy_field()
  drm: Prevent drm_copy_field() to attempt copying a NULL pointer

 drivers/gpu/drm/drm_ioctl.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.36.1

[PATCH 1/2] drm: Use size_t type for len variable in drm_copy_field()

[Javier Martinez Canillas]

The strlen() function returns a size_t which is an unsigned int on 32-bit
arches and an unsigned long on 64-bit arches. But in the drm_copy_field()
function, the strlen() return value is assigned to an 'int len' variable.

Later, the len variable is passed as copy_from_user() third argument that
is an unsigned long parameter as well.

In theory, this can lead to an integer overflow via type conversion. Since
the assignment happens to a signed int lvalue instead of a size_t lvalue.

In practice though, that's unlikely since the values copied are set by DRM
drivers and not controlled by userspace. But using a size_t for len is the
correct thing to do anyways.

Reported-by: Peter Robinson <pbrobinson@gmail.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/drm_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 8faad23dc1d8..e1b9a03e619c 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -472,7 +472,7 @@ EXPORT_SYMBOL(drm_invalid_op);
  */
 static int drm_copy_field(char __user *buf, size_t *buf_len, const char *value)
 {
-	int len;
+	size_t len;
 
 	/* don't overflow userbuf */
 	len = strlen(value);
-- 
2.36.1

Re: [PATCH 1/2] drm: Use size_t type for len variable in drm_copy_field()

[Thomas Zimmermann]

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



Am 01.07.22 um 14:07 schrieb Javier Martinez Canillas:
> The strlen() function returns a size_t which is an unsigned int on 32-bit
> arches and an unsigned long on 64-bit arches. But in the drm_copy_field()
> function, the strlen() return value is assigned to an 'int len' variable.
> 
> Later, the len variable is passed as copy_from_user() third argument that
> is an unsigned long parameter as well.
> 
> In theory, this can lead to an integer overflow via type conversion. Since
> the assignment happens to a signed int lvalue instead of a size_t lvalue.
> 
> In practice though, that's unlikely since the values copied are set by DRM
> drivers and not controlled by userspace. But using a size_t for len is the
> correct thing to do anyways.
> 
> Reported-by: Peter Robinson <pbrobinson@gmail.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
> 
>   drivers/gpu/drm/drm_ioctl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 8faad23dc1d8..e1b9a03e619c 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -472,7 +472,7 @@ EXPORT_SYMBOL(drm_invalid_op);
>    */
>   static int drm_copy_field(char __user *buf, size_t *buf_len, const char *value)
>   {
> -	int len;
> +	size_t len;
>   
>   	/* don't overflow userbuf */
>   	len = strlen(value);

-- 
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 --]

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

[Javier Martinez Canillas]

The drm_copy_field() helper is used to copy some struct drm_driver fields
to userspace through the DRM_IOCTL_VERSION ioctl. But it could be possible
that the driver didn't set some of the fields and are instead set to NULL.

If that is the case, the drm_copy_field() attempts to copy a NULL pointer:

[ +10.395966] Unable to handle kernel access to user memory outside uaccess routines at virtual address 0000000000000000
[  +0.010955] Mem abort info:
[  +0.002835]   ESR = 0x0000000096000004
[  +0.003872]   EC = 0x25: DABT (current EL), IL = 32 bits
[  +0.005395]   SET = 0, FnV = 0
[  +0.003113]   EA = 0, S1PTW = 0
[  +0.003182]   FSC = 0x04: level 0 translation fault
[  +0.004964] Data abort info:
[  +0.002919]   ISV = 0, ISS = 0x00000004
[  +0.003886]   CM = 0, WnR = 0
[  +0.003040] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000115dad000
[  +0.006536] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
[  +0.006925] Internal error: Oops: 96000004 [#1] SMP
...
[  +0.011113] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  +0.007061] pc : __pi_strlen+0x14/0x150
[  +0.003895] lr : drm_copy_field+0x30/0x1a4
[  +0.004156] sp : ffff8000094b3a50
[  +0.003355] x29: ffff8000094b3a50 x28: ffff8000094b3b70 x27: 0000000000000040
[  +0.007242] x26: ffff443743c2ba00 x25: 0000000000000000 x24: 0000000000000040
[  +0.007243] x23: ffff443743c2ba00 x22: ffff8000094b3b70 x21: 0000000000000000
[  +0.007241] x20: 0000000000000000 x19: ffff8000094b3b90 x18: 0000000000000000
[  +0.007241] x17: 0000000000000000 x16: 0000000000000000 x15: 0000aaab14b9af40
[  +0.007241] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
[  +0.007239] x11: 0000000000000000 x10: 0000000000000000 x9 : ffffa524ad67d4d8
[  +0.007242] x8 : 0101010101010101 x7 : 7f7f7f7f7f7f7f7f x6 : 6c6e6263606e7141
[  +0.007239] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
[  +0.007241] x2 : 0000000000000000 x1 : ffff8000094b3b90 x0 : 0000000000000000
[  +0.007240] Call trace:
[  +0.002475]  __pi_strlen+0x14/0x150
[  +0.003537]  drm_version+0x84/0xac
[  +0.003448]  drm_ioctl_kernel+0xa8/0x16c
[  +0.003975]  drm_ioctl+0x270/0x580
[  +0.003448]  __arm64_sys_ioctl+0xb8/0xfc
[  +0.003978]  invoke_syscall+0x78/0x100
[  +0.003799]  el0_svc_common.constprop.0+0x4c/0xf4
[  +0.004767]  do_el0_svc+0x38/0x4c
[  +0.003357]  el0_svc+0x34/0x100
[  +0.003185]  el0t_64_sync_handler+0x11c/0x150
[  +0.004418]  el0t_64_sync+0x190/0x194
[  +0.003716] Code: 92402c04 b200c3e8 f13fc09f 5400088c (a9400c02)
[  +0.006180] ---[ end trace 0000000000000000 ]---

Reported-by: Peter Robinson <pbrobinson@gmail.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/drm_ioctl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index e1b9a03e619c..dacaddc59c82 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -474,6 +474,10 @@ static int drm_copy_field(char __user *buf, size_t *buf_len, const char *value)
 {
 	size_t len;
 
+	/* don't attempt to copy a NULL pointer */
+	if (WARN_ONCE(!value, "BUG: the value to copy was not set!"))
+		return -EINVAL;
+
 	/* don't overflow userbuf */
 	len = strlen(value);
 	if (len > *buf_len)
-- 
2.36.1

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

[Thomas Zimmermann]

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

Hi

Am 01.07.22 um 14:07 schrieb Javier Martinez Canillas:
> The drm_copy_field() helper is used to copy some struct drm_driver fields
> to userspace through the DRM_IOCTL_VERSION ioctl. But it could be possible
> that the driver didn't set some of the fields and are instead set to NULL.
> 
> If that is the case, the drm_copy_field() attempts to copy a NULL pointer:
> 
> [ +10.395966] Unable to handle kernel access to user memory outside uaccess routines at virtual address 0000000000000000
> [  +0.010955] Mem abort info:
> [  +0.002835]   ESR = 0x0000000096000004
> [  +0.003872]   EC = 0x25: DABT (current EL), IL = 32 bits
> [  +0.005395]   SET = 0, FnV = 0
> [  +0.003113]   EA = 0, S1PTW = 0
> [  +0.003182]   FSC = 0x04: level 0 translation fault
> [  +0.004964] Data abort info:
> [  +0.002919]   ISV = 0, ISS = 0x00000004
> [  +0.003886]   CM = 0, WnR = 0
> [  +0.003040] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000115dad000
> [  +0.006536] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
> [  +0.006925] Internal error: Oops: 96000004 [#1] SMP
> ...
> [  +0.011113] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  +0.007061] pc : __pi_strlen+0x14/0x150
> [  +0.003895] lr : drm_copy_field+0x30/0x1a4
> [  +0.004156] sp : ffff8000094b3a50
> [  +0.003355] x29: ffff8000094b3a50 x28: ffff8000094b3b70 x27: 0000000000000040
> [  +0.007242] x26: ffff443743c2ba00 x25: 0000000000000000 x24: 0000000000000040
> [  +0.007243] x23: ffff443743c2ba00 x22: ffff8000094b3b70 x21: 0000000000000000
> [  +0.007241] x20: 0000000000000000 x19: ffff8000094b3b90 x18: 0000000000000000
> [  +0.007241] x17: 0000000000000000 x16: 0000000000000000 x15: 0000aaab14b9af40
> [  +0.007241] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
> [  +0.007239] x11: 0000000000000000 x10: 0000000000000000 x9 : ffffa524ad67d4d8
> [  +0.007242] x8 : 0101010101010101 x7 : 7f7f7f7f7f7f7f7f x6 : 6c6e6263606e7141
> [  +0.007239] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
> [  +0.007241] x2 : 0000000000000000 x1 : ffff8000094b3b90 x0 : 0000000000000000
> [  +0.007240] Call trace:
> [  +0.002475]  __pi_strlen+0x14/0x150
> [  +0.003537]  drm_version+0x84/0xac
> [  +0.003448]  drm_ioctl_kernel+0xa8/0x16c
> [  +0.003975]  drm_ioctl+0x270/0x580
> [  +0.003448]  __arm64_sys_ioctl+0xb8/0xfc
> [  +0.003978]  invoke_syscall+0x78/0x100
> [  +0.003799]  el0_svc_common.constprop.0+0x4c/0xf4
> [  +0.004767]  do_el0_svc+0x38/0x4c
> [  +0.003357]  el0_svc+0x34/0x100
> [  +0.003185]  el0t_64_sync_handler+0x11c/0x150
> [  +0.004418]  el0t_64_sync+0x190/0x194
> [  +0.003716] Code: 92402c04 b200c3e8 f13fc09f 5400088c (a9400c02)
> [  +0.006180] ---[ end trace 0000000000000000 ]---
> 
> Reported-by: Peter Robinson <pbrobinson@gmail.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>   drivers/gpu/drm/drm_ioctl.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index e1b9a03e619c..dacaddc59c82 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -474,6 +474,10 @@ static int drm_copy_field(char __user *buf, size_t *buf_len, const char *value)
>   {
>   	size_t len;
>   
> +	/* 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).

Best regards
Thomas


>   	/* don't overflow userbuf */
>   	len = strlen(value);
>   	if (len > *buf_len)

-- 
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 --]

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

[Javier Martinez Canillas]

Hello Thomas,

Thanks for your feedback.

On 7/4/22 14:30, Thomas Zimmermann wrote:
> Hi
> 
> Am 01.07.22 um 14:07 schrieb Javier Martinez Canillas:
>> The drm_copy_field() helper is used to copy some struct drm_driver fields
>> to userspace through the DRM_IOCTL_VERSION ioctl. But it could be possible
>> that the driver didn't set some of the fields and are instead set to NULL.
>>
>> If that is the case, the drm_copy_field() attempts to copy a NULL pointer:
>>
>> [ +10.395966] Unable to handle kernel access to user memory outside uaccess routines at virtual address 0000000000000000
>> [  +0.010955] Mem abort info:
>> [  +0.002835]   ESR = 0x0000000096000004
>> [  +0.003872]   EC = 0x25: DABT (current EL), IL = 32 bits
>> [  +0.005395]   SET = 0, FnV = 0
>> [  +0.003113]   EA = 0, S1PTW = 0
>> [  +0.003182]   FSC = 0x04: level 0 translation fault
>> [  +0.004964] Data abort info:
>> [  +0.002919]   ISV = 0, ISS = 0x00000004
>> [  +0.003886]   CM = 0, WnR = 0
>> [  +0.003040] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000115dad000
>> [  +0.006536] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
>> [  +0.006925] Internal error: Oops: 96000004 [#1] SMP
>> ...
>> [  +0.011113] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [  +0.007061] pc : __pi_strlen+0x14/0x150
>> [  +0.003895] lr : drm_copy_field+0x30/0x1a4
>> [  +0.004156] sp : ffff8000094b3a50
>> [  +0.003355] x29: ffff8000094b3a50 x28: ffff8000094b3b70 x27: 0000000000000040
>> [  +0.007242] x26: ffff443743c2ba00 x25: 0000000000000000 x24: 0000000000000040
>> [  +0.007243] x23: ffff443743c2ba00 x22: ffff8000094b3b70 x21: 0000000000000000
>> [  +0.007241] x20: 0000000000000000 x19: ffff8000094b3b90 x18: 0000000000000000
>> [  +0.007241] x17: 0000000000000000 x16: 0000000000000000 x15: 0000aaab14b9af40
>> [  +0.007241] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
>> [  +0.007239] x11: 0000000000000000 x10: 0000000000000000 x9 : ffffa524ad67d4d8
>> [  +0.007242] x8 : 0101010101010101 x7 : 7f7f7f7f7f7f7f7f x6 : 6c6e6263606e7141
>> [  +0.007239] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
>> [  +0.007241] x2 : 0000000000000000 x1 : ffff8000094b3b90 x0 : 0000000000000000
>> [  +0.007240] Call trace:
>> [  +0.002475]  __pi_strlen+0x14/0x150
>> [  +0.003537]  drm_version+0x84/0xac
>> [  +0.003448]  drm_ioctl_kernel+0xa8/0x16c
>> [  +0.003975]  drm_ioctl+0x270/0x580
>> [  +0.003448]  __arm64_sys_ioctl+0xb8/0xfc
>> [  +0.003978]  invoke_syscall+0x78/0x100
>> [  +0.003799]  el0_svc_common.constprop.0+0x4c/0xf4
>> [  +0.004767]  do_el0_svc+0x38/0x4c
>> [  +0.003357]  el0_svc+0x34/0x100
>> [  +0.003185]  el0t_64_sync_handler+0x11c/0x150
>> [  +0.004418]  el0t_64_sync+0x190/0x194
>> [  +0.003716] Code: 92402c04 b200c3e8 f13fc09f 5400088c (a9400c02)
>> [  +0.006180] ---[ end trace 0000000000000000 ]---
>>
>> Reported-by: Peter Robinson <pbrobinson@gmail.com>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>
>>   drivers/gpu/drm/drm_ioctl.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index e1b9a03e619c..dacaddc59c82 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -474,6 +474,10 @@ static int drm_copy_field(char __user *buf, size_t *buf_len, const char *value)
>>   {
>>   	size_t len;
>>   
>> +	/* 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.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

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

[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);
-- 
2.36.1

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

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

[Thomas Zimmermann]

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

Re: [PATCH 0/2] drm: A couple of fixes for drm_copy_field() helper function

[Peter Robinson]

On Fri, Jul 1, 2022 at 1:08 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Hello,
>
> Peter Robinson reported me a kernel bug in one of his aarch64 test boards
> and even though I was not able to reproduce it, I think that figured out
> what the problem was. It seems the cause is that a DRM driver doesn't set
> some of the struct drm fields copied to userspace via DRM_IOCTL_VERSION.
>
> Even though this is a driver bug, we can make drm_copy_field() more robust
> and warn about it instead of leading to an attempt to copy a NULL pointer.
>
> While looking at this, I also found that a variable in drm_copy_field() is
> not using the correct type. So I included that change in the patch-set too.
>
> Best regards,
> Javier

For the series
Tested-by: Peter Robinson <pbrobinson@gmail.com>

>
> Javier Martinez Canillas (2):
>   drm: Use size_t type for len variable in drm_copy_field()
>   drm: Prevent drm_copy_field() to attempt copying a NULL pointer
>
>  drivers/gpu/drm/drm_ioctl.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> --
> 2.36.1
>