[Mesa-dev] [PATCH 07/10] anv: Add sizeless anv_bo_cache_import()
Chad Versace
chadversary at chromium.org
Tue Oct 17 17:52:40 UTC 2017
On Mon 16 Oct 2017, Jason Ekstrand wrote:
> On Mon, Oct 16, 2017 at 11:55 AM, Chad Versace <[1]chadversary at chromium.org>
> wrote:
>
> The patch renames anv_bo_cache_import() to
> anv_bo_cache_import_with_size(); and adds a new variant,
> anv_bo_cache_import(), that lacks the 'size' parameter. Same as
> pre-patch, anv_bo_cache_import_with_size() continues to validate the
> imported size.
>
> The patch is essentially a refactor patch. It should change no existing
> behavior.
>
> This split makes it easier to implement 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.
> ---
> src/intel/vulkan/anv_allocator.c | 118 +++++++++++++++++++++++++-----
> ---------
> src/intel/vulkan/anv_device.c | 7 ++-
> src/intel/vulkan/anv_intel.c | 5 +-
> src/intel/vulkan/anv_private.h | 3 +
> src/intel/vulkan/anv_queue.c | 5 +-
> 5 files changed, 90 insertions(+), 48 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_allocator.c b/src/intel/vulkan/anv_
> allocator.c
> index 401cea40e6..1deecc36d7 100644
> --- a/src/intel/vulkan/anv_allocator.c
> +++ b/src/intel/vulkan/anv_allocator.c
> @@ -1269,62 +1269,98 @@ anv_bo_cache_alloc(struct anv_device *device,
> return VK_SUCCESS;
> }
>
> +static VkResult
> +anv_bo_cache_import_locked(struct anv_device *device,
> + struct anv_bo_cache *cache,
> + int fd, struct anv_bo **bo_out)
> +{
> + uint32_t gem_handle = anv_gem_fd_to_handle(device, fd);
> + if (!gem_handle)
> + return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHR);
> +
> + struct anv_cached_bo *bo = anv_bo_cache_lookup_locked(cache,
> gem_handle);
> + if (bo) {
> + __sync_fetch_and_add(&bo->refcount, 1);
> + return VK_SUCCESS;
> + }
> +
> + off_t size = lseek(fd, 0, SEEK_END);
> + if (size == (off_t)-1) {
> + anv_gem_close(device, gem_handle);
> + return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHR);
> + }
> +
> + bo = vk_alloc(&device->alloc, sizeof(struct anv_cached_bo), 8,
> + VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
> + if (!bo) {
> + anv_gem_close(device, gem_handle);
> + return vk_error(VK_ERROR_OUT_OF_HOST_MEMORY);
> + }
> +
> + bo->refcount = 1;
> + anv_bo_init(&bo->bo, gem_handle, size);
> + _mesa_hash_table_insert(cache->bo_map, (void *)(uintptr_t)gem_handle,
> bo);
> + *bo_out = &bo->bo;
> +
> + return VK_SUCCESS;
> +}
> +
> 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)
> {
> + VkResult result;
> +
> pthread_mutex_lock(&cache->mutex);
> + result = anv_bo_cache_import_locked(device, cache, fd, bo_out);
> + pthread_mutex_unlock(&cache->mutex);
> +
> + return result;
> +}
> +
> +VkResult
> +anv_bo_cache_import_with_size(struct anv_device *device,
> + struct anv_bo_cache *cache,
> + int fd, uint64_t size,
> + struct anv_bo **bo_out)
>
>
> I don't really like this function... I'd rather we do the size checks in the
> caller but we can do that refactor later.
I don't believe the caller can check the size securely. See my comments
below.
>
> +{
> + struct anv_bo *bo = NULL;
> + VkResult result;
>
> /* 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_lock(&cache->mutex);
> +
> + result = anv_bo_cache_import_locked(device, cache, fd, &bo);
> + if (result != VK_SUCCESS) {
> pthread_mutex_unlock(&cache->mutex);
> - return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHR);
> + return result;
> }
>
> - 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) {
> - anv_gem_close(device, gem_handle);
> - pthread_mutex_unlock(&cache->mutex);
> - return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHR);
> - }
> -
> - bo = vk_alloc(&device->alloc, sizeof(struct anv_cached_bo), 8,
> - VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
> - if (!bo) {
> - anv_gem_close(device, gem_handle);
> - pthread_mutex_unlock(&cache->mutex);
> - return vk_error(VK_ERROR_OUT_OF_HOST_MEMORY);
> - }
> -
> - bo->refcount = 1;
> -
> - anv_bo_init(&bo->bo, gem_handle, size);
> -
> - _mesa_hash_table_insert(cache->bo_map, (void *)(uintptr_t)
> gem_handle, bo);
> + /* 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.
> + *
> + * To successfully prevent that attack, we must check the fd's size and
> + * complete the fd's import into the cache during the same critical
> + * section. Otherwise, if the cache were unlocked between importing
> the fd
> + * and checking the its size, then the attacker could exploit a race by
> + * importing the fd concurrently in separate threads.
> + */
> + if (bo->size != size) {
>
>
> FYI: There are patches in-flight to make this a < rather than a !=.
Should I replace < with != now?
>
>
> + anv_bo_cache_release(device, cache, bo);
> + pthread_mutex_unlock(&cache->mutex);
>
>
> This is a problem... anv_bo_cache_release will try to take the lock but it's
> already held here.
Right. anv_bo_cache_release() locks/unlocks the mutex while
anv_bo_cache_import_with_size() holds the lock. Oops.
One solution, which you won't like, is to add a new function
anv_bo_cache_release_locked() and call that within
anv_bo_cache_import_with_size().
> I think the solution is to just not separate bo_cach_import
> from bo_cache_import_locked and let bo_cache_import do all the locking.
> There's no harm in doing an import and then releasing it later if the size is
> wrong.
I believe it's unsafe to complete the import then release it if the size
is wrong. I explained that in the comment above that begins "To
successfully prevent that attack...".
>
>
> + return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHR);
> }
>
> pthread_mutex_unlock(&cache->mutex);
> - *bo_out = &bo->bo;
> + *bo_out = bo;
>
> return VK_SUCCESS;
> }
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index 1634b5158c..94bbf8c819 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -1542,9 +1542,10 @@ VkResult anv_AllocateMemory(
> assert(fd_info->handleType ==
> 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);
> + result = anv_bo_cache_import_with_size(device, &device->bo_cache,
> + fd_info->fd,
> + pAllocateInfo->
> allocationSize,
> + &mem->bo);
> if (result != VK_SUCCESS)
> goto fail;
>
> diff --git a/src/intel/vulkan/anv_intel.c b/src/intel/vulkan/anv_intel.c
> index d6bad44091..9c1321a065 100644
> --- a/src/intel/vulkan/anv_intel.c
> +++ b/src/intel/vulkan/anv_intel.c
> @@ -75,8 +75,9 @@ 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);
> + result = anv_bo_cache_import_with_size(device, &device->bo_cache,
> + pCreateInfo->fd, image->size,
> + &mem->bo);
> if (result != VK_SUCCESS)
> goto fail_import;
>
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> private.h
> index 27d2c34203..ecdd9d5e32 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -717,6 +717,9 @@ VkResult anv_bo_cache_alloc(struct anv_device *device,
> struct anv_bo_cache *cache,
> uint64_t size, struct anv_bo **bo);
> VkResult anv_bo_cache_import(struct anv_device *device,
> + struct anv_bo_cache *cache,
> + int fd, struct anv_bo **bo);
> +VkResult anv_bo_cache_import_with_size(struct anv_device *device,
> struct anv_bo_cache *cache,
> int fd, uint64_t size, struct anv_bo **bo);
> VkResult anv_bo_cache_export(struct anv_device *device,
> diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c
> index aff7c53119..e26254a87e 100644
> --- a/src/intel/vulkan/anv_queue.c
> +++ b/src/intel/vulkan/anv_queue.c
> @@ -1006,6 +1006,7 @@ VkResult anv_ImportSemaphoreFdKHR(
> ANV_FROM_HANDLE(anv_device, device, _device);
> ANV_FROM_HANDLE(anv_semaphore, semaphore, pImportSemaphoreFdInfo->
> semaphore);
> int fd = pImportSemaphoreFdInfo->fd;
> + VkResult result;
>
> struct anv_semaphore_impl new_impl = {
> .type = ANV_SEMAPHORE_TYPE_NONE,
> @@ -1033,8 +1034,8 @@ VkResult anv_ImportSemaphoreFdKHR(
> } else {
> new_impl.type = ANV_SEMAPHORE_TYPE_BO;
>
> - VkResult result = anv_bo_cache_import(device, &device->bo_cache,
> - fd, 4096, &[2]new_impl.bo);
> + result = anv_bo_cache_import_with_size(device, &device->bo_cache,
> fd,
> + 4096, &[3]new_impl.bo);
> if (result != VK_SUCCESS)
> return result;
>
> --
> 2.13.0
>
> _______________________________________________
> mesa-dev mailing list
> [4]mesa-dev at lists.freedesktop.org
> [5]https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
> References:
>
> [1] mailto:chadversary at chromium.org
> [2] http://new_impl.bo/
> [3] http://new_impl.bo/
> [4] mailto:mesa-dev at lists.freedesktop.org
> [5] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list