<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? > > > + 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"></div></div></blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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>