<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Oct 17, 2017 at 10:52 AM, Chad Versace <<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon 16 Oct 2017, Jason Ekstrand wrote: > On Mon, Oct 16, 2017 at 11:55 AM, Chad Versace <[1]<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>> <div><div class="h5">> 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. </div></div>I don't believe the caller can check the size securely. See my comments below. <div><div class="h5"> > > +{ > + 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 !=. </div></div>Should I replace < with != now? </blockquote><div> </div><div>No, that's a functional change; this is a refactor. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > > + 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...". <div><div class="h5"> > > > + 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, </div></div>> - fd, 4096, &[2]<a href="http://new_impl.bo" rel="noreferrer" target="_blank">new_impl.bo</a>); > + result = anv_bo_cache_import_with_size(device, &device->bo_cache, > fd, > + 4096, &[3]<a href="http://new_impl.bo" rel="noreferrer" target="_blank">new_impl.bo</a>); > if (result != VK_SUCCESS) > return result; > > -- > 2.13.0 > > _______________________________________________ > mesa-dev mailing list > [4]<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > [5]<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > > References: > > [1] mailto:<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a> > [2] <a href="http://new_impl.bo/" rel="noreferrer" target="_blank">http://new_impl.bo/</a> > [3] <a href="http://new_impl.bo/" rel="noreferrer" target="_blank">http://new_impl.bo/</a> > [4] mailto:<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > [5] <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> <div class="HOEnZb"><div class="h5"> > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div>