Re: [Mesa-dev] [PATCH] dri/kms: Always zero out struct drm_mode_create_dumb
On Sun, Nov 16, 2014 at 01:37:52AM +, Emil Velikov wrote: On 13/11/14 18:05, Thierry Reding wrote: From: Thierry Reding tred...@nvidia.com The DRM_IOCTL_MODE_CREATE_DUMB (and others) IOCTL isn't very rigorously specified, which has the effect that some kernel drivers do not consider the .pitch and .size fields of struct drm_mode_create_dumb outputs only. Instead they will use these as lower bounds and overwrite them only if the values that they compute are larger than what userspace provided. This works if and only if userspace initializes the fields explicitly to either 0 or some meaningful value. However, if userspace just leaves the values uninitialized and the struct drm_mode_create_dumb is allocated on the stack for example, the driver may try to overallocate buffers. Fortunately most userspace does zero out the structure before passing it to the IOCTL, but there are rare exceptions. Mesa is one of them. In an attempt to rectify this situation, kernel drivers are being updated to not use the .pitch and .size fields as inputs. However in order to fix the issue with older kernels, make sure that Mesa always zeros out the structure as well. Future IOCTLs should be more rigorously defined so that structures can be validated and IOCTLs rejected if output fields aren't set to zero. Thanks Thierry. I'm pretty sure the intent here was not to misuse the API, yet again zeroing the struct sounds like a good idea. I've added Daniel's r-b and pushed this to master. I didn't mean to imply that there were any such intentions. In fact the API documents that these fields are outputs so it shouldn't be necessary to set them. As such, the Mesa code wasn't doing anything wrong. But it turns out this documentation wasn't sufficient and some drivers used the fields nevertheless. Do you think it's of any use if we push this for the stable branches ? I've not checked your drm changes, this I don't know if we actually check/validate pitch size. Is the ioctl going to carry on, throw a warning or just error out ? I don't think this needs to go into stable branches. The only drivers that were using this were ones that are unlikely to be used with Mesa. The only reason I noticed is because I've been working on a patch that lets Nouveau integrate better on Tegra, which has the side-effect of these dumb buffer allocations going through the Tegra DRM driver. The DRM changes happened in two parts: first all drivers that were abusing these fields were changed not to do that anymore. The second change was to zero out these fields in the DRM core before the IOCTL data is passed into drivers so that new drivers won't fall into the same trap. That means even for the rare cases where this is actually relevant new kernels will be able to cope with older Mesa releases. For future IOCTLs the decision was made to better document inputs and outputs as well as require outputs to be zeroed out by userspace so that the kernel can run some sanity checks and refuse an IOCTL with invalid input/output. Thierry pgpxW2UEgGUQZ.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] dri/kms: Always zero out struct drm_mode_create_dumb
On 13/11/14 18:05, Thierry Reding wrote: From: Thierry Reding tred...@nvidia.com The DRM_IOCTL_MODE_CREATE_DUMB (and others) IOCTL isn't very rigorously specified, which has the effect that some kernel drivers do not consider the .pitch and .size fields of struct drm_mode_create_dumb outputs only. Instead they will use these as lower bounds and overwrite them only if the values that they compute are larger than what userspace provided. This works if and only if userspace initializes the fields explicitly to either 0 or some meaningful value. However, if userspace just leaves the values uninitialized and the struct drm_mode_create_dumb is allocated on the stack for example, the driver may try to overallocate buffers. Fortunately most userspace does zero out the structure before passing it to the IOCTL, but there are rare exceptions. Mesa is one of them. In an attempt to rectify this situation, kernel drivers are being updated to not use the .pitch and .size fields as inputs. However in order to fix the issue with older kernels, make sure that Mesa always zeros out the structure as well. Future IOCTLs should be more rigorously defined so that structures can be validated and IOCTLs rejected if output fields aren't set to zero. Thanks Thierry. I'm pretty sure the intent here was not to misuse the API, yet again zeroing the struct sounds like a good idea. I've added Daniel's r-b and pushed this to master. Do you think it's of any use if we push this for the stable branches ? I've not checked your drm changes, this I don't know if we actually check/validate pitch size. Is the ioctl going to carry on, throw a warning or just error out ? Cheers, Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] dri/kms: Always zero out struct drm_mode_create_dumb
On Thu, Nov 13, 2014 at 07:05:51PM +0100, Thierry Reding wrote: From: Thierry Reding tred...@nvidia.com The DRM_IOCTL_MODE_CREATE_DUMB (and others) IOCTL isn't very rigorously specified, which has the effect that some kernel drivers do not consider the .pitch and .size fields of struct drm_mode_create_dumb outputs only. Instead they will use these as lower bounds and overwrite them only if the values that they compute are larger than what userspace provided. This works if and only if userspace initializes the fields explicitly to either 0 or some meaningful value. However, if userspace just leaves the values uninitialized and the struct drm_mode_create_dumb is allocated on the stack for example, the driver may try to overallocate buffers. Fortunately most userspace does zero out the structure before passing it to the IOCTL, but there are rare exceptions. Mesa is one of them. In an attempt to rectify this situation, kernel drivers are being updated to not use the .pitch and .size fields as inputs. However in order to fix the issue with older kernels, make sure that Mesa always zeros out the structure as well. Future IOCTLs should be more rigorously defined so that structures can be validated and IOCTLs rejected if output fields aren't set to zero. Signed-off-by: Thierry Reding tred...@nvidia.com Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch --- src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 2 +- src/gbm/backends/dri/gbm_dri.c| 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c index ee71027c5872..507983ec251a 100644 --- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c +++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c @@ -131,10 +131,10 @@ kms_sw_displaytarget_create(struct sw_winsys *ws, kms_sw_dt-width = width; kms_sw_dt-height = height; + memset(create_req, 0, sizeof(create_req)); create_req.bpp = 32; create_req.width = width; create_req.height = height; - create_req.handle = 0; ret = drmIoctl(kms_sw-fd, DRM_IOCTL_MODE_CREATE_DUMB, create_req); if (ret) goto free_bo; diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri.c index 470a54f3dd89..bc263297ec33 100644 --- a/src/gbm/backends/dri/gbm_dri.c +++ b/src/gbm/backends/dri/gbm_dri.c @@ -774,6 +774,7 @@ create_dumb(struct gbm_device *gbm, if (bo == NULL) return NULL; + memset(create_arg, 0, sizeof(create_arg)); create_arg.bpp = 32; create_arg.width = width; create_arg.height = height; -- 2.1.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
