| Message ID | 5a97774020847f6b63e161197254d15ef1d786ea.1621485792.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: sni_53c710: Fix a resource leak in an error handling path | expand |
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
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
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.
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. >
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);
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(+)