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

[thomas@shipmail.org (Thomas Hellstrom)]

On 09/15/2011 03:20 AM, Inki Dae wrote:
> Hi, Thomas.
>
>    
>> -----Original Message-----
>> From: Thomas Hellstrom [mailto:thomas at shipmail.org]
>> Sent: Wednesday, September 14, 2011 4:57 PM
>> To: Inki Dae
>> Cc: 'Rob Clark'; kyungmin.park at samsung.com; sw0312.kim at samsung.com; linux-
>> arm-kernel at lists.infradead.org; dri-devel at lists.freedesktop.org
>> Subject: Re: [PATCH v4] DRM: add DRM Driver for Samsung SoC EXYNOS4210.
>>
>> On 09/14/2011 07:55 AM, Inki Dae wrote:
>>      
>>>
>>>        
>>>> -----Original Message-----
>>>> From: Rob Clark [mailto:robdclark at gmail.com]
>>>> Sent: Wednesday, September 14, 2011 11:26 AM
>>>> To: Inki Dae
>>>> Cc: Thomas Hellstrom; kyungmin.park at samsung.com;
>>>>          
> sw0312.kim at samsung.com;
>    
>>>> linux-arm-kernel at lists.infradead.org; dri-devel at lists.freedesktop.org
>>>> Subject: Re: [PATCH v4] DRM: add DRM Driver for Samsung SoC EXYNOS4210.
>>>>
>>>> On Tue, Sep 13, 2011 at 9:03 PM, Inki Dae<inki.dae@samsung.com>   wrote:
>>>>
>>>>          
>>>>> Hi Thomas.
>>>>>
>>>>>
>>>>>            
>>>>>> -----Original Message-----
>>>>>> From: Thomas Hellstrom [mailto:thomas at shipmail.org]
>>>>>> Sent: Monday, September 12, 2011 3:32 PM
>>>>>> To: Rob Clark
>>>>>> Cc: Inki Dae; kyungmin.park at samsung.com; sw0312.kim at samsung.com;
>>>>>>              
>> linux-
>>      
>>>>>> arm-kernel at lists.infradead.org; dri-devel at lists.freedesktop.org
>>>>>> Subject: Re: [PATCH v4] DRM: add DRM Driver for Samsung SoC
>>>>>>              
>> EXYNOS4210.
>>      
>>>>>> On 09/11/2011 11:26 PM, Thomas Hellstrom wrote:
>>>>>>
>>>>>>              
>>>>>>> On 09/10/2011 07:31 PM, Rob Clark wrote:
>>>>>>>
>>>>>>>                
>>>>>>>> On Sat, Sep 10, 2011 at 9:04 AM, Thomas
>>>>>>>> Hellstrom<thomas@shipmail.org>     wrote:
>>>>>>>>
>>>>>>>>                  
>>>>>>>>> On 09/09/2011 01:38 PM, Inki Dae wrote:
>>>>>>>>>
>>>>>>>>>                    
>>>>>>>>>> This patch is a DRM Driver for Samsung SoC Exynos4210 and now
>>>>>>>>>>
>>>>>>>>>>                      
>>>> enables
>>>>
>>>>          
>>>>>>>>>> only FIMD yet but we will add HDMI support also in the future.
>>>>>>>>>>
>>>>>>>>>> from now on, I will remove RFC prefix because I think we have got
>>>>>>>>>> comments
>>>>>>>>>> enough.
>>>>>>>>>>
>>>>>>>>>> this patch is based on git repository below:
>>>>>>>>>>
>>>>>>>>>>                      
> git://git.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git,
>    
>>>>>>>>>> branch name: drm-next
>>>>>>>>>> commit-id: bcc65fd8e929a9d9d34d814d6efc1d2793546922
>>>>>>>>>>
>>>>>>>>>> you can refer to our working repository below:
>>>>>>>>>> http://git.infradead.org/users/kmpark/linux-2.6-samsung
>>>>>>>>>> branch name: samsung-drm
>>>>>>>>>>
>>>>>>>>>> We tried to re-use lowlevel codes of the FIMD driver(s3c-fb.c
>>>>>>>>>> based on Linux framebuffer) but couldn't so because lowlevel
>>>>>>>>>>                      
>> codes
>>      
>>>>>>>>>> of s3c-fb.c are included internally and so FIMD module of this
>>>>>>>>>> driver has
>>>>>>>>>> its own lowlevel codes.
>>>>>>>>>>
>>>>>>>>>> We used GEM framework for buffer management and DMA
>>>>>>>>>>
>>>>>>>>>>                      
>>>> APIs(dma_alloc_*)
>>>>
>>>>          
>>>>>>>>>> for buffer allocation. by using DMA API, we could use CMA later.
>>>>>>>>>>
>>>>>>>>>> Refer to this link for CMA(Continuous Memory Allocator):
>>>>>>>>>> http://lkml.org/lkml/2011/7/20/45
>>>>>>>>>>
>>>>>>>>>> this driver supports only physically continuous
>>>>>>>>>>                      
> memory(non-iommu).
>    
>>>>>>>>>> Links to previous versions of the patchset:
>>>>>>>>>> v1:<       https://lwn.net/Articles/454380/>
>>>>>>>>>> v2:<       http://www.spinics.net/lists/kernel/msg1224275.html>
>>>>>>>>>> v3:<       http://www.gossamer-
>>>>>>>>>>
>>>>>>>>>>                      
>>>> threads.com/lists/linux/kernel/1423684>
>>>>
>>>>          
>>>>>>>>>> Changelog v2:
>>>>>>>>>> DRM: add DRM_IOCTL_SAMSUNG_GEM_MMAP ioctl command.
>>>>>>>>>>
>>>>>>>>>>        this feature maps user address space to physical memory
>>>>>>>>>>
>>>>>>>>>>                      
>>>> region
>>>>
>>>>          
>>>>>>>>>>        once user application requests DRM_IOCTL_SAMSUNG_GEM_MMAP
>>>>>>>>>>
>>>>>>>>>>                      
>>>> ioctl.
>>>>
>>>>          
>>>>>>>>>> DRM: code clean and add exception codes.
>>>>>>>>>>
>>>>>>>>>> Changelog v3:
>>>>>>>>>> DRM: Support multiple irq.
>>>>>>>>>>
>>>>>>>>>>        FIMD and HDMI have their own irq handler but DRM Framework
>>>>>>>>>>
>>>>>>>>>>                      
>>>> can
>>>>
>>>>          
>>>>>>>>>> regiter
>>>>>>>>>>        only one irq handler this patch supports mutiple irq for
>>>>>>>>>> Samsung SoC.
>>>>>>>>>>
>>>>>>>>>> DRM: Consider modularization.
>>>>>>>>>>
>>>>>>>>>>        each DRM, FIMD could be built as a module.
>>>>>>>>>>
>>>>>>>>>> DRM: Have indenpendent crtc object.
>>>>>>>>>>
>>>>>>>>>>        crtc isn't specific to SoC Platform so this patch gets a
>>>>>>>>>>                      
>> crtc
>>      
>>>>>>>>>>        to be used as common object.
>>>>>>>>>>        created crtc could be attached to any encoder object.
>>>>>>>>>>
>>>>>>>>>> DRM: code clean and add exception codes.
>>>>>>>>>>
>>>>>>>>>> Changelog v4:
>>>>>>>>>> DRM: remove is_defult from samsung_fb.
>>>>>>>>>>
>>>>>>>>>>        is_default isn't used for default framebuffer.
>>>>>>>>>>
>>>>>>>>>> DRM: code refactoring to fimd module.
>>>>>>>>>>        this patch is be considered with multiple display objects
>>>>>>>>>>                      
>> and
>>      
>>>>>>>>>>        would use its own request_irq() to register a irq handler
>>>>>>>>>> instead of
>>>>>>>>>>        drm framework's one.
>>>>>>>>>>
>>>>>>>>>> DRM: remove find_samsung_drm_gem_object()
>>>>>>>>>>
>>>>>>>>>> DRM: move kernel private data structures and definitions to
>>>>>>>>>>                      
>> driver
>>      
>>>>>>>>>> folder.
>>>>>>>>>>
>>>>>>>>>>        samsung_drm.h would contain only public information for
>>>>>>>>>>
>>>>>>>>>>                      
>>>>> userspace
>>>>>
>>>>>            
>>>>>>>>>>        ioctl interface.
>>>>>>>>>>
>>>>>>>>>> DRM: code refactoring to gem modules.
>>>>>>>>>>        buffer module isn't dependent of gem module anymore.
>>>>>>>>>>
>>>>>>>>>> DRM: fixed security issue.
>>>>>>>>>>
>>>>>>>>>> DRM: remove encoder porinter from specific connector.
>>>>>>>>>>
>>>>>>>>>>        samsung connector doesn't need to have generic encoder.
>>>>>>>>>>
>>>>>>>>>> DRM: code clean and add exception codes.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Inki Dae<inki.dae@samsung.com>
>>>>>>>>>> Signed-off-by: Joonyoung Shim<jy0922.shim@samsung.com>
>>>>>>>>>> Signed-off-by: SeungWoo Kim<sw0312.kim@samsung.com>
>>>>>>>>>> Signed-off-by: kyungmin.park<kyungmin.park@samsung.com>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> +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.
>>
>>      
> Ok, I understood. but in case of using CMA, DRM driver would have private
> memory pool which is reserved only for itself. so although the video player
> requested gem buffer allocation until it receives an -ENOMEM and then the
> system is try to access the disc, it would work fine. because the region
> requested by system and the region requested by DRM driver could entirely be
> separated. DRM driver wouldn't have implications for the system region.
>
>    
>> 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.
>>
>> 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.
>>
>>      
> I think if we use private cma region for DRM driver then it is similar to
> via and sis way.
>
>    

In that case, I agree the driver should be OK.

...

Thanks,
Thomas