[PATCH] DSPBRIDGE: check pointer before calling Proc_Detach

[PATCH] DSPBRIDGE: check pointer before calling Proc_Detach

[Omar Ramirez Luna]

From: Ernest Ramos Falcon <ernesto@ti.com>

This patch adds a check before calling Proc_Detach, otherwise
in case a failure occurs on PROC_Attach and hProcessor is
invalid when the userspace task closes its handle, unexpected
behavior might be seen as this pointer is dereferenced in
PROC_Detach.

Signed-off-by: Ernest Ramos Falcon <ernesto@ti.com>
---
 drivers/dsp/bridge/rmgr/drv_interface.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c
index 32aff66..b0f0d93 100644
--- a/drivers/dsp/bridge/rmgr/drv_interface.c
+++ b/drivers/dsp/bridge/rmgr/drv_interface.c
@@ -539,7 +539,8 @@ static int bridge_release(struct inode *ip, struct file *filp)
 	pr_ctxt = filp->private_data;
 	flush_signals(current);
 	DRV_RemoveAllResources(pr_ctxt);
-	PROC_Detach(pr_ctxt);
+	if (pr_ctxt->hProcessor)
+		PROC_Detach(pr_ctxt);
 	MEM_Free(pr_ctxt);
 
 	filp->private_data = NULL;
-- 
1.6.2.4

Re: [PATCH] DSPBRIDGE: check pointer before calling Proc_Detach

[Ameya Palande]

Hi,

On Thu, 2010-02-04 at 22:17 +0100, ext Omar Ramirez Luna wrote:
> From: Ernest Ramos Falcon <ernesto@ti.com>
> 
> This patch adds a check before calling Proc_Detach, otherwise
> in case a failure occurs on PROC_Attach and hProcessor is
> invalid when the userspace task closes its handle, unexpected
> behavior might be seen as this pointer is dereferenced in
> PROC_Detach.
> 
> Signed-off-by: Ernest Ramos Falcon <ernesto@ti.com>
> ---
>  drivers/dsp/bridge/rmgr/drv_interface.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c
> index 32aff66..b0f0d93 100644
> --- a/drivers/dsp/bridge/rmgr/drv_interface.c
> +++ b/drivers/dsp/bridge/rmgr/drv_interface.c
> @@ -539,7 +539,8 @@ static int bridge_release(struct inode *ip, struct file *filp)
>  	pr_ctxt = filp->private_data;
>  	flush_signals(current);
>  	DRV_RemoveAllResources(pr_ctxt);
> -	PROC_Detach(pr_ctxt);
> +	if (pr_ctxt->hProcessor)
> +		PROC_Detach(pr_ctxt);
>  	MEM_Free(pr_ctxt);
>  
>  	filp->private_data = NULL;

I guess PROC_Detach already checks for a valid hProcessor handle!
Why we need this check here again?

Also if you go ahead with this patch, then how are you going to cover a
use case where userspace application calls PROC_Attach() which fails,
but without checking the failure userspace application calls
PROC_Detach()!

Cheers,
Ameya.

RE: [PATCH] DSPBRIDGE: check pointer before calling Proc_Detach

[Ramos Falcon, Ernesto]

>-----Original Message-----
>From: Ameya Palande [mailto:ameya.palande@nokia.com]
>Sent: Wednesday, February 10, 2010 7:48 AM
>To: Ramirez Luna, Omar
>Cc: linux-omap; Doyu Hiroshi (Nokia-D/Helsinki); Contreras Felipe (Nokia-
>D/Helsinki); Menon, Nishanth; Ramos Falcon, Ernesto
>Subject: Re: [PATCH] DSPBRIDGE: check pointer before calling Proc_Detach
>
>Hi,
>
>On Thu, 2010-02-04 at 22:17 +0100, ext Omar Ramirez Luna wrote:
>> From: Ernest Ramos Falcon <ernesto@ti.com>
>>
>> This patch adds a check before calling Proc_Detach, otherwise
>> in case a failure occurs on PROC_Attach and hProcessor is
>> invalid when the userspace task closes its handle, unexpected
>> behavior might be seen as this pointer is dereferenced in
>> PROC_Detach.
>>
>> Signed-off-by: Ernest Ramos Falcon <ernesto@ti.com>
>> ---
>>  drivers/dsp/bridge/rmgr/drv_interface.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c
>b/drivers/dsp/bridge/rmgr/drv_interface.c
>> index 32aff66..b0f0d93 100644
>> --- a/drivers/dsp/bridge/rmgr/drv_interface.c
>> +++ b/drivers/dsp/bridge/rmgr/drv_interface.c
>> @@ -539,7 +539,8 @@ static int bridge_release(struct inode *ip, struct
>file *filp)
>>  	pr_ctxt = filp->private_data;
>>  	flush_signals(current);
>>  	DRV_RemoveAllResources(pr_ctxt);
>> -	PROC_Detach(pr_ctxt);
>> +	if (pr_ctxt->hProcessor)
>> +		PROC_Detach(pr_ctxt);
>>  	MEM_Free(pr_ctxt);
>>
>>  	filp->private_data = NULL;
>
>I guess PROC_Detach already checks for a valid hProcessor handle!
>Why we need this check here again?
>
Sorry, I think this applied for different version of dspbridge.


>Also if you go ahead with this patch, then how are you going to cover a
>use case where userspace application calls PROC_Attach() which fails,
>but without checking the failure userspace application calls
>PROC_Detach()!
>
PROC_Detach can not be called directly from user space, is deprecated; the only place where this is function is called is bridge_release, so nothing would happen. Anyway, dismiss this patch.

>Cheers,
>Ameya.