[Mesa-dev] [PATCH 07/10] anv: Add sizeless anv_bo_cache_import()
Jason Ekstrand
jason at jlekstrand.net
Tue Oct 17 18:11:46 UTC 2017
On Tue, Oct 17, 2017 at 10:52 AM, Chad Versace <chadversary at chromium.org>
wrote:
> 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...".
>
I don't think trying ti import in multiple threads will actually affect
anything. They'll all get a BO (some of them will get the same one) and
the BO size will be wrong in all of them and they will all reject it. If
I'm missing something, I'm happy to know what it is.
> >
> >
> > + 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171017/dcad3aac/attachment-0001.html>
More information about the mesa-dev
mailing list