Hi Thomas. > -----Original Message----- > From: Thomas Hellstrom [mailto:thomas@xxxxxxxxxxxx] > Sent: Monday, September 12, 2011 3:32 PM > To: Rob Clark > Cc: Inki Dae; kyungmin.park@xxxxxxxxxxx; sw0312.kim@xxxxxxxxxxx; linux- > arm-kernel@xxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx > 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@xxxxxxxxxxxx> 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@xxxxxxxxxxx> > >>>> Signed-off-by: Joonyoung Shim<jy0922.shim@xxxxxxxxxxx> > >>>> Signed-off-by: SeungWoo Kim<sw0312.kim@xxxxxxxxxxx> > >>>> Signed-off-by: kyungmin.park<kyungmin.park@xxxxxxxxxxx> > >>>> --- > >>>> > >>>> +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. > /Thomas > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
