Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl datafor a NULL pointer

[Tommi Rantala]

2013/3/17 Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>:
> On Mon, Mar 18, 2013 at 07:42:58AM +1000, Dave Airlie wrote:
>> On Mon, Mar 18, 2013 at 7:40 AM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
>> > On Sun, Mar 17, 2013 at 08:50:03PM +0100, Daniel Vetter wrote:
>> >> On Sat, Mar 16, 2013 at 11:19 AM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
>> >> > If *userspace* doesn't request either IOC_IN | IOC_OUT in their ioctl
>> >> > command (which are seperate from the ioctl number), then kdata is set to
>> >> > NULL.
>> >>
>> >> Doesn't that mean that we need these checks everywhere? Or at least a
>> >> fixup in drm core proper?
>> >
>> > That's my conclusion. We either add a flag to ask drm_ioctl to prevent
>> > passing NULL pointers (as the existing behaviour may be useful
>> > somewhere, and I have not checked all callees) or saturate our callbacks
>> > with NULL checks.
>>
>> Do we have the kernel's expected IOC_IN/IOC_OUT flags at that point as well?
>>
>> we could check them and block NULL in that case.
>
> Yes. For the core ioctls, we use drm_ioctls[nr].cmd rather than the
> value passed in by userspace for the IOC_IN|IN_OUT bits. So:

Thanks, trinity can indeed set the in/out bits randomly, in a way that
does not match the driver ioctl definition.

Your patch almost fixes this. For the driver ioctls we will want to
grab the cmd from cmd_drv. So the patch should be:

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 25f91cd..5210f33 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -408,6 +408,7 @@ long drm_ioctl(struct file *filp,
                usize = asize = _IOC_SIZE(cmd);
                if (drv_size > asize)
                        asize = drv_size;
+               cmd = ioctl->cmd_drv;
        }
        else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
                ioctl = &drm_ioctls[nr];

Can you please submit this officially?

> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 25f91cd..79b8bd1 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -408,6 +408,7 @@ long drm_ioctl(struct file *filp,
>                 usize = asize = _IOC_SIZE(cmd);
>                 if (drv_size > asize)
>                         asize = drv_size;
> +               cmd = ioctl->cmd;
>         }
>         else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
>                 ioctl = &drm_ioctls[nr];
>
>
> --
> Chris Wilson, Intel Open Source Technology Centre
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/