Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Thomas Hellstrom <thomas@shipmail.org>]

On 5/27/19 2:35 PM, Emil Velikov wrote:
> Hi Thomas,
>
> On 2019/05/27, Thomas Hellstrom wrote:
>
>>> I think we might be talking past each other, let's take a step back:
>>>
>>>   - as of previous patch, all of vmwgfx ioctls size is consistently
>>> handled by the core
>> I don't think I follow you here, AFAICT patch 3/5 only affects and
>> relaxes the execbuf checking (and in fact a little more than I would
>> like)?
>>
> Precisely, it makes execbuf ioctl behave like all other ioctls - both
> vmwgfx and rest of drm.

But we're still enforcing a non-relaxed size check for the other vmwgfx 
private ioctls, right? Which is relaxed, together with the directions, 
in this commit?

(Not that it matters much to the discussion, though).

>
>>>   - handling of featue flags, as always, is responsibility of the
>>> driver
>>> ifself
>>>   - with this patch, ioctl direction is also handled by core.
>>>
>>> Here core ensures we only copy in/out as much data as the kernel
>>> implementation can handle.
>>>
>>>
>>> Let's consider the following real world example - msm and virtio_gpu.
>>>
>>> An in field of an _IOW ioctl becomes in/out aka _IORW ioctl.
>>>   - we add a flag to annotate/request the out, as always invalid flags
>>> are return -EINVAL
>>>   - we change the ioctl encoding
>>>
>>> As currently handled by core DRM, old kernel/new userspace and
>>> vice-versa works just fine. Sadly, vmwgfx will error out, while it
>>> could
>>> be avoided.
>> IMO basically we have a tradeoff between strict checking in this case,
>> and new user-space vs old kernel "hazzle-free" transition in the
>> relaxed case.
>>
> Precisely. If I read the code correctly, ATM new userspace will fail
> against old kernels. Unless userspace writes two versions of the ioctl -
> with with each encoding.
>
>>> As said above, I'll gladly adjust core and/or others, if this relaxed
>>> approach causes an issue somewhere. A specific use-case, real or
>>> hypothetical will be appreciated.
>> To me there are two important reasons to keep the strict approach.
>>
>> 1) Avoid user-space mistakes early in the development cycle. We can't
>> distinguish between buggy user-space and "new" user-space. This is
>> important because of [a]) below.
>>
> Can you provide a concrete example, please?

OK, let's say you were developing fence wait functionality. Like 
vmw_fence_obj_wait ioctl. Then suddenly you started to wonder why the 
wait never timed out as it should. The reason turn out to be that 
signals were restarting the waits with the original timeout. So you 
change the ioctl from W to RW and add a kernel-computed time to the 
argument. Everything is fine, except that you forget to change this in a 
user-space application somewhere.

So now what happens, is that that user-space bug can live on undetected 
as in 1), and that means you can never go back and implement a stricter 
check because that would completely break old user-space.

The current code will trap (and has historically trapped) code like 
this. That's mainly why I'm reluctant to give it up, but I guess it can 
be conditionally compiled in for debug purposes.

>
>> 2) Catch a lot of fuzzer combinations and error out early instead of
>> forwarding them to the ioctl function where they may cause harm.
>>
> Struggling to see why this is a problem? At some point the fuzzer will
> get past this first line of defence, so we want to make the rest of the
> ioctl is robust.
>
>
>> I think the new user-space vs old kernel can be handled nicely in user-
>> space with feature flags or API versions. That's the way we've handled
>> them up to now?
>>
> How is a feature flag doing to help if the encoding changes from _IOW
> to _IORW?

Ah, you're referring to old user-space new kernel? Yes, I was probably 
reading a bit too fast. Sorry about that.

So we're basically landing in a tradeoff between trapping problems like 
above, and hazzle-free ioctl argument definition change.

OK, so I'm ok with that as long as there is a way we can compile in 
strict checking, which will likely has to be as a vmwgfx-specific wrapper.

/Thomas


>
>
> Thanks
> Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel