RE: [PATCH] DSPBRIDGE: Validate node handle from user

2010-02-11 Thread Ameya Palande 
On Tue, 2010-02-09 at 18:52 +0100, ext Ramos Falcon, Ernesto wrote:
 
 -Original Message-
 From: Ameya Palande [mailto:ameya.pala...@nokia.com]
 Sent: Tuesday, February 09, 2010 11:32 AM
 To: Ramos Falcon, Ernesto
 Cc: linux-omap@vger.kernel.org; Contreras Felipe (Nokia-D/Helsinki); Doyu
 Hiroshi (Nokia-D/Helsinki)
 Subject: Re: [PATCH] DSPBRIDGE: Validate node handle from user
 
 Hi Ernesto,
 
 On Tue, 2010-02-09 at 18:08 +0100, ext Ramos Falcon, Ernesto wrote:
  From 8310b586b025b0703c3951560849c4ea0250b6e1 Mon Sep 17 00:00:00 2001
  From: Ernesto Ramos erne...@ti.com
  Date: Fri, 29 Jan 2010 16:21:59 -0600
  Subject: [PATCH] DSPBRIDGE: Validate node handle from user.
 
  Add checks to validate the node handles received from user.
 
  Signed-off-by: Ernesto Ramos erne...@ti.com
  ---
   drivers/dsp/bridge/pmgr/wcd.c  |   91 -
   drivers/dsp/bridge/rmgr/node.c |  174 +-
 -
   2 files changed, 146 insertions(+), 119 deletions(-)
 
  diff --git a/drivers/dsp/bridge/pmgr/wcd.c
 b/drivers/dsp/bridge/pmgr/wcd.c
  index 74654dc..2e6eeb0 100644
  --- a/drivers/dsp/bridge/pmgr/wcd.c
  +++ b/drivers/dsp/bridge/pmgr/wcd.c
  @@ -1066,6 +1066,24 @@ u32 PROCWRAP_Stop(union Trapped_Args *args, void
 *pr_ctxt)
  return retVal;
   }
 
  +bool validate_node_handle(struct NODE_OBJECT *hNode, void *pr_ctxt)
  +{
  +   bool retVal = false;
  +   struct PROCESS_CONTEXT *pCtxt = pr_ctxt;
  +   struct NODE_RES_OBJECT *pNode = pCtxt-pNodeList;
  +
  +   if (hNode == (struct NODE_OBJECT *) DSP_HGPPNODE)
  +   retVal = true;
  +
  +   while (pNode  !retVal) {
  +   if (hNode == pNode-hNode)
 
 If you have several nodes allocated by user space, then what you are
 validating here is for any node! Is that ok?
 
 This validation and design itself doesn't look good to me. If we don't
 want to trust user space, then instead of checking the node handle in
 every function it is better to store all user space specific date inside
 pr_context and use it from there.
 
 
 The user can launch several nodes, how are we going to know which node handle 
 to use?
 I think we may need to receive at least one index or id to the node handle.

Yes, I guess thats the correct way! We need to maintain how many nodes
are allocated for a user process, and just make sure that this id /
index is = number of allocated nodes.

Cheers,
Ameya.

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] DSPBRIDGE: Validate node handle from user

2010-02-11 Thread Ramos Falcon, Ernesto 


-Original Message-
From: Ameya Palande [mailto:ameya.pala...@nokia.com]
Sent: Thursday, February 11, 2010 7:51 AM
To: Ramos Falcon, Ernesto
Cc: linux-omap@vger.kernel.org; Contreras Felipe (Nokia-D/Helsinki); Doyu
Hiroshi (Nokia-D/Helsinki)
Subject: RE: [PATCH] DSPBRIDGE: Validate node handle from user

On Tue, 2010-02-09 at 18:52 +0100, ext Ramos Falcon, Ernesto wrote:

 -Original Message-
 From: Ameya Palande [mailto:ameya.pala...@nokia.com]
 Sent: Tuesday, February 09, 2010 11:32 AM
 To: Ramos Falcon, Ernesto
 Cc: linux-omap@vger.kernel.org; Contreras Felipe (Nokia-D/Helsinki);
Doyu
 Hiroshi (Nokia-D/Helsinki)
 Subject: Re: [PATCH] DSPBRIDGE: Validate node handle from user
 
 Hi Ernesto,
 
 On Tue, 2010-02-09 at 18:08 +0100, ext Ramos Falcon, Ernesto wrote:
  From 8310b586b025b0703c3951560849c4ea0250b6e1 Mon Sep 17 00:00:00 2001
  From: Ernesto Ramos erne...@ti.com
  Date: Fri, 29 Jan 2010 16:21:59 -0600
  Subject: [PATCH] DSPBRIDGE: Validate node handle from user.
 
  Add checks to validate the node handles received from user.
 
  Signed-off-by: Ernesto Ramos erne...@ti.com
  ---
   drivers/dsp/bridge/pmgr/wcd.c  |   91 -
   drivers/dsp/bridge/rmgr/node.c |  174 +--
---
 -
   2 files changed, 146 insertions(+), 119 deletions(-)
 
  diff --git a/drivers/dsp/bridge/pmgr/wcd.c
 b/drivers/dsp/bridge/pmgr/wcd.c
  index 74654dc..2e6eeb0 100644
  --- a/drivers/dsp/bridge/pmgr/wcd.c
  +++ b/drivers/dsp/bridge/pmgr/wcd.c
  @@ -1066,6 +1066,24 @@ u32 PROCWRAP_Stop(union Trapped_Args *args,
void
 *pr_ctxt)
  return retVal;
   }
 
  +bool validate_node_handle(struct NODE_OBJECT *hNode, void *pr_ctxt)
  +{
  +   bool retVal = false;
  +   struct PROCESS_CONTEXT *pCtxt = pr_ctxt;
  +   struct NODE_RES_OBJECT *pNode = pCtxt-pNodeList;
  +
  +   if (hNode == (struct NODE_OBJECT *) DSP_HGPPNODE)
  +   retVal = true;
  +
  +   while (pNode  !retVal) {
  +   if (hNode == pNode-hNode)
 
 If you have several nodes allocated by user space, then what you are
 validating here is for any node! Is that ok?
 
 This validation and design itself doesn't look good to me. If we don't
 want to trust user space, then instead of checking the node handle in
 every function it is better to store all user space specific date inside
 pr_context and use it from there.
 

 The user can launch several nodes, how are we going to know which node
handle to use?
 I think we may need to receive at least one index or id to the node
handle.

Yes, I guess thats the correct way! We need to maintain how many nodes
are allocated for a user process, and just make sure that this id /
index is = number of allocated nodes.


This comparison (=) won't work because the user can eliminate nodes in the 
middle of the list in which case we may need to maintain a list of valid 
indexes. So I don't see any improvement with this way to validate the handles. 

I was thinking in an array an based on the index verify that the handle is 
valid, independently of the number of nodes this would be very quick, but the 
disadvantage would be that the number per process will be limited to the size 
of the array.

Cheers,
Ameya.

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] DSPBRIDGE: Validate node handle from user

2010-02-11 Thread Ameya Palande 
Hi Ernesto,

On Thu, 2010-02-11 at 19:44 +0100, ext Ramos Falcon, Ernesto wrote:
 
 -Original Message-
 From: Ameya Palande [mailto:ameya.pala...@nokia.com]
 Sent: Thursday, February 11, 2010 7:51 AM
 To: Ramos Falcon, Ernesto
 Cc: linux-omap@vger.kernel.org; Contreras Felipe (Nokia-D/Helsinki); Doyu
 Hiroshi (Nokia-D/Helsinki)
 Subject: RE: [PATCH] DSPBRIDGE: Validate node handle from user
 
 On Tue, 2010-02-09 at 18:52 +0100, ext Ramos Falcon, Ernesto wrote:
 
  -Original Message-
  From: Ameya Palande [mailto:ameya.pala...@nokia.com]
  Sent: Tuesday, February 09, 2010 11:32 AM
  To: Ramos Falcon, Ernesto
  Cc: linux-omap@vger.kernel.org; Contreras Felipe (Nokia-D/Helsinki);
 Doyu
  Hiroshi (Nokia-D/Helsinki)
  Subject: Re: [PATCH] DSPBRIDGE: Validate node handle from user
  
  Hi Ernesto,
  
  On Tue, 2010-02-09 at 18:08 +0100, ext Ramos Falcon, Ernesto wrote:
   From 8310b586b025b0703c3951560849c4ea0250b6e1 Mon Sep 17 00:00:00 2001
   From: Ernesto Ramos erne...@ti.com
   Date: Fri, 29 Jan 2010 16:21:59 -0600
   Subject: [PATCH] DSPBRIDGE: Validate node handle from user.
  
   Add checks to validate the node handles received from user.
  
   Signed-off-by: Ernesto Ramos erne...@ti.com

SNIP

  If you have several nodes allocated by user space, then what you are
  validating here is for any node! Is that ok?
  
  This validation and design itself doesn't look good to me. If we don't
  want to trust user space, then instead of checking the node handle in
  every function it is better to store all user space specific date inside
  pr_context and use it from there.
  
 
  The user can launch several nodes, how are we going to know which node
 handle to use?
  I think we may need to receive at least one index or id to the node
 handle.
 
 Yes, I guess thats the correct way! We need to maintain how many nodes
 are allocated for a user process, and just make sure that this id /
 index is = number of allocated nodes.
 
 
 This comparison (=) won't work because the user can eliminate nodes in the 
 middle of the list in which case we may need to maintain a list of valid 
 indexes. So I don't see any improvement with this way to validate the 
 handles. 

I agree :(

 I was thinking in an array an based on the index verify that the handle is 
 valid, independently of the number of nodes this would be very quick, but the 
 disadvantage would be that the number per process will be limited to the size 
 of the array.

Yes, array has disadvantage of size, but Linux also uses initial array
of 32 size for storing file pointers. 

On a second thought our problem looks similar to following use case:

User space application opens several files using open() system call and
receives handles for each. Now it passes handle to kernel using read(),
write() system calls to identify the file to operate on. How does kernel
validates the handle received from user space?

I guess we can base our solution on a bitmap to find next available
slot:

DECLARE_BITMAP();
find_first_zero_bit();
set_bit();
clear_bit();

And use array[free_slot] to store the pointer.

With solution I guess we can get rid of these checks ;)

Cheers,
Ameya.

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] DSPBRIDGE: Validate node handle from user

2010-02-09 Thread Ramos Falcon, Ernesto 
From 8310b586b025b0703c3951560849c4ea0250b6e1 Mon Sep 17 00:00:00 2001
From: Ernesto Ramos erne...@ti.com
Date: Fri, 29 Jan 2010 16:21:59 -0600
Subject: [PATCH] DSPBRIDGE: Validate node handle from user.

Add checks to validate the node handles received from user.

Signed-off-by: Ernesto Ramos erne...@ti.com
---
 drivers/dsp/bridge/pmgr/wcd.c  |   91 -
 drivers/dsp/bridge/rmgr/node.c |  174 +--
 2 files changed, 146 insertions(+), 119 deletions(-)

diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
index 74654dc..2e6eeb0 100644
--- a/drivers/dsp/bridge/pmgr/wcd.c
+++ b/drivers/dsp/bridge/pmgr/wcd.c
@@ -1066,6 +1066,24 @@ u32 PROCWRAP_Stop(union Trapped_Args *args, void 
*pr_ctxt)
return retVal;
 }

+bool validate_node_handle(struct NODE_OBJECT *hNode, void *pr_ctxt)
+{
+   bool retVal = false;
+   struct PROCESS_CONTEXT *pCtxt = pr_ctxt;
+   struct NODE_RES_OBJECT *pNode = pCtxt-pNodeList;
+
+   if (hNode == (struct NODE_OBJECT *) DSP_HGPPNODE)
+   retVal = true;
+
+   while (pNode  !retVal) {
+   if (hNode == pNode-hNode)
+   retVal = true;
+   pNode = pNode-next;
+   }
+
+   return retVal;
+}
+
 /*
  *  NODEWRAP_Allocate 
  */
@@ -1140,6 +1158,10 @@ u32 NODEWRAP_AllocMsgBuf(union Trapped_Args *args, void 
*pr_ctxt)
if (!args-ARGS_NODE_ALLOCMSGBUF.uSize)
return DSP_ESIZE;

+   if (!validate_node_handle(args-ARGS_NODE_ALLOCMSGBUF.hNode,
+   pr_ctxt))
+   return DSP_EHANDLE;
+
if (args-ARGS_NODE_ALLOCMSGBUF.pAttr) {/* Optional argument */
cp_fm_usr(attr, args-ARGS_NODE_ALLOCMSGBUF.pAttr, status, 1);
if (DSP_SUCCEEDED(status))
@@ -1148,6 +1170,7 @@ u32 NODEWRAP_AllocMsgBuf(union Trapped_Args *args, void 
*pr_ctxt)
}
/* IN OUT argument */
cp_fm_usr(pBuffer, args-ARGS_NODE_ALLOCMSGBUF.pBuffer, status, 1);
+
if (DSP_SUCCEEDED(status)) {
status = NODE_AllocMsgBuf(args-ARGS_NODE_ALLOCMSGBUF.hNode,
 args-ARGS_NODE_ALLOCMSGBUF.uSize,
@@ -1166,6 +1189,11 @@ u32 NODEWRAP_ChangePriority(union Trapped_Args *args, 
void *pr_ctxt)

GT_0trace(WCD_debugMask, GT_ENTER,
 NODEWRAP_ChangePriority: entered\n);
+
+   if (!validate_node_handle(args-ARGS_NODE_CHANGEPRIORITY.hNode,
+   pr_ctxt))
+   return DSP_EHANDLE;
+
retVal = NODE_ChangePriority(args-ARGS_NODE_CHANGEPRIORITY.hNode,
args-ARGS_NODE_CHANGEPRIORITY.iPriority);

@@ -1186,6 +1214,13 @@ u32 NODEWRAP_Connect(union Trapped_Args *args, void 
*pr_ctxt)

GT_0trace(WCD_debugMask, GT_ENTER, NODEWRAP_Connect: entered\n);

+   if (!validate_node_handle(args-ARGS_NODE_CONNECT.hNode,
+   pr_ctxt) ||
+   !validate_node_handle(args-ARGS_NODE_CONNECT.hOtherNode,
+   pr_ctxt)) {
+   status = DSP_EHANDLE;
+   goto func_cont;
+   }
/* Optional argument */
if (pSize) {
if (get_user(cbDataSize, pSize))
@@ -1211,6 +1246,7 @@ u32 NODEWRAP_Connect(union Trapped_Args *args, void 
*pr_ctxt)
pAttrs = attrs;

}
+
if (DSP_SUCCEEDED(status)) {
status = NODE_Connect(args-ARGS_NODE_CONNECT.hNode,
 args-ARGS_NODE_CONNECT.uStream,
@@ -1233,6 +1269,11 @@ u32 NODEWRAP_Create(union Trapped_Args *args, void 
*pr_ctxt)
u32 retVal;

GT_0trace(WCD_debugMask, GT_ENTER, NODEWRAP_Create: entered\n);
+
+   if (!validate_node_handle(args-ARGS_NODE_CREATE.hNode,
+   pr_ctxt))
+   return DSP_EHANDLE;
+
retVal = NODE_Create(args-ARGS_NODE_CREATE.hNode);

return retVal;
@@ -1246,6 +1287,11 @@ u32 NODEWRAP_Delete(union Trapped_Args *args, void 
*pr_ctxt)
u32 retVal;

GT_0trace(WCD_debugMask, GT_ENTER, NODEWRAP_Delete: entered\n);
+
+   if (!validate_node_handle(args-ARGS_NODE_DELETE.hNode,
+   pr_ctxt))
+   return DSP_EHANDLE;
+
retVal = NODE_Delete(args-ARGS_NODE_DELETE.hNode, pr_ctxt);

return retVal;
@@ -1259,6 +1305,14 @@ u32 NODEWRAP_FreeMsgBuf(union Trapped_Args *args, void 
*pr_ctxt)
DSP_STATUS status = DSP_SOK;
struct DSP_BUFFERATTR *pAttr = NULL;
struct DSP_BUFFERATTR attr;
+
+   if (!args-ARGS_NODE_FREEMSGBUF.pBuffer)
+   return DSP_EPOINTER;
+
+   if (!validate_node_handle(args-ARGS_NODE_FREEMSGBUF.hNode,
+   pr_ctxt))
+   return DSP_EHANDLE;
+
if (args-ARGS_NODE_FREEMSGBUF.pAttr) { /* Optional argument */
cp_fm_usr(attr, args-ARGS_NODE_FREEMSGBUF.pAttr, status, 1);
if (DSP_SUCCEEDED(status))
@@ -1266,9 +1320,6 @@ u32

Re: [PATCH] DSPBRIDGE: Validate node handle from user

2010-02-09 Thread Ameya Palande 
Hi Ernesto,

On Tue, 2010-02-09 at 18:08 +0100, ext Ramos Falcon, Ernesto wrote:
 From 8310b586b025b0703c3951560849c4ea0250b6e1 Mon Sep 17 00:00:00 2001
 From: Ernesto Ramos erne...@ti.com
 Date: Fri, 29 Jan 2010 16:21:59 -0600
 Subject: [PATCH] DSPBRIDGE: Validate node handle from user.
 
 Add checks to validate the node handles received from user.
 
 Signed-off-by: Ernesto Ramos erne...@ti.com
 ---
  drivers/dsp/bridge/pmgr/wcd.c  |   91 -
  drivers/dsp/bridge/rmgr/node.c |  174 +--
  2 files changed, 146 insertions(+), 119 deletions(-)
 
 diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
 index 74654dc..2e6eeb0 100644
 --- a/drivers/dsp/bridge/pmgr/wcd.c
 +++ b/drivers/dsp/bridge/pmgr/wcd.c
 @@ -1066,6 +1066,24 @@ u32 PROCWRAP_Stop(union Trapped_Args *args, void 
 *pr_ctxt)
 return retVal;
  }
 
 +bool validate_node_handle(struct NODE_OBJECT *hNode, void *pr_ctxt)
 +{
 +   bool retVal = false;
 +   struct PROCESS_CONTEXT *pCtxt = pr_ctxt;
 +   struct NODE_RES_OBJECT *pNode = pCtxt-pNodeList;
 +
 +   if (hNode == (struct NODE_OBJECT *) DSP_HGPPNODE)
 +   retVal = true;
 +
 +   while (pNode  !retVal) {
 +   if (hNode == pNode-hNode)

If you have several nodes allocated by user space, then what you are
validating here is for any node! Is that ok?

This validation and design itself doesn't look good to me. If we don't
want to trust user space, then instead of checking the node handle in
every function it is better to store all user space specific date inside
pr_context and use it from there.

Cheers,
Ameya.

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] DSPBRIDGE: Validate node handle from user

2010-02-09 Thread Ramos Falcon, Ernesto 


-Original Message-
From: Ameya Palande [mailto:ameya.pala...@nokia.com]
Sent: Tuesday, February 09, 2010 11:32 AM
To: Ramos Falcon, Ernesto
Cc: linux-omap@vger.kernel.org; Contreras Felipe (Nokia-D/Helsinki); Doyu
Hiroshi (Nokia-D/Helsinki)
Subject: Re: [PATCH] DSPBRIDGE: Validate node handle from user

Hi Ernesto,

On Tue, 2010-02-09 at 18:08 +0100, ext Ramos Falcon, Ernesto wrote:
 From 8310b586b025b0703c3951560849c4ea0250b6e1 Mon Sep 17 00:00:00 2001
 From: Ernesto Ramos erne...@ti.com
 Date: Fri, 29 Jan 2010 16:21:59 -0600
 Subject: [PATCH] DSPBRIDGE: Validate node handle from user.

 Add checks to validate the node handles received from user.

 Signed-off-by: Ernesto Ramos erne...@ti.com
 ---
  drivers/dsp/bridge/pmgr/wcd.c  |   91 -
  drivers/dsp/bridge/rmgr/node.c |  174 +-
-
  2 files changed, 146 insertions(+), 119 deletions(-)

 diff --git a/drivers/dsp/bridge/pmgr/wcd.c
b/drivers/dsp/bridge/pmgr/wcd.c
 index 74654dc..2e6eeb0 100644
 --- a/drivers/dsp/bridge/pmgr/wcd.c
 +++ b/drivers/dsp/bridge/pmgr/wcd.c
 @@ -1066,6 +1066,24 @@ u32 PROCWRAP_Stop(union Trapped_Args *args, void
*pr_ctxt)
 return retVal;
  }

 +bool validate_node_handle(struct NODE_OBJECT *hNode, void *pr_ctxt)
 +{
 +   bool retVal = false;
 +   struct PROCESS_CONTEXT *pCtxt = pr_ctxt;
 +   struct NODE_RES_OBJECT *pNode = pCtxt-pNodeList;
 +
 +   if (hNode == (struct NODE_OBJECT *) DSP_HGPPNODE)
 +   retVal = true;
 +
 +   while (pNode  !retVal) {
 +   if (hNode == pNode-hNode)

If you have several nodes allocated by user space, then what you are
validating here is for any node! Is that ok?

This validation and design itself doesn't look good to me. If we don't
want to trust user space, then instead of checking the node handle in
every function it is better to store all user space specific date inside
pr_context and use it from there.


The user can launch several nodes, how are we going to know which node handle 
to use?
I think we may need to receive at least one index or id to the node handle.


Cheers,
Ameya.

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html