Re: [PATCH 3/5] drm/vmwgfx: use core drm to extend/check vmw_execbuf_ioctl

[Thomas Hellstrom <thomas@shipmail.org>]

On 5/24/19 12:53 PM, Emil Velikov wrote:
> On 2019/05/24, Daniel Vetter wrote:
>> On Fri, May 24, 2019 at 8:05 AM Thomas Hellstrom <thellstrom@vmware.com> wrote:
>>> On Wed, 2019-05-22 at 21:09 +0200, Daniel Vetter wrote:
>>>> On Wed, May 22, 2019 at 9:01 PM Thomas Hellstrom <
>>>> thellstrom@vmware.com> wrote:
>>>>> Hi, Emil,
>>>>>
>>>>> On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
>>>>>> From: Emil Velikov <emil.velikov@collabora.com>
>>>>>>
>>>>>> Currently vmw_execbuf_ioctl() open-codes the permission checking,
>>>>>> size
>>>>>> extending and copying that is already done in core drm.
>>>>>>
>>>>>> Kill all the duplication, adding a few comments for clarity.
>>>>> Ah, there is core functionality for this now.
>>>>>
>>>>> What worries me though with the core approach is that the sizes are
>>>>> not
>>>>> capped by the size of the kernel argument definition, which makes
>>>>> mailicious user-space being able to force kmallocs() the size of
>>>>> the
>>>>> maximum ioctl size. Should probably be fixed before pushing this.
>>>> Hm I always worked under the assumption that kmalloc and friends
>>>> should be userspace hardened. Otherwise stuff like kmalloc_array
>>>> doesn't make any sense, everyone just feeds it unchecked input and
>>>> expects that helper to handle overflows.
>>>>
>>>> If we assume kmalloc isn't hardened against that, then we have a much
>>>> bigger problem than just vmwgfx ioctls ...
>>> After checking the drm_ioctl code I realize that what I thought was new
>>> behaviour actually has been around for a couple of years, so
>>> fixing isn't really tied to this patch series...
>>>
>>> What caused me to react was that previously we used to have this
>>>
>>> e4fda9f264e1 ("drm: Perform ioctl command validation on the stored
>>> kernel values")
>>>
>>> and we seem to have lost that now, if not for the io flags then at
>>> least for the size part. For the size of the ioctl arguments, I think
>>> in general if the kernel only touches a subset of the user-space
>>> specified size I see no reason why we should malloc / copy more than
>>> that?
>> I guess we could optimize that, but we'd probably still need to zero
>> clear the added size for forward compat with newer userspace. Iirc
>> we've had some issues in this area.
>>
>>> Now, given the fact that the maximum ioctl argument size is quite
>>> limited, that might not be a big problem or a problem at all. Otherwise
>>> it would be pretty easy for a malicious process to allocate most or all
>>> of a system's resident memory?
>> The biggest you can allocate from kmalloc is limited by the largest
>> contiguous chunk alloc_pages gives you, which is limited by MAX_ORDER
>> from the page buddy allocator. You need lots of process to be able to
>> exhaust memory like that (and like I said, the entire kernel would be
>> broken if we'd consider this a security issue). If you want to make
>> sure that a process group can't exhaust memory this way then you need
>> to set appropriate cgroups limits.
> I do agree with all the sentiments that drm_ioctl() could use some extra
> optimisation and hardening. At the same time I would remind that the
> code has been used as-is by vmwgfx and other drivers for years.
>
> In other words: let's keep that work as orthogonal series.
>
> What do you guys think?

I agree. Then I only had a concern with one of the patches.

/Thomas


> 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