On Thu, 16 Oct 2025 12:02:10 +0200 Marcin Ślusarz <[email protected]> wrote:
> On Thu, Oct 16, 2025 at 11:52:24AM +0200, Boris Brezillon wrote: > > On Thu, 16 Oct 2025 10:42:21 +0200 > > Marcin Ślusarz <[email protected]> wrote: > > > > > On Wed, Oct 15, 2025 at 06:03:23PM +0200, Boris Brezillon wrote: > > > > +static int panfrost_ioctl_sync_bo(struct drm_device *ddev, void *data, > > > > + struct drm_file *file) > > > > +{ > > > > + struct drm_panfrost_sync_bo *args = data; > > > > + struct drm_panfrost_bo_sync_op *ops; > > > > + struct drm_gem_object *obj; > > > > + int ret; > > > > + u32 i; > > > > + > > > > + if (args->pad) > > > > + return -EINVAL; > > > > + > > > > + ops = kvmalloc_array(args->op_count, sizeof(*ops), GFP_KERNEL); > > > > + if (!ops) { > > > > + DRM_DEBUG("Failed to allocate incoming BO sync ops > > > > array\n"); > > > > + return -ENOMEM; > > > > + } > > > > + > > > > + if (copy_from_user(ops, (void __user *)(uintptr_t)args->ops, > > > > + args->op_count * sizeof(*ops))) { > > > > + DRM_DEBUG("Failed to copy in BO sync ops\n"); > > > > + ret = -EFAULT; > > > > + goto err_ops; > > > > + } > > > > + > > > > + for (i = 0; i < args->op_count; i++) { > > > > > > If args->op_count is 0, if I'm not mistaken, kvmalloc_array and > > > copy_to_user will succeed, but then this function will return > > > unitialized value. Maybe add an explicit check for op_count == 0 > > > at the beginning and avoid going through all that code? > > > > If args->op_count=0 the loop would be exited right away, so I'm not too > > sure where the problem is. > > Maybe I didn't explain it correctly, so let me clear that up: > ret is not initialized and not set anywhere if op_count is 0. Ah, right. I overlooked the fact ret wasn't initialized. Sorry for the noise. > > > This being said, I agree it's not worth > > going through kvmalloc_array() and copy_from_user() if we know there's > > nothing to do. And it's probably a bit fragile to rely on > > kvmalloc_array() not returning NULL when the size is zero (I actually > > thought it was), so I agree we'd rather bail out early in that case. > > > > > > > > > + if (ops[i].flags & ~PANFROST_BO_SYNC_OP_FLAGS) { > > > > + ret = -EINVAL; > > > > + goto err_ops; > > > > + } > > > > + > > > > + obj = drm_gem_object_lookup(file, ops[i].handle); > > > > + if (!obj) { > > > > + ret = -ENOENT; > > > > + goto err_ops; > > > > + } > > > > + > > > > + ret = panfrost_gem_sync(obj, ops[i].flags, > > > > + ops[i].offset, ops[i].size); > > > > + > > > > + drm_gem_object_put(obj); > > > > + > > > > + if (ret) > > > > + goto err_ops; > > > > + } > > > > + > > > > +err_ops: > > > > + kvfree(ops); > > > > + > > > > + return ret; > > > > +} > >
