[PATCH] scsi: sni_53c710: Fix a resource leak in an error handling path

[PATCH] scsi: sni_53c710: Fix a resource leak in an error handling path

[Christophe JAILLET]

After a successful 'NCR_700_detect()' call, 'NCR_700_release()' must be
called to release some DMA related resources, as already done in the
remove function.

Fixes: c27d85f3f3c5 ("[SCSI] SNI RM 53c710 driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/scsi/sni_53c710.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/sni_53c710.c b/drivers/scsi/sni_53c710.c
index 678651b9b4dd..f6d60d542207 100644
--- a/drivers/scsi/sni_53c710.c
+++ b/drivers/scsi/sni_53c710.c
@@ -98,6 +98,7 @@ static int snirm710_probe(struct platform_device *dev)
 
  out_put_host:
 	scsi_host_put(host);
+	NCR_700_release(host);
  out_kfree:
 	iounmap(hostdata->base);
 	kfree(hostdata);
-- 
2.30.2

Re: [PATCH] scsi: sni_53c710: Fix a resource leak in an error handling path

[Christophe JAILLET]

Le 20/05/2021 à 06:44, Christophe JAILLET a écrit :
> After a successful 'NCR_700_detect()' call, 'NCR_700_release()' must be
> called to release some DMA related resources, as already done in the
> remove function.
> 
> Fixes: c27d85f3f3c5 ("[SCSI] SNI RM 53c710 driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>   drivers/scsi/sni_53c710.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/sni_53c710.c b/drivers/scsi/sni_53c710.c
> index 678651b9b4dd..f6d60d542207 100644
> --- a/drivers/scsi/sni_53c710.c
> +++ b/drivers/scsi/sni_53c710.c
> @@ -98,6 +98,7 @@ static int snirm710_probe(struct platform_device *dev)
>   
>    out_put_host:
>   	scsi_host_put(host);
> +	NCR_700_release(host);
>    out_kfree:
>   	iounmap(hostdata->base);
>   	kfree(hostdata);
> 
Hi,

please note that this patch is speculative
All the drivers I've look at don't call NCR_700_release in the error 
handling path of the probe. They only do in the remove function.
So it is likely that this patch is wrong and that the truth is elsewhere.

'scsi_host_put()' is used in the probe and 'scsi_remove_host()' in the 
remove function. That maybe is the trick, but I've not been able to see 
how NCR_700_release (or equivalent) was called in this case.

So review with care!

CJ

Re: [PATCH] scsi: sni_53c710: Fix a resource leak in an error handling path

[Dan Carpenter]

On Thu, May 20, 2021 at 06:44:25AM +0200, Christophe JAILLET wrote:
> After a successful 'NCR_700_detect()' call, 'NCR_700_release()' must be
> called to release some DMA related resources, as already done in the
> remove function.
> 
> Fixes: c27d85f3f3c5 ("[SCSI] SNI RM 53c710 driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Good catch.

Reveiwed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH] scsi: sni_53c710: Fix a resource leak in an error handling path

[Thomas Bogendoerfer]

On Thu, May 20, 2021 at 06:44:25AM +0200, Christophe JAILLET wrote:
> After a successful 'NCR_700_detect()' call, 'NCR_700_release()' must be
> called to release some DMA related resources, as already done in the
> remove function.
> 
> Fixes: c27d85f3f3c5 ("[SCSI] SNI RM 53c710 driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/scsi/sni_53c710.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/sni_53c710.c b/drivers/scsi/sni_53c710.c
> index 678651b9b4dd..f6d60d542207 100644
> --- a/drivers/scsi/sni_53c710.c
> +++ b/drivers/scsi/sni_53c710.c
> @@ -98,6 +98,7 @@ static int snirm710_probe(struct platform_device *dev)
>  
>   out_put_host:
>  	scsi_host_put(host);
> +	NCR_700_release(host);

shouldn't this done before the scsi_host_put() to avoid a use after free ?
lasi700.c has the same problem. And it looks like NCR_700_detect() will leak
dma memory, if scsi_host_alloc() failed.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] scsi: sni_53c710: Fix a resource leak in an error handling path

[Christophe JAILLET]

Le 20/05/2021 à 17:06, Thomas Bogendoerfer a écrit :
> On Thu, May 20, 2021 at 06:44:25AM +0200, Christophe JAILLET wrote:
>> After a successful 'NCR_700_detect()' call, 'NCR_700_release()' must be
>> called to release some DMA related resources, as already done in the
>> remove function.
>>
>> Fixes: c27d85f3f3c5 ("[SCSI] SNI RM 53c710 driver")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   drivers/scsi/sni_53c710.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/scsi/sni_53c710.c b/drivers/scsi/sni_53c710.c
>> index 678651b9b4dd..f6d60d542207 100644
>> --- a/drivers/scsi/sni_53c710.c
>> +++ b/drivers/scsi/sni_53c710.c
>> @@ -98,6 +98,7 @@ static int snirm710_probe(struct platform_device *dev)
>>   
>>    out_put_host:
>>   	scsi_host_put(host);
>> +	NCR_700_release(host);
> 
> shouldn't this done before the scsi_host_put() to avoid a use after free ?

I did it this way because remove function are:
    scsi_remove_host
    NCR_700_release

The other reason was to free resources in the reverse order than allocation.
But this logic does'nt work in all cases.

> lasi700.c has the same problem.

and a400t.c and bvme6000_scsi.c and mvme16x_scsi.c and sim710.c and 
zorro7xx.c.
That is to say all drivers that use NCR_700_detect().

That is why I'm unsure with my patch. It is unusual to have ALL users 
wrong. It is more likely that I missed something and that the code is 
correct.

I also don't fully understand why we use 'scsi_host_put' in some place 
and 'scsi_remove_host' in remove functions.
Referenced managed resources sometimes have the needed mechanism to 
automagically release some resource.

> And it looks like NCR_700_detect() will leak
> dma memory, if scsi_host_alloc() failed.

Agreed. And same if scsi_add_host fails at the end of the function.

> 
> Thomas.
>