On Wed, Sep 14, 2011 at 2:57 AM, Thomas Hellstrom <thomas@xxxxxxxxxxxx> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel