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

[Lucas De Marchi]

On Tue, Jun 27, 2023 at 09:24:13PM +0000, Paulo Zanoni wrote:
>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 chose (c) since the commit message itself already seems sufficient.

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

Applied, thanks

Lucas De Marchi

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