Re: [PATCH] staging: comedi: drivers: prevent memory leak

[Ian Abbott]

On 17/09/2019 07:33, Dan Carpenter wrote:

On Mon, Sep 16, 2019 at 09:41:43PM -0500, Navid Emamdoost wrote:

In das1800_attach, the buffer allocated via kmalloc_array needs to be
released if an error happens.

Signed-off-by: Navid Emamdoost 


Commedit calls ->detach() if the ->attach() fails so this patch would
lead to a double free.  See comedi_device_attach():

drivers/staging/comedi/drivers.c
983  }
984  if (!driv->attach) {
985  /* driver does not support manual configuration */
986  dev_warn(dev->class_dev,
987   "driver '%s' does not support attach using 
comedi_config\n",
988   driv->driver_name);
989  module_put(driv->module);
990  ret = -EIO;
991  goto out;
992  }
993  dev->driver = driv;
994  dev->board_name = dev->board_ptr ? *(const char 
**)dev->board_ptr
995   : dev->driver->driver_name;
996  ret = driv->attach(dev, it);
   ^
997  if (ret >= 0)
998  ret = comedi_device_postconfig(dev);
999  if (ret < 0) {
   1000  comedi_device_detach(dev);
 ^

   1001  module_put(driv->module);
   1002  }
   1003  /* On success, the driver module count has been incremented. */


Yes, everything should be freed properly by comedi_device_detach(). 
From comedi_device_detach(), some of the stuff is freed by 
dev->driver->detach(), and the remainder is freed by 
comedi_device_detach_cleanup().


--
-=( Ian Abbott  || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

Re: [PATCH] staging: comedi: drivers: prevent memory leak

[Dan Carpenter]

On Mon, Sep 16, 2019 at 09:41:43PM -0500, Navid Emamdoost wrote:
> In das1800_attach, the buffer allocated via kmalloc_array needs to be
> released if an error happens.
> 
> Signed-off-by: Navid Emamdoost 

Commedit calls ->detach() if the ->attach() fails so this patch would
lead to a double free.  See comedi_device_attach():

drivers/staging/comedi/drivers.c
   983  }
   984  if (!driv->attach) {
   985  /* driver does not support manual configuration */
   986  dev_warn(dev->class_dev,
   987   "driver '%s' does not support attach using 
comedi_config\n",
   988   driv->driver_name);
   989  module_put(driv->module);
   990  ret = -EIO;
   991  goto out;
   992  }
   993  dev->driver = driv;
   994  dev->board_name = dev->board_ptr ? *(const char 
**)dev->board_ptr
   995   : dev->driver->driver_name;
   996  ret = driv->attach(dev, it);
  ^
   997  if (ret >= 0)
   998  ret = comedi_device_postconfig(dev);
   999  if (ret < 0) {
  1000  comedi_device_detach(dev);
^

  1001  module_put(driv->module);
  1002  }
  1003  /* On success, the driver module count has been incremented. */

regards,
dan carpenter

[PATCH] staging: comedi: drivers: prevent memory leak

[Navid Emamdoost]

In das1800_attach, the buffer allocated via kmalloc_array needs to be
released if an error happens.

Signed-off-by: Navid Emamdoost 
---
 drivers/staging/comedi/drivers/das1800.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers/das1800.c 
b/drivers/staging/comedi/drivers/das1800.c
index f16aa7e9f4f3..5f2d5f7a6229 100644
--- a/drivers/staging/comedi/drivers/das1800.c
+++ b/drivers/staging/comedi/drivers/das1800.c
@@ -1237,12 +1237,16 @@ static int das1800_attach(struct comedi_device *dev,
 
dev->pacer = comedi_8254_init(dev->iobase + DAS1800_COUNTER,
  I8254_OSC_BASE_5MHZ, I8254_IO8, 0);
-   if (!dev->pacer)
+   if (!dev->pacer) {
+   kfree(devpriv->fifo_buf);
return -ENOMEM;
+   }
 
ret = comedi_alloc_subdevices(dev, 4);
-   if (ret)
+   if (ret) {
+   kfree(devpriv->fifo_buf);
return ret;
+   }
 
/*
 * Analog Input subdevice
@@ -1290,8 +1294,10 @@ static int das1800_attach(struct comedi_device *dev,
s->insn_write   = das1800_ao_insn_write;
 
ret = comedi_alloc_subdev_readback(s);
-   if (ret)
+   if (ret) {
+   kfree(devpriv->fifo_buf);
return ret;
+   }
 
/* initialize all channels to 0V */
for (i = 0; i < s->n_chan; i++) {
-- 
2.17.1