Message ID | 1498145294-7974-2-git-send-email-stefan.bader@canonical.com |
---|---|
State | New |
Headers | show |
On 22/06/17 16:28, Stefan Bader wrote: > From: Sinclair Yeh <syeh@vmware.com> > > When vmw_gb_surface_define_ioctl() is called with an existing buffer, > we end up returning an uninitialized variable in the backup_handle. > > The fix is to first initialize backup_handle to 0 just to be sure, and > second, when a user-provided buffer is found, we will use the > req->buffer_handle as the backup_handle. > > Cc: <stable@vger.kernel.org> > Reported-by: Murray McAllister <murray.mcallister@insomniasec.com> > Signed-off-by: Sinclair Yeh <syeh@vmware.com> > Reviewed-by: Deepak Rawat <drawat@vmware.com> > > CVE-2017-9605 > > (backported from commit 07678eca2cf9c9a18584e546c2b2a0d0c9a3150c) > Signed-off-by: Stefan Bader <stefan.bader@canonical.com> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > index 8a98b88..9593df6 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > @@ -1178,7 +1178,7 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, > uint32_t size; > struct vmw_master *vmaster = vmw_master(file_priv->master); > const struct svga3d_surface_desc *desc; > - uint32_t backup_handle; > + uint32_t backup_handle = 0; > > if (req->mip_levels > DRM_VMW_MAX_MIP_LEVELS) > return -EINVAL; > @@ -1247,6 +1247,17 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, > if (req->buffer_handle != SVGA3D_INVALID_ID) { > ret = vmw_user_dmabuf_lookup(tfile, req->buffer_handle, > &res->backup); > + if (ret == 0) { > + if (res->backup->base.num_pages * PAGE_SIZE < > + res->backup_size) { > + DRM_ERROR("Surface backup buffer is too small.\n"); > + vmw_dmabuf_unreference(&res->backup); > + ret = -EINVAL; > + goto out_unlock; > + } > + } else { > + backup_handle = req->buffer_handle; > + } > } else if (req->drm_surface_flags & > drm_vmw_surface_flag_create_buffer) > ret = vmw_user_dmabuf_alloc(dev_priv, tfile, > Seems OK to me as a backport. Acked-by: Colin Ian King <colin.king@canonical.com>
On Thu, Jun 22, 2017 at 05:28:13PM +0200, Stefan Bader wrote: > From: Sinclair Yeh <syeh@vmware.com> > > When vmw_gb_surface_define_ioctl() is called with an existing buffer, > we end up returning an uninitialized variable in the backup_handle. > > The fix is to first initialize backup_handle to 0 just to be sure, and > second, when a user-provided buffer is found, we will use the > req->buffer_handle as the backup_handle. > > Cc: <stable@vger.kernel.org> > Reported-by: Murray McAllister <murray.mcallister@insomniasec.com> > Signed-off-by: Sinclair Yeh <syeh@vmware.com> > Reviewed-by: Deepak Rawat <drawat@vmware.com> > > CVE-2017-9605 > > (backported from commit 07678eca2cf9c9a18584e546c2b2a0d0c9a3150c) > Signed-off-by: Stefan Bader <stefan.bader@canonical.com> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > index 8a98b88..9593df6 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > @@ -1178,7 +1178,7 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, > uint32_t size; > struct vmw_master *vmaster = vmw_master(file_priv->master); > const struct svga3d_surface_desc *desc; > - uint32_t backup_handle; > + uint32_t backup_handle = 0; > > if (req->mip_levels > DRM_VMW_MAX_MIP_LEVELS) > return -EINVAL; > @@ -1247,6 +1247,17 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, > if (req->buffer_handle != SVGA3D_INVALID_ID) { > ret = vmw_user_dmabuf_lookup(tfile, req->buffer_handle, > &res->backup); > + if (ret == 0) { > + if (res->backup->base.num_pages * PAGE_SIZE < > + res->backup_size) { > + DRM_ERROR("Surface backup buffer is too small.\n"); > + vmw_dmabuf_unreference(&res->backup); > + ret = -EINVAL; > + goto out_unlock; > + } > + } else { > + backup_handle = req->buffer_handle; > + } > } else if (req->drm_surface_flags & > drm_vmw_surface_flag_create_buffer) > ret = vmw_user_dmabuf_alloc(dev_priv, tfile, > -- > 2.7.4 Commit d80efd5cb3dec16a8d1aea9b8a4a7921972dba65 ("drm/vmwgfx: Initial DX support"), which introduces that check is a complex change, so I understand that it's not included. As far as I could check whether that test would be OK, I noticed a BUG_ON at drivers/gpu/drm/vmwgfx/vmwgfx_resource.c:vmw_resource_buf_alloc, which indicates it might be correct to check for that. Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Applied to trusty master-next branch. Thanks. Cascardo.
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index 8a98b88..9593df6 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -1178,7 +1178,7 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, uint32_t size; struct vmw_master *vmaster = vmw_master(file_priv->master); const struct svga3d_surface_desc *desc; - uint32_t backup_handle; + uint32_t backup_handle = 0; if (req->mip_levels > DRM_VMW_MAX_MIP_LEVELS) return -EINVAL; @@ -1247,6 +1247,17 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, if (req->buffer_handle != SVGA3D_INVALID_ID) { ret = vmw_user_dmabuf_lookup(tfile, req->buffer_handle, &res->backup); + if (ret == 0) { + if (res->backup->base.num_pages * PAGE_SIZE < + res->backup_size) { + DRM_ERROR("Surface backup buffer is too small.\n"); + vmw_dmabuf_unreference(&res->backup); + ret = -EINVAL; + goto out_unlock; + } + } else { + backup_handle = req->buffer_handle; + } } else if (req->drm_surface_flags & drm_vmw_surface_flag_create_buffer) ret = vmw_user_dmabuf_alloc(dev_priv, tfile,