* [PATCH] drm: never write to the userspace more data than the caller wants
@ 2013-10-17 0:12 Pavel Roskin
2013-10-17 12:26 ` Chris Wilson
2013-10-22 9:38 ` [PATCH] drm: Restrict ioctl size to kernel struct size Chris Wilson
0 siblings, 2 replies; 6+ messages in thread
From: Pavel Roskin @ 2013-10-17 0:12 UTC (permalink / raw)
To: Dave Airlie, linux-kernel, dri-devel, Chris Wilson
The amount of data wanted by the userspace caller is encoded in the
ioctl number. Generic drm ioctls were ignoring it.
As a result, Intel Xorg driver didn't work for i386 userspace on x86_64
kernel on some systems. sizeof(struct drm_mode_get_connector) is 76
bytes on i686 and 80 bytes on x86_64 due to the tail alignment (the data
positions match). The userspace was using the 4 bytes after the
structure to hold the result of the ioctl. Since drm_ioctl() was
copying 80 bytes instead of 76, it was clobbering that data.
A workaround has been committed to xf86-video-intel.
Signed-off-by: Pavel Roskin <proski@gnu.org>
Cc: stable@vger.kernel.org
---
drivers/gpu/drm/drm_drv.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index e572dd2..8a1c721 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -403,8 +403,11 @@ long drm_ioctl(struct file *filp,
}
else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
ioctl = &drm_ioctls[nr];
+ usize = _IOC_SIZE(cmd);
cmd = ioctl->cmd;
- usize = asize = _IOC_SIZE(cmd);
+ asize = _IOC_SIZE(cmd);
+ if (unlikely(usize > asize))
+ usize = asize;
} else
goto err_i1;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: never write to the userspace more data than the caller wants
2013-10-17 0:12 [PATCH] drm: never write to the userspace more data than the caller wants Pavel Roskin
@ 2013-10-17 12:26 ` Chris Wilson
2013-10-17 14:58 ` Pavel Roskin
2013-10-22 9:38 ` [PATCH] drm: Restrict ioctl size to kernel struct size Chris Wilson
1 sibling, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2013-10-17 12:26 UTC (permalink / raw)
To: Pavel Roskin; +Cc: Dave Airlie, linux-kernel, dri-devel
On Wed, Oct 16, 2013 at 08:12:35PM -0400, Pavel Roskin wrote:
> The amount of data wanted by the userspace caller is encoded in the
> ioctl number. Generic drm ioctls were ignoring it.
>
> As a result, Intel Xorg driver didn't work for i386 userspace on x86_64
> kernel on some systems. sizeof(struct drm_mode_get_connector) is 76
> bytes on i686 and 80 bytes on x86_64 due to the tail alignment (the data
> positions match). The userspace was using the 4 bytes after the
> structure to hold the result of the ioctl. Since drm_ioctl() was
> copying 80 bytes instead of 76, it was clobbering that data.
>
> A workaround has been committed to xf86-video-intel.
>
> Signed-off-by: Pavel Roskin <proski@gnu.org>
> Cc: stable@vger.kernel.org
Similar patch:
http://lists.freedesktop.org/archives/dri-devel/2013-October/047412.html
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: never write to the userspace more data than the caller wants
2013-10-17 12:26 ` Chris Wilson
@ 2013-10-17 14:58 ` Pavel Roskin
0 siblings, 0 replies; 6+ messages in thread
From: Pavel Roskin @ 2013-10-17 14:58 UTC (permalink / raw)
To: Chris Wilson; +Cc: Dave Airlie, linux-kernel, dri-devel
On Thu, 17 Oct 2013 13:26:47 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Oct 16, 2013 at 08:12:35PM -0400, Pavel Roskin wrote:
> > The amount of data wanted by the userspace caller is encoded in the
> > ioctl number. Generic drm ioctls were ignoring it.
> >
> > As a result, Intel Xorg driver didn't work for i386 userspace on
> > x86_64 kernel on some systems. sizeof(struct
> > drm_mode_get_connector) is 76 bytes on i686 and 80 bytes on x86_64
> > due to the tail alignment (the data positions match). The
> > userspace was using the 4 bytes after the structure to hold the
> > result of the ioctl. Since drm_ioctl() was copying 80 bytes
> > instead of 76, it was clobbering that data.
> >
> > A workaround has been committed to xf86-video-intel.
> >
> > Signed-off-by: Pavel Roskin <proski@gnu.org>
> > Cc: stable@vger.kernel.org
>
> Similar patch:
> http://lists.freedesktop.org/archives/dri-devel/2013-October/047412.html
> -Chris
Wow, it's great that you also thought about it!
Your patch does almost the same thing. There is one difference. If
the userspace requests more data than the kernel needs, your patch would
trust the userspace and set usize to whatever the user wants. It would
set asize to the same value, allocating more memory than the driver
wants, up to 16383 bytes. I don't think it's a good idea for
performance reasons. My patch would decrease usize rather than increase
asize.
The code for driver-specific ioctls could be fixed too, it's just not
so urgent as fixing a real bug.
That said, I have no format objection against your patch. It would be
great to have that bug fixed.
--
Regards,
Pavel Roskin
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] drm: Restrict ioctl size to kernel struct size
2013-10-17 0:12 [PATCH] drm: never write to the userspace more data than the caller wants Pavel Roskin
2013-10-17 12:26 ` Chris Wilson
@ 2013-10-22 9:38 ` Chris Wilson
2013-10-22 10:40 ` Ville Syrjälä
2013-10-22 15:57 ` Pavel Roskin
1 sibling, 2 replies; 6+ messages in thread
From: Chris Wilson @ 2013-10-22 9:38 UTC (permalink / raw)
To: dri-devel; +Cc: Pavel Roskin
Prevent the user from passing in an ioctl command with up to 16,383
bytes specified for the struct to be allocated and copied, and
instead only allocate enough space to satisfy the kernel.
Suggested-by: Pavel Roskin <proski@gnu.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Pavel Roskin <proski@gnu.org>
Cc: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/drm_drv.c | 30 +++++++++++++-----------------
1 file changed, 13 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 05ad9ba0a67e..8c5fbc9d41ad 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -354,6 +354,16 @@ static int drm_version(struct drm_device *dev, void *data,
return err;
}
+static unsigned ioctl_size(unsigned kcmd, unsigned ucmd, unsigned *ksize, unsigned *usize)
+{
+ *ksize = _IOC_SIZE(kcmd);
+ *usize = _IOC_SIZE(ucmd);
+ if (*usize > *ksize)
+ *usize = *ksize;
+
+ return kcmd;
+}
+
/**
* Called whenever a process performs an ioctl on /dev/drm.
*
@@ -393,25 +403,11 @@ long drm_ioctl(struct file *filp,
goto err_i1;
if ((nr >= DRM_COMMAND_BASE) && (nr < DRM_COMMAND_END) &&
(nr < DRM_COMMAND_BASE + dev->driver->num_ioctls)) {
- u32 drv_size;
ioctl = &dev->driver->ioctls[nr - DRM_COMMAND_BASE];
- drv_size = _IOC_SIZE(ioctl->cmd_drv);
- usize = asize = _IOC_SIZE(cmd);
- if (drv_size > asize)
- asize = drv_size;
- cmd = ioctl->cmd_drv;
- }
- else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
- u32 drv_size;
-
+ cmd = ioctl_size(ioctl->cmd_drv, cmd, &asize, &usize);
+ } else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
ioctl = &drm_ioctls[nr];
-
- drv_size = _IOC_SIZE(ioctl->cmd);
- usize = asize = _IOC_SIZE(cmd);
- if (drv_size > asize)
- asize = drv_size;
-
- cmd = ioctl->cmd;
+ cmd = ioctl_size(ioctl->cmd, cmd, &asize, &usize);
} else
goto err_i1;
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: Restrict ioctl size to kernel struct size
2013-10-22 9:38 ` [PATCH] drm: Restrict ioctl size to kernel struct size Chris Wilson
@ 2013-10-22 10:40 ` Ville Syrjälä
2013-10-22 15:57 ` Pavel Roskin
1 sibling, 0 replies; 6+ messages in thread
From: Ville Syrjälä @ 2013-10-22 10:40 UTC (permalink / raw)
To: Chris Wilson; +Cc: Pavel Roskin, dri-devel
On Tue, Oct 22, 2013 at 10:38:03AM +0100, Chris Wilson wrote:
> Prevent the user from passing in an ioctl command with up to 16,383
> bytes specified for the struct to be allocated and copied, and
> instead only allocate enough space to satisfy the kernel.
>
> Suggested-by: Pavel Roskin <proski@gnu.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Pavel Roskin <proski@gnu.org>
> Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/drm_drv.c | 30 +++++++++++++-----------------
> 1 file changed, 13 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 05ad9ba0a67e..8c5fbc9d41ad 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -354,6 +354,16 @@ static int drm_version(struct drm_device *dev, void *data,
> return err;
> }
>
> +static unsigned ioctl_size(unsigned kcmd, unsigned ucmd, unsigned *ksize, unsigned *usize)
> +{
> + *ksize = _IOC_SIZE(kcmd);
> + *usize = _IOC_SIZE(ucmd);
> + if (*usize > *ksize)
> + *usize = *ksize;
> +
> + return kcmd;
> +}
> +
> /**
> * Called whenever a process performs an ioctl on /dev/drm.
> *
> @@ -393,25 +403,11 @@ long drm_ioctl(struct file *filp,
> goto err_i1;
> if ((nr >= DRM_COMMAND_BASE) && (nr < DRM_COMMAND_END) &&
> (nr < DRM_COMMAND_BASE + dev->driver->num_ioctls)) {
> - u32 drv_size;
> ioctl = &dev->driver->ioctls[nr - DRM_COMMAND_BASE];
> - drv_size = _IOC_SIZE(ioctl->cmd_drv);
> - usize = asize = _IOC_SIZE(cmd);
> - if (drv_size > asize)
> - asize = drv_size;
> - cmd = ioctl->cmd_drv;
> - }
> - else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
> - u32 drv_size;
> -
> + cmd = ioctl_size(ioctl->cmd_drv, cmd, &asize, &usize);
> + } else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
> ioctl = &drm_ioctls[nr];
> -
> - drv_size = _IOC_SIZE(ioctl->cmd);
> - usize = asize = _IOC_SIZE(cmd);
> - if (drv_size > asize)
> - asize = drv_size;
> -
> - cmd = ioctl->cmd;
> + cmd = ioctl_size(ioctl->cmd, cmd, &asize, &usize);
> } else
> goto err_i1;
>
> --
> 1.8.4.rc3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: Restrict ioctl size to kernel struct size
2013-10-22 9:38 ` [PATCH] drm: Restrict ioctl size to kernel struct size Chris Wilson
2013-10-22 10:40 ` Ville Syrjälä
@ 2013-10-22 15:57 ` Pavel Roskin
1 sibling, 0 replies; 6+ messages in thread
From: Pavel Roskin @ 2013-10-22 15:57 UTC (permalink / raw)
To: Chris Wilson; +Cc: dri-devel
On Tue, 22 Oct 2013 10:38:03 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Prevent the user from passing in an ioctl command with up to 16,383
> bytes specified for the struct to be allocated and copied, and
> instead only allocate enough space to satisfy the kernel.
>
> Suggested-by: Pavel Roskin <proski@gnu.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Pavel Roskin <proski@gnu.org>
> Cc: dri-devel@lists.freedesktop.org
Thank you for taking care of that!
Reviewed-by: Pavel Roskin <proski@gnu.org>
--
Regards,
Pavel Roskin
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-22 15:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-17 0:12 [PATCH] drm: never write to the userspace more data than the caller wants Pavel Roskin
2013-10-17 12:26 ` Chris Wilson
2013-10-17 14:58 ` Pavel Roskin
2013-10-22 9:38 ` [PATCH] drm: Restrict ioctl size to kernel struct size Chris Wilson
2013-10-22 10:40 ` Ville Syrjälä
2013-10-22 15:57 ` Pavel Roskin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.