On Tue, Oct 17, 2017 at 5:27 PM, Chad Versace <chadvers...@chromium.org>
wrote:
> This change prepares for VK_ANDROID_native_buffer. When the user imports
> a gralloc hande into a VkImage using VK_ANDROID_native_buffer, the user
> provides no size. The driver must infer the size from the internals of
> the gralloc buffer.
>
> The patch is essentially a refactor patch. It should change no behavior
> other than improved debug messages.
> ---
>
> Jason, you were right about the race; it poses no security problem.
>
> This patch replaces 07/10 in the series.
>
>
>
> src/intel/vulkan/anv_allocator.c | 21 +++------------------
> src/intel/vulkan/anv_device.c | 21 +++++++++++++++++++--
> src/intel/vulkan/anv_intel.c | 12 +++++++++++-
> src/intel/vulkan/anv_private.h | 2 +-
> src/intel/vulkan/anv_queue.c | 7 ++++++-
> 5 files changed, 40 insertions(+), 23 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_allocator.c b/src/intel/vulkan/anv_
> allocator.c
> index 401cea40e6..27eedb53aa 100644
> --- a/src/intel/vulkan/anv_allocator.c
> +++ b/src/intel/vulkan/anv_allocator.c
> @@ -1272,13 +1272,10 @@ anv_bo_cache_alloc(struct anv_device *device,
> VkResult
> anv_bo_cache_import(struct anv_device *device,
> struct anv_bo_cache *cache,
> - int fd, uint64_t size, struct anv_bo **bo_out)
> + int fd, struct anv_bo **bo_out)
> {
> pthread_mutex_lock(&cache->mutex);
>
> - /* The kernel is going to give us whole pages anyway */
> - size = align_u64(size, 4096);
> -
> uint32_t gem_handle = anv_gem_fd_to_handle(device, fd);
> if (!gem_handle) {
> pthread_mutex_unlock(&cache->mutex);
> @@ -1287,22 +1284,10 @@ anv_bo_cache_import(struct anv_device *device,
>
> struct anv_cached_bo *bo = anv_bo_cache_lookup_locked(cache,
> gem_handle);
> if (bo) {
> - if (bo->bo.size != size) {
> - pthread_mutex_unlock(&cache->mutex);
> - return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHR);
> - }
> __sync_fetch_and_add(&bo->refcount, 1);
> } else {
> - /* For security purposes, we reject BO imports where the size does
> not
> - * match exactly. This prevents a malicious client from passing a
> - * buffer to a trusted client, lying about the size, and telling the
> - * trusted client to try and texture from an image that goes
> - * out-of-bounds. This sort of thing could lead to GPU hangs or
> worse
> - * in the trusted client. The trusted client can protect itself
> against
> - * this sort of attack but only if it can trust the buffer size.
> - */
> - off_t import_size = lseek(fd, 0, SEEK_END);
> - if (import_size == (off_t)-1 || import_size < size) {
> + off_t size = lseek(fd, 0, SEEK_END);
> + if (size == (off_t)-1) {
> anv_gem_close(device, gem_handle);
> pthread_mutex_unlock(&cache->mutex);
> return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHR);
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index 1634b5158c..900de9778c 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -1543,11 +1543,28 @@ VkResult anv_AllocateMemory(
> VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_FD_BIT_KHR);
>
> result = anv_bo_cache_import(device, &device->bo_cache,
> - fd_info->fd,
> pAllocateInfo->allocationSize,
> - &mem->bo);
> + fd_info->fd, &mem->bo);
> if (result != VK_SUCCESS)
> goto fail;
>
> + /* For security purposes, we reject BO imports where the size does
> not
> + * match exactly. This prevents a malicious client from passing
> + * a buffer to a trusted client, lying about the size, and telling
> the
> + * trusted client to try and texture from an image that goes
> + * out-of-bounds. This sort of thing could lead to GPU hangs or
> worse
> + * in the trusted client. The trusted client can protect itself
> against
> + * this sort of attack but only if it can trust the buffer size.
> + */
> + if (pAllocateInfo->allocationSize != mem->bo->size) {
>
You're not checking the aligned size like we were before. Maybe make that
align_u64(pAllocateInfo->allocationSize, 4096)? In any case, I don't think
it matters because the client will be getting the size based on an image
query which will likely be a multiple of 4k. Still, it'd be nice to not
have a behavioral change. With that,
Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net>
> + result = vk_errorf(device->instace, device,
> + VK_ERROR_INVALID_EXTERNAL_HANDLE_KHR,
> + "invalid allocationSize for
> VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_FD_BIT_KHR: "
> + "expected %"PRIu64"B, actual %"PRIu64"B",
> + mem->bo->size, pAllocateInfo->allocationSize)
> ;
> + anv_bo_cache_release(device, &device->bo_cache, mem->bo);
> + goto fail;
> + }
> +
> /* From the Vulkan spec:
> *
> * "Importing memory from a file descriptor transfers ownership
> of
> diff --git a/src/intel/vulkan/anv_intel.c b/src/intel/vulkan/anv_intel.c
> index d6bad44091..a80b1e4623 100644
> --- a/src/intel/vulkan/anv_intel.c
> +++ b/src/intel/vulkan/anv_intel.c
> @@ -76,10 +76,20 @@ VkResult anv_CreateDmaBufImageINTEL(
> image = anv_image_from_handle(image_h);
>
> result = anv_bo_cache_import(device, &device->bo_cache,
> - pCreateInfo->fd, image->size, &mem->bo);
> + pCreateInfo->fd, &mem->bo);
> if (result != VK_SUCCESS)
> goto fail_import;
>
> + if (image->size != mem->bo->size) {
> + result = vk_errorf(device->instace, device,
> + VK_ERROR_INVALID_EXTERNAL_HANDLE_KHR,
> + "invalid dma-buf size in
> vkCreateDmaBufImageINTEL: "
> + "expected %"PRIu64"B, actual %"PRIu64"B",
> + image->size, mem->bo->size);
> + anv_bo_cache_release(device, &device->bo_cache, mem->bo);
> + goto fail_import;
> + }
> +
> if (device->instance->physicalDevice.supports_48bit_addresses)
> mem->bo->flags |= EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> private.h
> index 8af3f5c69e..61e23282b0 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -718,7 +718,7 @@ VkResult anv_bo_cache_alloc(struct anv_device *device,
> uint64_t size, struct anv_bo **bo);
> VkResult anv_bo_cache_import(struct anv_device *device,
> struct anv_bo_cache *cache,
> - int fd, uint64_t size, struct anv_bo **bo);
> + int fd, struct anv_bo **bo);
> VkResult anv_bo_cache_export(struct anv_device *device,
> struct anv_bo_cache *cache,
> struct anv_bo *bo_in, int *fd_out);
> diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c
> index 7e675e2274..63335490fd 100644
> --- a/src/intel/vulkan/anv_queue.c
> +++ b/src/intel/vulkan/anv_queue.c
> @@ -1023,10 +1023,15 @@ VkResult anv_ImportSemaphoreFdKHR(
> new_impl.type = ANV_SEMAPHORE_TYPE_BO;
>
> VkResult result = anv_bo_cache_import(device, &device->bo_cache,
> - fd, 4096, &new_impl.bo);
> + fd, &new_impl.bo);
> if (result != VK_SUCCESS)
> return result;
>
> + if (new_impl.bo->size != 4096) {
> + anv_bo_cache_release(device, &device->bo_cache, new_impl.bo);
> + return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHR);
> + }
> +
> /* If we're going to use this as a fence, we need to *not* have
> the
> * EXEC_OBJECT_ASYNC bit set.
> */
> --
> 2.13.0
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
