[Intel-xe] [PATCH 1/2 v2] drm/xe: fix bounds checking for 'len' in xe_engine_create_ioctl

[Zanoni, Paulo R]

On Tue, 2023-06-27 at 16:10 -0300, Lucas De Marchi wrote:
> On Mon, Jun 26, 2023 at 02:22:20PM -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.
> > 
> > Switch 'len' to u32. It is the result of the multiplication of two u16
> > numbers, so it won't be able to overflow back into smaller numbers as
> > an u32.
> > 
> > Very simple reproducer:
> >    https://people.freedesktop.org/~pzanoni/engine-create-bug/
> 
> since this will be forever in the git log, maybe instead of a link that
> may vanish just add something like this in the commit message itself?

Well, my git log references to people.freedesktop.org have lasted
longer than my git log references to bugzilla.freedesktop.org :)

To be fair the bugs are so simple that we could also just remove the
links, so I'm fine with any of the 3 possible approaches: (a) keep it
as-is; (b) paste code as you suggested or (c) completely remove the
mention to the reproducer.

I don't think I have permission to merge this, so whoever gets to merge
this can choose the appropriate way to mend the commit messages. Or do
I need to resend the series to the list?

Thanks,
Paulo

> 
> 	fd_ = ...;
> 	vm_id_ = ...;
> 	instance = ...;
> 	struct drm_xe_engine_create engine_create = {
> 		.extensions = (uintptr_t) nullptr,
> 		.width = 65535,
> 		.num_placements = 32769,
> 		.vm_id = vm_id_,
> 		.flags = 0,
> 		.engine_id = 0,
> 		.instances = (uintptr_t) &instance,
> 	};
> 	rc = drmIoctl(fd_, DRM_IOCTL_XE_ENGINE_CREATE, &engine_create);
> 	assert(rc == 0);
> 
> > 
> > v2: Make len u32 instead of checking for <=0 (José).
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> 
> 
> Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
> 
> Lucas De Marchi
> 
> > ---
> > 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..530f55a33b03 100644
> > --- a/drivers/gpu/drm/xe/xe_engine.c
> > +++ b/drivers/gpu/drm/xe/xe_engine.c
> > @@ -522,7 +522,7 @@ int xe_engine_create_ioctl(struct drm_device *dev, void *data,
> > 	struct xe_engine *e = NULL;
> > 	u32 logical_mask;
> > 	u32 id;
> > -	int len;
> > +	u32 len;
> > 	int err;
> > 
> > 	if (XE_IOCTL_ERR(xe, args->flags) ||
> > -- 
> > 2.39.2
> >