Message ID | 20220701120755.2135100-3-javierm@redhat.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | drm: A couple of fixes for drm_copy_field() helper function | expand |
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)
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.
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);
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);
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)
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(+)