<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Apr 3, 2017 at 4:26 PM, 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 Wed 15 Mar 2017, Jason Ekstrand wrote: > This cache allows us to easily ensure that we have a unique anv_bo for > each gem handle. We'll need this in order to support multiple-import of > memory objects and semaphores. > > v2 (Jason Ekstrand): > - Reject BO imports if the size doesn't match the prime fd size as > reported by lseek(). > > v3 (Jason Ekstrand): > - Fix reference counting around cache_release (Chris Willson) > - Move the mutex_unlock() later in cache_release > --- > src/intel/vulkan/anv_allocator.c | 261 +++++++++++++++++++++++++++++++++++++++ > src/intel/vulkan/anv_private.h | 26 ++++ > 2 files changed, 287 insertions(+) > +VkResult > +anv_bo_cache_import(struct anv_device *device, > + struct anv_bo_cache *cache, > + int fd, uint64_t size, struct anv_bo **bo_out, > + VkAllocationCallbacks *alloc) > +{ > + 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); > + return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX); > + } > + > + struct anv_cached_bo *bo = anv_bo_cache_lookup_locked(cache, gem_handle); > + if (bo) { > + assert(bo->bo.size == size); For security's sake, we must always check the size, even if the bo is in the cache. An assertion isn't enough. A malicious client may tell the truth the first time when sending the fd to the trusted app. The malicious client may then dup the fd and send it to the trusted app with a false size. </blockquote><div> </div><div>Yup. Fixed locally. </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> > + __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_KHX); > + } > + > + struct anv_cached_bo *bo = > + vk_alloc(alloc, size, 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); > + } > + > + pthread_mutex_unlock(&cache->mutex); > + > + /* From the Vulkan spec: > + * > + * "Importing memory from a file descriptor transfers ownership of > + * the file descriptor from the application to the Vulkan > + * implementation. The application must not perform any operations on > + * the file descriptor after a successful import." > + * > + * If the import fails, we leave the file descriptor open. > + */ > + close(fd); > + > + *bo_out = &bo->bo; > + > + return VK_SUCCESS; > +} </div></div></blockquote></div> </div></div>