[Intel-xe] [PATCH 1/2] drm/xe: check for negative 'len' in xe_engine_create_ioctl

[Zanoni, Paulo R]

On Thu, 2023-06-22 at 22:54 +0000, Souza, Jose wrote:
> On Thu, 2023-06-22 at 15:42 -0700, Paulo Zanoni wrote:
> > There's this shared machine running xe.ko and I often log in to see my
> > tmux corrupted by messages such as:
> > 
> >     usercopy: Kernel memory overwrite attempt detected to wrapped address (offset 0, size 18446660151965198754)!
> > 
> > I also sometimes see:
> > 
> >     kernel BUG at mm/usercopy.c:102!
> > 
> > Someone is running a program that's definitely submitting random
> > numbers to this ioctl. If you pass width=65535 and
> > num_placements=32769 then you get a negative 'len', which avoids the
> > EINVAL check, leading to the bug.
> > 
> > Very simple reproducer:
> >     https://people.freedesktop.org/~pzanoni/engine-create-bug/
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_engine.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_engine.c b/drivers/gpu/drm/xe/xe_engine.c
> > index 6e6b2913f766..3d35d9f94894 100644
> > --- a/drivers/gpu/drm/xe/xe_engine.c
> > +++ b/drivers/gpu/drm/xe/xe_engine.c
> > @@ -530,7 +530,7 @@ int xe_engine_create_ioctl(struct drm_device *dev, void *data,
> >  		return -EINVAL;
> >  
> >  	len = args->width * args->num_placements;
> > -	if (XE_IOCTL_ERR(xe, !len || len > XE_HW_ENGINE_MAX_INSTANCE))
> > +	if (XE_IOCTL_ERR(xe, len <= 0 || len > XE_HW_ENGINE_MAX_INSTANCE))
> >  		return -EINVAL;
> 
> What about make len unsigned? The user takes a unsigned long...

I knew someone was going to propose this. It is another way to fix the
issue. I can do that if it's more desirable. I had to make a guess on
which approach I would be more easily acceptable and I suppose I
guessed wrong :).

Speaking of alternative approaches, another thing I considered was to
also separately check width and num_placements, in case there existed
some values for them where both are >XE_HW_ENGINE_MAX_INSTANCE but the
multiplication resulted in overflow, leaving 'len' with a valid value.
But as far as I checked this is not valid since they're uint16_t and
len is int.


> 
> >  
> >  	err = __copy_from_user(eci, user_eci,
>