[PATCH v4] DRM: add DRM Driver for Samsung SoC EXYNOS4210.

[robdclark@gmail.com (Rob Clark)]

On Wed, Sep 14, 2011 at 2:57 AM, Thomas Hellstrom <thomas@shipmail.org> wrote:
>>>>>>>>>
>>>>>>>>> +static struct drm_ioctl_desc samsung_ioctls[] = {
>>>>>>>>> + ? ? ? DRM_IOCTL_DEF_DRV(SAMSUNG_GEM_CREATE,
>>>>>>>>> samsung_drm_gem_create_ioctl,
>>>>>>>>> + ? ? ? ? ? ? ? ? ? ? ? DRM_UNLOCKED | DRM_AUTH),
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>> With reference my previous security comment.
>>>>>>>>
>>>>>>>> Let's say you have a compromised video player running as a DRM
>>>>>>>> client, that
>>>>>>>> tries to repeatedly allocate huge GEM buffers...
>>>>>>>>
>>>>>>>> What will happen when all DMA memory is exhausted? Will this cause
>>>>>>>> other
>>>>>>>> device drivers to see an OOM, or only DRM?
>>>>>>>>
>>>>>>>> The old DRI model basically allowed any authorized DRI client to
>>>>>>>> exhaust
>>>>>>>> video ram or AGP memory, but never system memory. Newer DRI drivers
>>>>>>>> typically only allow DRI masters to do that.
>>>>>>>> as
>>>>>>>>
>>>>>>>> I don't think an authorized DRI client should be able to easily
>>>>>>>>
>>>>>
>>>>> exhaust
>>>>>
>>>>>>>>
>>>>>>>> resources (DMA memory) used by other device drivers causing them to
>>>>>>>> fail.
>>>>>>>>
>>>>>>>
>>>>>>> I'm not entirely sure what else can be done, other than have a
>>>>>>> threshold on max MB allocatable of buffer memory..
>>>>>>>
>>>>>>
>>>>>> Yes, I think that's what needs to be done, and that threshold should
>>>>>> be low enough to keep other device drivers running in the worst
>>>>>> allocation case.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> In the samsung driver case, he is only allocating scanout memory
>>>>>>>
>>>
>>> from
>>>
>>>>>>>
>>>>>>> CMA, so the limit will be the CMA region size.. beyond that you
>>>>>>>
>>>
>>> can't
>>>
>>>>>>>
>>>>>>> get physically contiguous memory. ?So I think this driver is safe.
>>>>>>>
>>>>>>
>>>>>> It's not really what well-behaved user-space drivers do that should
>>>>>>
>>>
>>> be
>>>
>>>>>>
>>>>>> a concern, but what compromized application *might* do that is a
>>>>>>
>>>>
>>>> concern.
>>>>
>>>>>
>>>>> Hmm. I might have missed your point here. If the buffer allocation
>>>>>
>>>
>>> ioctl
>>>
>>>>>
>>>>> only allows allocating CMA memory, then I agree the driver fits the old
>>>>> DRI security model, as long as no other devices on the platform will
>>>>> ever use CMA.
>>>>>
>>>>> But in that case, there really should be a way for the driver to say
>>>>> "Hey, all CMA memory on this system is mine", in the same way
>>>>> traditional video drivers can claim the VRAM PCI resource.
>>>>>
>>>>>
>>>>
>>>> CMA could reserve memory region for a specific driver so DRM Client
>>>>
>>>
>>> could
>>>
>>>>
>>>> request memory allocation from only the region.
>>>>
>>>>
>>>>>
>>>>> This is to avoid the possibility that future drivers that need CMA will
>>>>> be vulnerable to DOS-attacks from ill-behaved DRI clients.
>>>>>
>>>>>
>>>>
>>>> Thomas, if any application has root authority for ill-purpose then isn't
>>>>
>>>
>>> it
>>>
>>>>
>>>> possible to be vulnerable to DOS-attacks? I think DRM_AUTH means root
>>>> authority. I know DRM Framework gives any root application DRM_AUTH
>>>> authority for compatibility.
>>>>
>>>
>>> DRM_AUTH just means that the client has authenticated w/ X11 (meaning
>>> that it has permission to connect to x server)..
>>>
>>>
>>
>> Yes, I understood so. but see drm_open_helper() of drm_fops.c file please.
>> in this function, you can see a line below.
>> /* for compatibility root is always authenticated */
>> priv->authenticated = capable(CAP_SYS_ADMIN)
>>
>> I think the code above says that any application with root permission is
>> authenticated.
>>
>>
>
> Yes, that is true. A root client may be assumed to have AUTH permissions,
> but the inverse does not hold, meaning that an AUTH client may *not* be
> assumed to have root permissions. I think there is a ROOT_ONLY ioctl flag
> for that.
>
> The problem I'm seeing compared to other drivers is the following:
>
> Imagine for example that you have a disc driver that allocates temporary
> memory out of the same DMA pool as the DRM driver.
>
> Now you have a video player that is a DRM client. It contains a security
> flaw and is compromized by somebody trying to play a specially crafted video
> stream. The video player starts to allocate gem buffers until it receives an
> -ENOMEM. Then it stops allocating and does nothing.
>
> Now the system tries an important disc access (paging for example). This
> fails, because the video player has exhausted all DMA memory and the disc
> driver fails to allocate.
>
> The system is dead.
>
> The point is:
>
> If there is a chance that other drivers will use the same DMA/CMA pool as
> the DRM driver, DRM must leave enough DMA/CMA memory for those drivers to
> work.

ah, ok, I get your point


> The difference compared to other drm drivers:
>
> There are other drm drivers that work the same way, with a static allocator.
> For example "via" and "sis". But those drivers completely claim the
> resources they are using. Nobody else is expected to use VRAM / AGP.
>
> In the Samsung case, it's not clear to me whether the DMA/CMA pool *can* be
> shared with other devices.
> If it is, IMHO you must implement an allocation limit in DRM, if not, the
> driver should probably be safe.

It is possible to create a device private CMA pool..  although OTOH it
might be desirable to let some other drivers (like v4l2) use buffers
from the same pool..

I'm not entirely sure what will happen w/ dma_alloc_coherent, etc, if
the global CMA pool is exhausted.

Marek?  I guess you know what would happen?

BR,
-R

> Thanks,
> Thomas