* [PATCH 0/2] drm: A couple of fixes for drm_copy_field() helper function
@ 2022-07-01 12:07 Javier Martinez Canillas
2022-07-01 12:07 ` [PATCH 1/2] drm: Use size_t type for len variable in drm_copy_field() Javier Martinez Canillas
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Javier Martinez Canillas @ 2022-07-01 12:07 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Robinson, Javier Martinez Canillas, Daniel Vetter,
David Airlie, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, dri-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] drm: Use size_t type for len variable in drm_copy_field()
2022-07-01 12:07 [PATCH 0/2] drm: A couple of fixes for drm_copy_field() helper function Javier Martinez Canillas
@ 2022-07-01 12:07 ` Javier Martinez Canillas
2022-07-04 12:27 ` Thomas Zimmermann
2022-07-01 12:07 ` [PATCH 2/2] drm: Prevent drm_copy_field() to attempt copying a NULL pointer Javier Martinez Canillas
2022-07-01 17:47 ` [PATCH 0/2] drm: A couple of fixes for drm_copy_field() helper function Peter Robinson
2 siblings, 1 reply; 9+ messages in thread
From: Javier Martinez Canillas @ 2022-07-01 12:07 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Robinson, Javier Martinez Canillas, Daniel Vetter,
David Airlie, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] drm: Prevent drm_copy_field() to attempt copying a NULL pointer
2022-07-01 12:07 [PATCH 0/2] drm: A couple of fixes for drm_copy_field() helper function Javier Martinez Canillas
2022-07-01 12:07 ` [PATCH 1/2] drm: Use size_t type for len variable in drm_copy_field() Javier Martinez Canillas
@ 2022-07-01 12:07 ` Javier Martinez Canillas
2022-07-04 12:30 ` Thomas Zimmermann
2022-07-01 17:47 ` [PATCH 0/2] drm: A couple of fixes for drm_copy_field() helper function Peter Robinson
2 siblings, 1 reply; 9+ messages in thread
From: Javier Martinez Canillas @ 2022-07-01 12:07 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Robinson, Javier Martinez Canillas, Daniel Vetter,
David Airlie, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] drm: A couple of fixes for drm_copy_field() helper function
2022-07-01 12:07 [PATCH 0/2] drm: A couple of fixes for drm_copy_field() helper function Javier Martinez Canillas
2022-07-01 12:07 ` [PATCH 1/2] drm: Use size_t type for len variable in drm_copy_field() Javier Martinez Canillas
2022-07-01 12:07 ` [PATCH 2/2] drm: Prevent drm_copy_field() to attempt copying a NULL pointer Javier Martinez Canillas
@ 2022-07-01 17:47 ` Peter Robinson
2 siblings, 0 replies; 9+ messages in thread
From: Peter Robinson @ 2022-07-01 17:47 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-kernel, Daniel Vetter, David Airlie, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, dri-devel
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm: Use size_t type for len variable in drm_copy_field()
2022-07-01 12:07 ` [PATCH 1/2] drm: Use size_t type for len variable in drm_copy_field() Javier Martinez Canillas
@ 2022-07-04 12:27 ` Thomas Zimmermann
0 siblings, 0 replies; 9+ messages in thread
From: Thomas Zimmermann @ 2022-07-04 12:27 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel
Cc: Peter Robinson, Daniel Vetter, David Airlie, Maarten Lankhorst,
Maxime Ripard, dri-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm: Prevent drm_copy_field() to attempt copying a NULL pointer
2022-07-01 12:07 ` [PATCH 2/2] drm: Prevent drm_copy_field() to attempt copying a NULL pointer Javier Martinez Canillas
@ 2022-07-04 12:30 ` Thomas Zimmermann
2022-07-04 12:36 ` Javier Martinez Canillas
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Zimmermann @ 2022-07-04 12:30 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel
Cc: David Airlie, dri-devel, Peter Robinson
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm: Prevent drm_copy_field() to attempt copying a NULL pointer
2022-07-04 12:30 ` Thomas Zimmermann
@ 2022-07-04 12:36 ` Javier Martinez Canillas
2022-07-04 12:55 ` Javier Martinez Canillas
0 siblings, 1 reply; 9+ messages in thread
From: Javier Martinez Canillas @ 2022-07-04 12:36 UTC (permalink / raw)
To: Thomas Zimmermann, linux-kernel; +Cc: David Airlie, dri-devel, Peter Robinson
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm: Prevent drm_copy_field() to attempt copying a NULL pointer
2022-07-04 12:36 ` Javier Martinez Canillas
@ 2022-07-04 12:55 ` Javier Martinez Canillas
2022-07-04 14:28 ` Thomas Zimmermann
0 siblings, 1 reply; 9+ messages in thread
From: Javier Martinez Canillas @ 2022-07-04 12:55 UTC (permalink / raw)
To: Thomas Zimmermann, linux-kernel; +Cc: David Airlie, dri-devel, Peter Robinson
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm: Prevent drm_copy_field() to attempt copying a NULL pointer
2022-07-04 12:55 ` Javier Martinez Canillas
@ 2022-07-04 14:28 ` Thomas Zimmermann
0 siblings, 0 replies; 9+ messages in thread
From: Thomas Zimmermann @ 2022-07-04 14:28 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel
Cc: David Airlie, dri-devel, Peter Robinson
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-07-04 14:28 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-01 12:07 [PATCH 0/2] drm: A couple of fixes for drm_copy_field() helper function Javier Martinez Canillas
2022-07-01 12:07 ` [PATCH 1/2] drm: Use size_t type for len variable in drm_copy_field() Javier Martinez Canillas
2022-07-04 12:27 ` Thomas Zimmermann
2022-07-01 12:07 ` [PATCH 2/2] drm: Prevent drm_copy_field() to attempt copying a NULL pointer Javier Martinez Canillas
2022-07-04 12:30 ` Thomas Zimmermann
2022-07-04 12:36 ` Javier Martinez Canillas
2022-07-04 12:55 ` Javier Martinez Canillas
2022-07-04 14:28 ` Thomas Zimmermann
2022-07-01 17:47 ` [PATCH 0/2] drm: A couple of fixes for drm_copy_field() helper function Peter Robinson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).