| Message ID | 1381913342-17634-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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.
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 >
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)
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(+)