[PATCH next] drm/xe: Fix uninitialized variable in xe_vm_bind_ioctl()

[PATCH next] drm/xe: Fix uninitialized variable in xe_vm_bind_ioctl()

[Dan Carpenter]

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.2

Re: [PATCH next] drm/xe: Fix uninitialized variable in xe_vm_bind_ioctl()

[Rodrigo Vivi]

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...

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
> 

Re: [PATCH next] drm/xe: Fix uninitialized variable in xe_vm_bind_ioctl()

[Dan Carpenter]

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

Re: [PATCH next] drm/xe: Fix uninitialized variable in xe_vm_bind_ioctl()

[Matthew Brost]

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
> 

Re: [PATCH next] drm/xe: Fix uninitialized variable in xe_vm_bind_ioctl()

[Matthew Brost]

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
> > 

Re: [PATCH next] drm/xe: Fix uninitialized variable in xe_vm_bind_ioctl()

[Matthew Brost]

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
> > >