[PATCH] drm: Pad drm_mode_get_connector to 64-bit boundary

[PATCH] drm: Pad drm_mode_get_connector to 64-bit boundary

[Chris Wilson]

Pavel Roskin reported that DRM_IOCTL_MODE_GETCONNECTOR was overwritting
the 4 bytes beyond the end of its structure with a 32-bit userspace
running on a 64-bit kernel. This is due to the padding gcc inserts as
the drm_mode_get_connector struct includes a u64 and its size is not a
natural multiple of u64s.

64-bit kernel:

sizeof(drm_mode_get_connector)=80, alignof=8
sizeof(drm_mode_get_encoder)=20, alignof=4
sizeof(drm_mode_modeinfo)=68, alignof=4

32-bit userspace:

sizeof(drm_mode_get_connector)=76, alignof=4
sizeof(drm_mode_get_encoder)=20, alignof=4
sizeof(drm_mode_modeinfo)=68, alignof=4

Fortuituously we can insert explicit padding to the tail of our
structures without breaking ABI.

Reported-by: Pavel Roskin <proski@gnu.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Cc: stable@vger.kernel.org
---
 include/uapi/drm/drm_mode.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 550811712f78..28acbaf4a81e 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -223,6 +223,8 @@ struct drm_mode_get_connector {
 	__u32 connection;
 	__u32 mm_width, mm_height; /**< HxW in millimeters */
 	__u32 subpixel;
+
+	__u32 pad;
 };
 
 #define DRM_MODE_PROP_PENDING	(1<<0)
-- 
1.8.4.rc3

Re: [PATCH] drm: Pad drm_mode_get_connector to 64-bit boundary

[Ville Syrjälä]

On Wed, Oct 16, 2013 at 09:49:02AM +0100, Chris Wilson wrote:
> Pavel Roskin reported that DRM_IOCTL_MODE_GETCONNECTOR was overwritting
> the 4 bytes beyond the end of its structure with a 32-bit userspace
> running on a 64-bit kernel. This is due to the padding gcc inserts as
> the drm_mode_get_connector struct includes a u64 and its size is not a
> natural multiple of u64s.
> 
> 64-bit kernel:
> 
> sizeof(drm_mode_get_connector)=80, alignof=8
> sizeof(drm_mode_get_encoder)=20, alignof=4
> sizeof(drm_mode_modeinfo)=68, alignof=4
> 
> 32-bit userspace:
> 
> sizeof(drm_mode_get_connector)=76, alignof=4
> sizeof(drm_mode_get_encoder)=20, alignof=4
> sizeof(drm_mode_modeinfo)=68, alignof=4
> 
> Fortuituously we can insert explicit padding to the tail of our
> structures without breaking ABI.
> 
> Reported-by: Pavel Roskin <proski@gnu.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: stable@vger.kernel.org

Hmm. But that only fixes things if you recompile the 32bit userland
code.

We could also fix old 32bit userland by adopting the same kind of size
handling that we use for driver specific ioctls. The code is already
there, we just need to set asize and usize appropriately.

> ---
>  include/uapi/drm/drm_mode.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 550811712f78..28acbaf4a81e 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -223,6 +223,8 @@ struct drm_mode_get_connector {
>  	__u32 connection;
>  	__u32 mm_width, mm_height; /**< HxW in millimeters */
>  	__u32 subpixel;
> +
> +	__u32 pad;
>  };
>  
>  #define DRM_MODE_PROP_PENDING	(1<<0)
> -- 
> 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

Re: [PATCH] drm: Pad drm_mode_get_connector to 64-bit boundary

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2496 bytes --]

On Wed, 2013-10-16 at 13:09 +0300, Ville Syrjälä wrote:
> On Wed, Oct 16, 2013 at 09:49:02AM +0100, Chris Wilson wrote:
> > Pavel Roskin reported that DRM_IOCTL_MODE_GETCONNECTOR was overwritting
> > the 4 bytes beyond the end of its structure with a 32-bit userspace
> > running on a 64-bit kernel. This is due to the padding gcc inserts as
> > the drm_mode_get_connector struct includes a u64 and its size is not a
> > natural multiple of u64s.
> > 
> > 64-bit kernel:
> > 
> > sizeof(drm_mode_get_connector)=80, alignof=8
> > sizeof(drm_mode_get_encoder)=20, alignof=4
> > sizeof(drm_mode_modeinfo)=68, alignof=4
> > 
> > 32-bit userspace:
> > 
> > sizeof(drm_mode_get_connector)=76, alignof=4
> > sizeof(drm_mode_get_encoder)=20, alignof=4
> > sizeof(drm_mode_modeinfo)=68, alignof=4
> > 
> > Fortuituously we can insert explicit padding to the tail of our
> > structures without breaking ABI.
> > 
> > Reported-by: Pavel Roskin <proski@gnu.org>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: stable@vger.kernel.org
> 
> Hmm. But that only fixes things if you recompile the 32bit userland
> code.

Which is not a fix at all, but an even worse ABI break (now 32-bit
kernels will overrun userland buffers too).

> We could also fix old 32bit userland by adopting the same kind of size
> handling that we use for driver specific ioctls. The code is already
> there, we just need to set asize and usize appropriately.

Right, you have to do something like that.

Ben.

> > ---
> >  include/uapi/drm/drm_mode.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 550811712f78..28acbaf4a81e 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -223,6 +223,8 @@ struct drm_mode_get_connector {
> >  	__u32 connection;
> >  	__u32 mm_width, mm_height; /**< HxW in millimeters */
> >  	__u32 subpixel;
> > +
> > +	__u32 pad;
> >  };
> >  
> >  #define DRM_MODE_PROP_PENDING	(1<<0)
> > -- 
> > 1.8.4.rc3
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Ben Hutchings
Horngren's Observation:
                   Among economists, the real world is often a special case.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] drm: Pad drm_mode_get_connector to 64-bit boundary

[Jani Nikula]

On Wed, 16 Oct 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Pavel Roskin reported that DRM_IOCTL_MODE_GETCONNECTOR was overwritting
> the 4 bytes beyond the end of its structure with a 32-bit userspace
> running on a 64-bit kernel. This is due to the padding gcc inserts as
> the drm_mode_get_connector struct includes a u64 and its size is not a
> natural multiple of u64s.
>
> 64-bit kernel:
>
> sizeof(drm_mode_get_connector)=80, alignof=8
> sizeof(drm_mode_get_encoder)=20, alignof=4
> sizeof(drm_mode_modeinfo)=68, alignof=4
>
> 32-bit userspace:
>
> sizeof(drm_mode_get_connector)=76, alignof=4
> sizeof(drm_mode_get_encoder)=20, alignof=4
> sizeof(drm_mode_modeinfo)=68, alignof=4
>
> Fortuituously we can insert explicit padding to the tail of our
> structures without breaking ABI.
>
> Reported-by: Pavel Roskin <proski@gnu.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: stable@vger.kernel.org
> ---
>  include/uapi/drm/drm_mode.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 550811712f78..28acbaf4a81e 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -223,6 +223,8 @@ struct drm_mode_get_connector {
>  	__u32 connection;
>  	__u32 mm_width, mm_height; /**< HxW in millimeters */
>  	__u32 subpixel;
> +
> +	__u32 pad;

Would a small comment here be in order?

Jani.

>  };
>  
>  #define DRM_MODE_PROP_PENDING	(1<<0)
> -- 
> 1.8.4.rc3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm: Pad drm_mode_get_connector to 64-bit boundary

[Chris Wilson]

On Wed, Oct 16, 2013 at 01:14:53PM +0300, Jani Nikula wrote:
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -223,6 +223,8 @@ struct drm_mode_get_connector {
> >  	__u32 connection;
> >  	__u32 mm_width, mm_height; /**< HxW in millimeters */
> >  	__u32 subpixel;
> > +
> > +	__u32 pad;
> 
> Would a small comment here be in order?

We haven't explained padding in the past. It has been taken for granted
that the pad members are there to make compat problems disappear.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm: Pad drm_mode_get_connector to 64-bit boundary

[Jani Nikula]

On Wed, 16 Oct 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Oct 16, 2013 at 01:14:53PM +0300, Jani Nikula wrote:
>> > --- a/include/uapi/drm/drm_mode.h
>> > +++ b/include/uapi/drm/drm_mode.h
>> > @@ -223,6 +223,8 @@ struct drm_mode_get_connector {
>> >  	__u32 connection;
>> >  	__u32 mm_width, mm_height; /**< HxW in millimeters */
>> >  	__u32 subpixel;
>> > +
>> > +	__u32 pad;
>> 
>> Would a small comment here be in order?
>
> We haven't explained padding in the past. It has been taken for granted
> that the pad members are there to make compat problems disappear.

Okay, sorry for the noise.

J.

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH] drm: Prevent overwriting from userspace underallocating core ioctl structs

[Chris Wilson]

Apply the protections from

commit 1b2f1489633888d4a06028315dc19d65768a1c05
Author: Dave Airlie <airlied@redhat.com>
Date:   Sat Aug 14 20:20:34 2010 +1000

    drm: block userspace under allocating buffer and having drivers overwrite it (v2)

to the core ioctl structs as well, for we found one instance where there
is a 32-/64-bit size mismatch and were guilty of writing beyond the end
of the user's buffer.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/drm_drv.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index e572dd20bdee..05ad9ba0a67e 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -402,9 +402,16 @@ long drm_ioctl(struct file *filp,
 		cmd = ioctl->cmd_drv;
 	}
 	else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
+		u32 drv_size;
+
 		ioctl = &drm_ioctls[nr];
-		cmd = ioctl->cmd;
+
+		drv_size = _IOC_SIZE(ioctl->cmd);
 		usize = asize = _IOC_SIZE(cmd);
+		if (drv_size > asize)
+			asize = drv_size;
+
+		cmd = ioctl->cmd;
 	} else
 		goto err_i1;
 
-- 
1.8.4.rc3

Re: [PATCH] drm: Prevent overwriting from userspace underallocating core ioctl structs

[Ville Syrjälä]

On Wed, Oct 16, 2013 at 11:22:44AM +0100, Chris Wilson wrote:
> Apply the protections from
> 
> commit 1b2f1489633888d4a06028315dc19d65768a1c05
> Author: Dave Airlie <airlied@redhat.com>
> Date:   Sat Aug 14 20:20:34 2010 +1000
> 
>     drm: block userspace under allocating buffer and having drivers overwrite it (v2)
> 
> to the core ioctl structs as well, for we found one instance where there
> is a 32-/64-bit size mismatch and were guilty of writing beyond the end
> of the user's buffer.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: stable@vger.kernel.org

Looks good.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_drv.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index e572dd20bdee..05ad9ba0a67e 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -402,9 +402,16 @@ long drm_ioctl(struct file *filp,
>  		cmd = ioctl->cmd_drv;
>  	}
>  	else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
> +		u32 drv_size;
> +
>  		ioctl = &drm_ioctls[nr];
> -		cmd = ioctl->cmd;
> +
> +		drv_size = _IOC_SIZE(ioctl->cmd);
>  		usize = asize = _IOC_SIZE(cmd);
> +		if (drv_size > asize)
> +			asize = drv_size;
> +
> +		cmd = ioctl->cmd;
>  	} else
>  		goto err_i1;
>  
> -- 
> 1.8.4.rc3

-- 
Ville Syrjälä
Intel OTC