drivers/gpu/drm/xe/xe_vm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
The error handling assumes that vm_bind_ioctl_check_args() will
initialize "bind_ops" but there are a couple early returns where that's
not true. Initialize "bind_ops" to NULL from the start.
Fixes: b43e864af0d4 ("drm/xe/uapi: Add DRM_XE_VM_BIND_FLAG_CPU_ADDR_MIRROR")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
drivers/gpu/drm/xe/xe_vm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 22a26aff3a6e..d85759b958d0 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -3287,7 +3287,7 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
struct xe_exec_queue *q = NULL;
u32 num_syncs, num_ufence = 0;
struct xe_sync_entry *syncs = NULL;
- struct drm_xe_vm_bind_op *bind_ops;
+ struct drm_xe_vm_bind_op *bind_ops = NULL;
struct xe_vma_ops vops;
struct dma_fence *fence;
int err;
--
2.47.2On Mon, Mar 10, 2025 at 01:48:00PM +0300, Dan Carpenter wrote:
> The error handling assumes that vm_bind_ioctl_check_args() will
> initialize "bind_ops" but there are a couple early returns where that's
> not true. Initialize "bind_ops" to NULL from the start.
It is not a couple, but only the one goto put_vm where this bind_ops
gets actually initialized, or not...
but perhaps the order in the exit is wrong and we should move the
kvfree(bind_ops) upper to the end of put_exec_queue?
Matt, thoughts on the order here?
Cc: Matthew Brost <matthew.brost@intel.com>
>
> Fixes: b43e864af0d4 ("drm/xe/uapi: Add DRM_XE_VM_BIND_FLAG_CPU_ADDR_MIRROR")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> drivers/gpu/drm/xe/xe_vm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 22a26aff3a6e..d85759b958d0 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -3287,7 +3287,7 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> struct xe_exec_queue *q = NULL;
> u32 num_syncs, num_ufence = 0;
> struct xe_sync_entry *syncs = NULL;
> - struct drm_xe_vm_bind_op *bind_ops;
> + struct drm_xe_vm_bind_op *bind_ops = NULL;
> struct xe_vma_ops vops;
> struct dma_fence *fence;
> int err;
> --
> 2.47.2
>
On Mon, Mar 10, 2025 at 12:56:46PM -0400, Rodrigo Vivi wrote:
> On Mon, Mar 10, 2025 at 01:48:00PM +0300, Dan Carpenter wrote:
> > The error handling assumes that vm_bind_ioctl_check_args() will
> > initialize "bind_ops" but there are a couple early returns where that's
> > not true. Initialize "bind_ops" to NULL from the start.
>
> It is not a couple, but only the one goto put_vm where this bind_ops
> gets actually initialized, or not...
>
I'm on linux-next. I'm not seeing the goto put_vm... I think we're
looking at different code.
3063 static int vm_bind_ioctl_check_args(struct xe_device *xe, struct xe_vm *vm,
3064 struct drm_xe_vm_bind *args,
3065 struct drm_xe_vm_bind_op **bind_ops)
3066 {
3067 int err;
3068 int i;
3069
3070 if (XE_IOCTL_DBG(xe, args->pad || args->pad2) ||
3071 XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
3072 return -EINVAL;
One.
3073
3074 if (XE_IOCTL_DBG(xe, args->extensions))
3075 return -EINVAL;
Two.
3076
3077 if (args->num_binds > 1) {
3078 u64 __user *bind_user =
3079 u64_to_user_ptr(args->vector_of_binds);
3080
3081 *bind_ops = kvmalloc_array(args->num_binds,
Initialized.
3082 sizeof(struct drm_xe_vm_bind_op),
3083 GFP_KERNEL | __GFP_ACCOUNT |
3084 __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
3085 if (!*bind_ops)
3086 return args->num_binds > 1 ? -ENOBUFS : -ENOMEM;
3087
3088 err = __copy_from_user(*bind_ops, bind_user,
3089 sizeof(struct drm_xe_vm_bind_op) *
3090 args->num_binds);
3091 if (XE_IOCTL_DBG(xe, err)) {
3092 err = -EFAULT;
3093 goto free_bind_ops;
3094 }
3095 } else {
3096 *bind_ops = &args->bind;
3097 }
> but perhaps the order in the exit is wrong and we should move the
> kvfree(bind_ops) upper to the end of put_exec_queue?
>
> Matt, thoughts on the order here?
>
> Cc: Matthew Brost <matthew.brost@intel.com>
I feel like ideally vm_bind_ioctl_check_args() would clean up after
itself on failure and, yes, it should be in reverse order from how
it was allocated.
regards,
dan carpenter
On Mon, Mar 10, 2025 at 09:22:50PM +0300, Dan Carpenter wrote:
> On Mon, Mar 10, 2025 at 12:56:46PM -0400, Rodrigo Vivi wrote:
> > On Mon, Mar 10, 2025 at 01:48:00PM +0300, Dan Carpenter wrote:
> > > The error handling assumes that vm_bind_ioctl_check_args() will
> > > initialize "bind_ops" but there are a couple early returns where that's
> > > not true. Initialize "bind_ops" to NULL from the start.
> >
> > It is not a couple, but only the one goto put_vm where this bind_ops
> > gets actually initialized, or not...
> >
>
> I'm on linux-next. I'm not seeing the goto put_vm... I think we're
> looking at different code.
>
> 3063 static int vm_bind_ioctl_check_args(struct xe_device *xe, struct xe_vm *vm,
> 3064 struct drm_xe_vm_bind *args,
> 3065 struct drm_xe_vm_bind_op **bind_ops)
> 3066 {
> 3067 int err;
> 3068 int i;
> 3069
> 3070 if (XE_IOCTL_DBG(xe, args->pad || args->pad2) ||
> 3071 XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> 3072 return -EINVAL;
>
> One.
>
> 3073
> 3074 if (XE_IOCTL_DBG(xe, args->extensions))
> 3075 return -EINVAL;
>
> Two.
>
> 3076
> 3077 if (args->num_binds > 1) {
> 3078 u64 __user *bind_user =
> 3079 u64_to_user_ptr(args->vector_of_binds);
> 3080
> 3081 *bind_ops = kvmalloc_array(args->num_binds,
>
> Initialized.
>
> 3082 sizeof(struct drm_xe_vm_bind_op),
> 3083 GFP_KERNEL | __GFP_ACCOUNT |
> 3084 __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
> 3085 if (!*bind_ops)
> 3086 return args->num_binds > 1 ? -ENOBUFS : -ENOMEM;
> 3087
> 3088 err = __copy_from_user(*bind_ops, bind_user,
> 3089 sizeof(struct drm_xe_vm_bind_op) *
> 3090 args->num_binds);
> 3091 if (XE_IOCTL_DBG(xe, err)) {
> 3092 err = -EFAULT;
> 3093 goto free_bind_ops;
> 3094 }
> 3095 } else {
> 3096 *bind_ops = &args->bind;
> 3097 }
>
> > but perhaps the order in the exit is wrong and we should move the
> > kvfree(bind_ops) upper to the end of put_exec_queue?
> >
> > Matt, thoughts on the order here?
> >
Rodrigo – I think you are looking in the wrong spot in the code. Dan's
subsequent reply, I believe, explains the issue correctly, so I think
the patch is good.
> > Cc: Matthew Brost <matthew.brost@intel.com>
>
> I feel like ideally vm_bind_ioctl_check_args() would clean up after
> itself on failure and, yes, it should be in reverse order from how
> it was allocated.
>
This is a bit of goofy error handling/convention—perhaps it could be
cleaned up in a follow-up.
That said, this patch is correct. However, the 'Fixes' tag looks
wrong—it has been broken for a bit longer than the tagged patch. We can
fix it upon merge.
With that:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> regards,
> dan carpenter
>
On Mon, Mar 10, 2025 at 10:04:22PM -0700, Matthew Brost wrote:
> On Mon, Mar 10, 2025 at 09:22:50PM +0300, Dan Carpenter wrote:
> > On Mon, Mar 10, 2025 at 12:56:46PM -0400, Rodrigo Vivi wrote:
> > > On Mon, Mar 10, 2025 at 01:48:00PM +0300, Dan Carpenter wrote:
> > > > The error handling assumes that vm_bind_ioctl_check_args() will
> > > > initialize "bind_ops" but there are a couple early returns where that's
> > > > not true. Initialize "bind_ops" to NULL from the start.
> > >
> > > It is not a couple, but only the one goto put_vm where this bind_ops
> > > gets actually initialized, or not...
> > >
> >
> > I'm on linux-next. I'm not seeing the goto put_vm... I think we're
> > looking at different code.
> >
> > 3063 static int vm_bind_ioctl_check_args(struct xe_device *xe, struct xe_vm *vm,
> > 3064 struct drm_xe_vm_bind *args,
> > 3065 struct drm_xe_vm_bind_op **bind_ops)
> > 3066 {
> > 3067 int err;
> > 3068 int i;
> > 3069
> > 3070 if (XE_IOCTL_DBG(xe, args->pad || args->pad2) ||
> > 3071 XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> > 3072 return -EINVAL;
> >
> > One.
> >
> > 3073
> > 3074 if (XE_IOCTL_DBG(xe, args->extensions))
> > 3075 return -EINVAL;
> >
> > Two.
> >
> > 3076
> > 3077 if (args->num_binds > 1) {
> > 3078 u64 __user *bind_user =
> > 3079 u64_to_user_ptr(args->vector_of_binds);
> > 3080
> > 3081 *bind_ops = kvmalloc_array(args->num_binds,
> >
> > Initialized.
> >
> > 3082 sizeof(struct drm_xe_vm_bind_op),
> > 3083 GFP_KERNEL | __GFP_ACCOUNT |
> > 3084 __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
> > 3085 if (!*bind_ops)
> > 3086 return args->num_binds > 1 ? -ENOBUFS : -ENOMEM;
> > 3087
> > 3088 err = __copy_from_user(*bind_ops, bind_user,
> > 3089 sizeof(struct drm_xe_vm_bind_op) *
> > 3090 args->num_binds);
> > 3091 if (XE_IOCTL_DBG(xe, err)) {
> > 3092 err = -EFAULT;
> > 3093 goto free_bind_ops;
> > 3094 }
> > 3095 } else {
> > 3096 *bind_ops = &args->bind;
> > 3097 }
> >
> > > but perhaps the order in the exit is wrong and we should move the
> > > kvfree(bind_ops) upper to the end of put_exec_queue?
> > >
> > > Matt, thoughts on the order here?
> > >
>
> Rodrigo – I think you are looking in the wrong spot in the code. Dan's
> subsequent reply, I believe, explains the issue correctly, so I think
> the patch is good.
>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> >
> > I feel like ideally vm_bind_ioctl_check_args() would clean up after
> > itself on failure and, yes, it should be in reverse order from how
> > it was allocated.
> >
>
> This is a bit of goofy error handling/convention—perhaps it could be
> cleaned up in a follow-up.
>
> That said, this patch is correct. However, the 'Fixes' tag looks
> wrong—it has been broken for a bit longer than the tagged patch. We can
> fix it upon merge.
>
> With that:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>
Actually, we have another problem too. The 'free_bind_ops' label in
vm_bind_ioctl_check_args() either isn't needed or it should *bind_ops to
NULL after kvfree to avoid a double free in put_vm label in
xe_vm_bind_ioctl().
This patch is still valid though.
Matt
> > regards,
> > dan carpenter
> >
On Mon, Mar 10, 2025 at 10:12:15PM -0700, Matthew Brost wrote:
> On Mon, Mar 10, 2025 at 10:04:22PM -0700, Matthew Brost wrote:
> > On Mon, Mar 10, 2025 at 09:22:50PM +0300, Dan Carpenter wrote:
> > > On Mon, Mar 10, 2025 at 12:56:46PM -0400, Rodrigo Vivi wrote:
> > > > On Mon, Mar 10, 2025 at 01:48:00PM +0300, Dan Carpenter wrote:
> > > > > The error handling assumes that vm_bind_ioctl_check_args() will
> > > > > initialize "bind_ops" but there are a couple early returns where that's
> > > > > not true. Initialize "bind_ops" to NULL from the start.
> > > >
> > > > It is not a couple, but only the one goto put_vm where this bind_ops
> > > > gets actually initialized, or not...
> > > >
> > >
> > > I'm on linux-next. I'm not seeing the goto put_vm... I think we're
> > > looking at different code.
> > >
> > > 3063 static int vm_bind_ioctl_check_args(struct xe_device *xe, struct xe_vm *vm,
> > > 3064 struct drm_xe_vm_bind *args,
> > > 3065 struct drm_xe_vm_bind_op **bind_ops)
> > > 3066 {
> > > 3067 int err;
> > > 3068 int i;
> > > 3069
> > > 3070 if (XE_IOCTL_DBG(xe, args->pad || args->pad2) ||
> > > 3071 XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> > > 3072 return -EINVAL;
> > >
> > > One.
> > >
> > > 3073
> > > 3074 if (XE_IOCTL_DBG(xe, args->extensions))
> > > 3075 return -EINVAL;
> > >
> > > Two.
> > >
> > > 3076
> > > 3077 if (args->num_binds > 1) {
> > > 3078 u64 __user *bind_user =
> > > 3079 u64_to_user_ptr(args->vector_of_binds);
> > > 3080
> > > 3081 *bind_ops = kvmalloc_array(args->num_binds,
> > >
> > > Initialized.
> > >
> > > 3082 sizeof(struct drm_xe_vm_bind_op),
> > > 3083 GFP_KERNEL | __GFP_ACCOUNT |
> > > 3084 __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
> > > 3085 if (!*bind_ops)
> > > 3086 return args->num_binds > 1 ? -ENOBUFS : -ENOMEM;
> > > 3087
> > > 3088 err = __copy_from_user(*bind_ops, bind_user,
> > > 3089 sizeof(struct drm_xe_vm_bind_op) *
> > > 3090 args->num_binds);
> > > 3091 if (XE_IOCTL_DBG(xe, err)) {
> > > 3092 err = -EFAULT;
> > > 3093 goto free_bind_ops;
> > > 3094 }
> > > 3095 } else {
> > > 3096 *bind_ops = &args->bind;
> > > 3097 }
> > >
> > > > but perhaps the order in the exit is wrong and we should move the
> > > > kvfree(bind_ops) upper to the end of put_exec_queue?
> > > >
> > > > Matt, thoughts on the order here?
> > > >
> >
> > Rodrigo – I think you are looking in the wrong spot in the code. Dan's
> > subsequent reply, I believe, explains the issue correctly, so I think
> > the patch is good.
> >
> > > > Cc: Matthew Brost <matthew.brost@intel.com>
> > >
> > > I feel like ideally vm_bind_ioctl_check_args() would clean up after
> > > itself on failure and, yes, it should be in reverse order from how
> > > it was allocated.
> > >
> >
> > This is a bit of goofy error handling/convention—perhaps it could be
> > cleaned up in a follow-up.
> >
> > That said, this patch is correct. However, the 'Fixes' tag looks
> > wrong—it has been broken for a bit longer than the tagged patch. We can
> > fix it upon merge.
> >
Cough as I eat my hat - the fixes tag in correct - the patch tagged
moved the args check after the VM lookup which created a bug.
> > With that:
> > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> >
>
> Actually, we have another problem too. The 'free_bind_ops' label in
> vm_bind_ioctl_check_args() either isn't needed or it should *bind_ops to
> NULL after kvfree to avoid a double free in put_vm label in
> xe_vm_bind_ioctl().
>
> This patch is still valid though.
>
Posted a follow up include Dan's original change and also my suggested
change above.
Matt
> Matt
>
> > > regards,
> > > dan carpenter
> > >
© 2016 - 2026 Red Hat, Inc.