linux-kernel - Re: [PATCH] staging: vme: fix memory leak in vme_user

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 27 Mar 2014 09:30:10 -0500 (CDT)
From:	Aaron Sierra <asierra@...-inc.com>
To:	DaeSeok Youn <daeseok.youn@...il.com>
Cc:	martyn welch <martyn.welch@...com>,
	manohar vanga <manohar.vanga@...il.com>,
	Greg KH <gregkh@...uxfoundation.org>, lisa@...apiadmin.com,
	yongjun wei <yongjun_wei@...ndmicro.com.cn>,
	devel <devel@...verdev.osuosl.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] staging: vme: fix memory leak in vme_user_probe()


----- Original Message -----
> From: "DaeSeok Youn" <daeseok.youn@...il.com>
> Sent: Wednesday, March 26, 2014 8:47:51 PM
>
> 2014-03-27 3:51 GMT+09:00 Aaron Sierra <asierra@...-inc.com>:
> > ----- Original Message -----
> >> From: "Daeseok Youn" <daeseok.youn@...il.com>
> >> Sent: Tuesday, March 25, 2014 10:01:48 PM
> >> Subject: [PATCH] staging: vme: fix memory leak in vme_user_probe()
> >>
> >>
> >> If vme_master_request() returns NULL when it failed,
> >> it need to free buffers for master.
> >>
> >> And also removes unreachable code in vme_user_probe().
> >>
> >> Signed-off-by: Daeseok Youn <daeseok.youn@...il.com>
> >> ---
> >>  drivers/staging/vme/devices/vme_user.c |    9 +++------
> >>  1 files changed, 3 insertions(+), 6 deletions(-)
> >
> > Nice catches Daeseok. I don't maintain this driver, but I have some
> > suggestions below.
> >
> >>
> >> diff --git a/drivers/staging/vme/devices/vme_user.c
> >> b/drivers/staging/vme/devices/vme_user.c
> >> index 7927927..ffb4eee 100644
> >> --- a/drivers/staging/vme/devices/vme_user.c
> >> +++ b/drivers/staging/vme/devices/vme_user.c
> >> @@ -776,7 +776,8 @@ static int vme_user_probe(struct vme_dev *vdev)
> >>               image[i].kern_buf = kmalloc(image[i].size_buf, GFP_KERNEL);
> >>               if (image[i].kern_buf == NULL) {
> >>                       err = -ENOMEM;
> >> -                     goto err_master_buf;
> >> +                     vme_master_free(image[i].resource);
> >> +                     goto err_master;
> >>               }
> >>       }
> >
> > I think it would be nice to keep all of the cleanup under the err_master
> > label.
> Actually, I changed like "err_slave" doing. When it failed to alloc
> buffer for slave,
> just called vme_slave_free(image[i].slave) and cleanup under the err_slave.

I do like the error path symmetry provided by your patch.

> >
> > That could be done by changing the kern_buf allocation in this part to
> > a devm_kmalloc. Then devm handles the kern_buf freeing entirely.
> I didn't know about devm_kmalloc(), I will check that function. Thanks!
> 
> >
> >>
> >> @@ -819,8 +820,6 @@ static int vme_user_probe(struct vme_dev *vdev)
> >>
> >>       return 0;
> >>
> >> -     /* Ensure counter set correcty to destroy all sysfs devices */
> >> -     i = VME_DEVS;
> >>  err_sysfs:
> >>       while (i > 0) {
> >>               i--;
> >> @@ -830,12 +829,10 @@ err_sysfs:
> >>
> >>       /* Ensure counter set correcty to unalloc all master windows */
> >>       i = MASTER_MAX + 1;
> >> -err_master_buf:
> >> -     for (i = MASTER_MINOR; i < (MASTER_MAX + 1); i++)
> >> -             kfree(image[i].kern_buf);
> >>  err_master:
> >>       while (i > MASTER_MINOR) {
> >>               i--;
> >> +             kfree(image[i].kern_buf);
> >>               vme_master_free(image[i].resource);
> >>       }
> >
> > Using devm_kmalloc as mentioned above, the while loop could be
> > simplified to this:
> >
> > err_master:
> >         while (i >= MASTER_MINOR) {
> >                 vme_master_free(image[i].resource);
> >                 i--;
> >         }
> It would be nice, but when it failed to vme_master_request() and than
> go to err_master,
> image[i].resource must be NULL. So a NULL exception has occurred in
> vme_master_free().
> 
> I think vme_master{slave}_free() need to check NULL and it can be
> possible to change code as your comment.
> please check for me. :-)

You are correct that vme_master_free() assumes that the resource being
freed is not NULL. I should have recognized that case. That could easily
be handled by testing the resource again in the loop, but I see the
benefit of the symmetry of your change.

-Aaron

> 
> >
> > If not moving to devm, this should be safe even though the first
> > kern_buf may be NULL:
> >
> > err_master:
> >         while (i >= MASTER_MINOR) {
> >                 kfree(image[i].kern_buf);
> >                 vme_master_free(image[i].resource);
> >                 i--;
> >         }
> kfree() is ok. But vme_master_free() function has an problem as mentioned
> above.
> 
> Thanks for review.
> Daeseok Youn.
> >
> > -Aaron
> >
> >>
> >> --
> >> 1.7.4.4
> >>
> >>
> >>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/