Re: [Patch VIA UniChrome DRM][2/5 Ver1] Add support for video command flush and interface for V4L kernel module
brucech...@via.com.tw wrote:
Bruce, Others.
The v4l interfaces look ok to me.
However, the double-buffer flush mechanism does not.
If I understand the code correctly, the user-space application prepares
command buffers directly in AGP, and asks the
drm module to submit them. We can't allow this for security reasons.
The user-space application could for example fill the buffer with
commands to texture from arbitrary system memory, getting hold of other
user's private data.
The whole ring-buffer stuff and the command verifier was once
implemented to fix that security problem.
I mentioned this in the last email I posted as a comment to the big
patch, and detailed the unichrome drm module security policy.
So this patch is a NAK from my side.
/Thomas
> Hello All:
> This is patch 2 of the 5 patches. It's based on 2.6.32-rc3+patch1 and for
> the support of
> video command flush(using double buffer to improve performance) and
> interfaces for
> communication with v4l kernel module.
>
> 5 files has been modified for this patch.
>
> Sign-off by Bruce C. Chang
> diff -Nur linux-2.6.32-rc3-old/drivers/gpu/drm/via/via_dma.c
> linux-2.6.32-rc3-new/drivers/gpu/drm/via/via_dma.c
> --- linux-2.6.32-rc3-old/drivers/gpu/drm/via/via_dma.c2009-10-08
> 10:14:16.0 +0800
> +++ linux-2.6.32-rc3-new/drivers/gpu/drm/via/via_dma.c2009-10-08
> 10:14:29.0 +0800
> @@ -68,6 +68,13 @@
> *vb++ = (w2); \
> dev_priv->dma_low += 8;
>
> +#define VIA_OUT_VIDEO_AGP_BUFFER(cmd1, cmd2)\
> + do {\
> + *cur_virtual++ = cmd1; \
> + *cur_virtual++ = cmd2; \
> + cmdbuf_info.cmd_size += 8; \
> + } while (0);
> +
> static void via_cmdbuf_flush(struct drm_via_private *dev_priv,
> uint32_t cmd_type);
> static void via_cmdbuf_start(drm_via_private_t * dev_priv);
> @@ -158,6 +165,7 @@
> int via_dma_cleanup(struct drm_device * dev)
> {
> struct drm_via_video_save_head *pnode;
> + int i;
>
> for (pnode = via_video_save_head; pnode; pnode =
> (struct drm_via_video_save_head *)pnode->next)
> @@ -167,12 +175,23 @@
> (drm_via_private_t *) dev->dev_private;
>
> if (dev_priv->ring.virtual_start) {
> - via_cmdbuf_reset(dev_priv);
> + if (dev_priv->cr_status == CR_FOR_RINGBUFFER)
> + via_cmdbuf_flush(dev_priv, HC_HAGPBpID_STOP);
> +
> + via_wait_idle(dev_priv);
>
> drm_core_ioremapfree(&dev_priv->ring.map, dev);
> dev_priv->ring.virtual_start = NULL;
> }
>
> + for (i = 0; i < 3; i++) {
> + if (dev_priv->video_agp_address_map[i].handle &&
> + dev_priv->video_agp_address_map[i].size)
> + drm_core_ioremapfree(dev_priv->
> + video_agp_address_map+i, dev);
> + /*Fix for suspend reuse video buf*/
> + dev_priv->video_agp_address_map[i].handle = NULL;
> + }
> }
>
> return 0;
> @@ -235,6 +254,7 @@
>
> via_cmdbuf_start(dev_priv);
>
> + dev_priv->cr_status = CR_FOR_RINGBUFFER;
> return 0;
> }
>
> @@ -343,12 +363,42 @@
> static int via_cmdbuffer(struct drm_device *dev, void *data, struct drm_file
> *file_priv)
> {
> drm_via_cmdbuffer_t *cmdbuf = data;
> - int ret;
> + drm_via_private_t *dev_priv = dev->dev_private;
> + int ret = 0, count;
>
> LOCK_TEST_WITH_RETURN(dev, file_priv);
>
> DRM_DEBUG("buf %p size %lu\n", cmdbuf->buf, cmdbuf->size);
>
> + if (dev_priv->cr_status == CR_FOR_VIDEO) {
> + /* Because our driver will hook CR stop cmd behind video cmd,
> + * all we need to do here is to wait for CR idle,
> + * and initialize ring buffer.
> + */
> + count = 1000;
> + while (count-- && (VIA_READ(VIA_REG_STATUS) &
> + VIA_CMD_RGTR_BUSY))
> + cpu_relax();
> + /* Seldom happen */
> + if (count < 0) {
> + DRM_INFO("The CR can't be idle from video agp cmd \
> + dispatch when it is needed by ring buffer \n");
> + return -1;
> + }
> + /* CR has been idle so that we need to initialize ring buffer */
> + dev_priv->dma_ptr = dev_priv->ring.virtual_start;
> + dev_priv->dma_low = 0;
> + dev_priv->dma_high = 0x100;
> + dev_priv->dma_wrap = 0x100;
> + dev_priv->dma_offset = 0x0;
> + dev_priv->last_pause_ptr = NULL;
> + dev_priv->hw_addr_ptr = dev_priv->mmio->handle + 0x418;
> +
> + via_cmdbuf_start(dev
RE: [Patch VIA UniChrome DRM][2/5 Ver1] Add support for video command flush and interface for V4L kernel module
Hello Thomas: > If I understand the code correctly, the user-space application prepares > command buffers directly in AGP, and asks the > drm module to submit them. We can't allow this for security reasons. The > user-space application could for example fill the buffer with > commands to texture from arbitrary system memory, getting hold of other > user's private data. > The whole ring-buffer stuff and the command verifier was once > implemented to fix that security problem. Thank you very much for your comment. What if we do a security check in these buffer before submit? Let me check if there is any way to work around for this security issue. Thanks and Best Regards = Bruce C. Chang(張祖明) VIA Technologies, Inc. Address: 1F, 531, Chung-Cheng Road, Hsin-Tien, 231 Taipei Tel: +886-2-22185452 Ext 7323 Mobile: +886-968343824 Fax: +886-2-22186282 Skype: Bruce.C.Chang Email: brucech...@via.com.tw -- Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
RE: [Patch VIA UniChrome DRM][2/5 Ver1] Add support for video command flush and interface for V4L kernel module
On Thu, 2009-10-08 at 02:35 -0700, brucech...@via.com.tw wrote: > Hello Thomas: > > > If I understand the code correctly, the user-space application prepares > > command buffers directly in AGP, and asks the > > drm module to submit them. We can't allow this for security reasons. The > > user-space application could for example fill the buffer with > > commands to texture from arbitrary system memory, getting hold of other > > user's private data. > > The whole ring-buffer stuff and the command verifier was once > > implemented to fix that security problem. > Thank you very much for your comment. What if we do a security > check in these buffer before submit? Let me check if there is any way > to work around for this security issue. Who would do that security check? Userspace? That doesn't work as userspace is not trusted. The kernel? Ok, but now it's reading commands out of a presumably write-combined AGP buffer, which is slow. You'd have been better off passing the commands to the kernel in regular memory, which is presumably exactly what the existing mechanism does. Keith -- Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [Patch VIA UniChrome DRM][2/5 Ver1] Add support for video command flush and interface for V4L kernel module
Dear Bruce, On Thu, Oct 08, 2009 at 05:35:51PM +0800, brucech...@via.com.tw wrote: > > If I understand the code correctly, the user-space application prepares > > command buffers directly in AGP, and asks the > > drm module to submit them. We can't allow this for security reasons. The > > user-space application could for example fill the buffer with > > commands to texture from arbitrary system memory, getting hold of other > > user's private data. > > The whole ring-buffer stuff and the command verifier was once > > implemented to fix that security problem. > > Thank you very much for your comment. What if we do a security check in these > buffer before submit? Let me check if there is any way to work around for > this security issue. Bruce, let me clarify: The fundamental assumiptions are: * the operating system kernel enforces security / permisssion between processes * DRM is used by an application which is running by one particular user * thus, the kernel needs to make security checks to ensure that whatever the application does will not violate the security constraints, i.e. * DRM api can not allow arbitrary memory read/write to physical addresses So if you want to add a security check to those buffers, the check has to be inside the kernel. Only the kernel can be trusted, not the userspace application that talks to the DRM API/ABI. Regards, -- - Harald Welte http://linux.via.com.tw/ VIA Free and Open Source Software Liaison -- Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
RE: [Patch VIA UniChrome DRM][2/5 Ver1] Add support for video command flush and interface for V4L kernel module
Hello Keith: Thank you very much for your suggestion. Allow me to discuss with our engineers. Regards = Bruce C. Chang(張祖明) VIA Technologies, Inc. Address: 1F, 531, Chung-Cheng Road, Hsin-Tien, 231 Taipei Tel: +886-2-22185452 Ext 7323 Mobile: +886-968343824 Fax: +886-2-22186282 Skype: Bruce.C.Chang Email: brucech...@via.com.tw -Original Message- From: Keith Whitwell [mailto:kei...@vmware.com] Sent: Thursday, October 08, 2009 6:11 PM To: Bruce Chang Cc: tho...@shipmail.org; Joseph Chan; dri-devel@lists.sourceforge.net; Harald Welte; Benjamin Chen Subject: RE: [Patch VIA UniChrome DRM][2/5 Ver1] Add support for video command flush and interface for V4L kernel module On Thu, 2009-10-08 at 02:35 -0700, brucech...@via.com.tw wrote: > Hello Thomas: > > > If I understand the code correctly, the user-space application > > prepares > > command buffers directly in AGP, and asks the > > drm module to submit them. We can't allow this for security reasons. The > > user-space application could for example fill the buffer with > > commands to texture from arbitrary system memory, getting hold of other > > user's private data. > > The whole ring-buffer stuff and the command verifier was once > > implemented to fix that security problem. > Thank you very much for your comment. What if we do a security > check in these buffer before submit? Let me check if there is any way > to work around for this security issue. Who would do that security check? Userspace? That doesn't work as userspace is not trusted. The kernel? Ok, but now it's reading commands out of a presumably write-combined AGP buffer, which is slow. You'd have been better off passing the commands to the kernel in regular memory, which is presumably exactly what the existing mechanism does. Keith -- Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
RE: [Patch VIA UniChrome DRM][2/5 Ver1] Add support for video command flush and interface for V4L kernel module
Hello Harald: Thank you very much for your education. We should go with better way. Regards = Bruce C. Chang(張祖明) VIA Technologies, Inc. Address: 1F, 531, Chung-Cheng Road, Hsin-Tien, 231 Taipei Tel: +886-2-22185452 Ext 7323 Mobile: +886-968343824 Fax: +886-2-22186282 Skype: Bruce.C.Chang Email: brucech...@via.com.tw -Original Message- From: Harald Welte [mailto:haraldwe...@viatech.com] Sent: Thursday, October 08, 2009 5:56 PM To: Bruce Chang Cc: tho...@shipmail.org; dri-devel@lists.sourceforge.net; airl...@gmail.com; Benjamin Chen; Joseph Chan Subject: Re: [Patch VIA UniChrome DRM][2/5 Ver1] Add support for video command flush and interface for V4L kernel module Dear Bruce, On Thu, Oct 08, 2009 at 05:35:51PM +0800, brucech...@via.com.tw wrote: > > If I understand the code correctly, the user-space application > > prepares > > command buffers directly in AGP, and asks the > > drm module to submit them. We can't allow this for security reasons. The > > user-space application could for example fill the buffer with > > commands to texture from arbitrary system memory, getting hold of other > > user's private data. > > The whole ring-buffer stuff and the command verifier was once > > implemented to fix that security problem. > > Thank you very much for your comment. What if we do a security check > in these buffer before submit? Let me check if there is any way to > work around for this security issue. Bruce, let me clarify: The fundamental assumiptions are: * the operating system kernel enforces security / permisssion between processes * DRM is used by an application which is running by one particular user * thus, the kernel needs to make security checks to ensure that whatever the application does will not violate the security constraints, i.e. * DRM api can not allow arbitrary memory read/write to physical addresses So if you want to add a security check to those buffers, the check has to be inside the kernel. Only the kernel can be trusted, not the userspace application that talks to the DRM API/ABI. Regards, -- - Harald Welte http://linux.via.com.tw/ VIA Free and Open Source Software Liaison -- Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [Patch VIA UniChrome DRM][2/5 Ver1] Add support for video command flush and interface for V4L kernel module
brucech...@via.com.tw wrote: > Hello Thomas: > > >> If I understand the code correctly, the user-space application prepares >> command buffers directly in AGP, and asks the >> drm module to submit them. We can't allow this for security reasons. The >> user-space application could for example fill the buffer with >> commands to texture from arbitrary system memory, getting hold of other >> user's private data. >> The whole ring-buffer stuff and the command verifier was once >> implemented to fix that security problem. >> > Thank you very much for your comment. What if we do a security check in > these buffer before submit? Let me check if there is any way to work around > for this security issue. > Bruce, I was wrong in that your patch 5/5 actually adds security checks to the buffers. However, there are two problems with that patch 1) It does security check on AGP memory, and as Keith pointed out, that's dead slow. 2) User-space could alter the contents of the AGP buffers just after the security check has been done, but before command submission. So I suggest you alter the user-space components to use the ring-buffer submission. It is a little more CPU-consuming, but I guess you have to live with it. /Thomas > Thanks and Best Regards > = > Bruce C. Chang(張祖明) > VIA Technologies, Inc. > Address: 1F, 531, Chung-Cheng Road, Hsin-Tien, 231 Taipei > Tel: +886-2-22185452 Ext 7323 > Mobile: +886-968343824 > Fax: +886-2-22186282 > Skype: Bruce.C.Chang > Email: brucech...@via.com.tw > -- Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
