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

[Thomas Hellstrom]

On 5/27/19 5:27 PM, Emil Velikov wrote:
> On 2019/05/27, Thomas Hellstrom wrote:
>> 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?
>>
> Regardless of the patch, all !execbuf vmwgfx ioctls use the related size
> checking from core drm.

Well it does, but since we (before this patch) enforce ioctl->cmd == 
cmd, we also enforce
_IOC_SIZE(ioctl->cmd) == _IOC_SIZE(cmd), which makes the core check 
pointless, or am I missing something?

>
>> (Not that it matters much to the discussion, though).
>>
> Agreed.
>
>>>
...
>>> 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.
>>
> If I understand you correctly, the W -> RW change in unnecessary. Yet
> the only negative effect that I can see is the copy_to_user() overhead.
>
> The copy should be negligible, yet it "feels" silly.
>
> Is there anything more serious that I've missed?

Well the point is in this case, that the write was necessary, but the 
code would work sort of OK anyway. It updated a kernel "cookie" to make 
sure the timeout would be correct even with the next call repetition. 
Now if an old header was floating around, there might be clients using 
it. And with the current core checks that typically wouldn't get 
noticed. With the check we'd immediately notice and abort. It feels a 
little like moving from ANSI C to K&R... :-)

>
>
> Having a closer look - vmwgfx (et al) seems to stand out, such that it
> does not provide a UABI define including the encoding. Hence it sort of
> duplicates that in userspace, by using the explicit drmCommand*
>
> Guess I could follow the suggestion in vmwgfx_drv.c move the defines to
> UABI, sync header and update mesa/xf86-video-vmwgfx.
>
> What do you think - yes, or please don't?

Please hold on for a while, and I'll discuss it internally.

>
>> 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.
>>
> This piece here, is the holly grail. I'll go further and suggest:
>
>   - add a strict encoding and size check, behind a config toggle
>   - make it a core drm thing and drop the custom vmwgfx one
>
> Will keep it disabled by default - but will clearly document Kconfig and
> docs that devs should toggle it to catch bugs.

Sounds good, but IIRC the reason why I kept it only to driver-private 
ioctls, is that there were errors with the drm ioctls. But that was a 
long time ago so I might remember incorrectly, or user-space has been fixed.

>
>>>> 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.
>>
> Ack, I'll proceed with the debug toggle suggestion.

Great.


>
> Thank you for the insightful input.
> Emil

Thanks,

Thomas