* [PATCH] staging: vt6656: Don't leak memory in drivers/staging/vt6656/ioctl.c::private_ioctl()
@ 2012-04-11 22:35 Jesper Juhl
2012-04-11 22:46 ` Xi Wang
0 siblings, 1 reply; 6+ messages in thread
From: Jesper Juhl @ 2012-04-11 22:35 UTC (permalink / raw)
To: linux-kernel
Cc: devel, Xi Wang, Joe Perches, Dan Carpenter, Greg Kroah-Hartman,
Forest Bond
If copy_to_user() fails in the WLAN_CMD_GET_NODE_LIST case of the
switch in drivers/staging/vt6656/ioctl.c::private_ioctl() we'll leak
the memory allocated to 'pNodeList'. Fix that by kfree'ing the memory
in the failure case.
Also remove a pointless cast (to type 'PSNodeList') of a kmalloc()
return value - kmalloc() returns a void pointer that is implicitly
converted, so there is no need for an explicit cast.
Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
drivers/staging/vt6656/ioctl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/vt6656/ioctl.c b/drivers/staging/vt6656/ioctl.c
index 1463d76..d59456c 100644
--- a/drivers/staging/vt6656/ioctl.c
+++ b/drivers/staging/vt6656/ioctl.c
@@ -565,7 +565,7 @@ int private_ioctl(PSDevice pDevice, struct ifreq *rq)
result = -ENOMEM;
break;
}
- pNodeList = (PSNodeList)kmalloc(sizeof(SNodeList) + (sNodeList.uItem * sizeof(SNodeItem)), (int)GFP_ATOMIC);
+ pNodeList = kmalloc(sizeof(SNodeList) + (sNodeList.uItem * sizeof(SNodeItem)), (int)GFP_ATOMIC);
if (pNodeList == NULL) {
result = -ENOMEM;
break;
@@ -601,6 +601,7 @@ int private_ioctl(PSDevice pDevice, struct ifreq *rq)
}
}
if (copy_to_user(pReq->data, pNodeList, sizeof(SNodeList) + (sNodeList.uItem * sizeof(SNodeItem)))) {
+ kfree(pNodeList);
result = -EFAULT;
break;
}
--
1.7.10
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: vt6656: Don't leak memory in drivers/staging/vt6656/ioctl.c::private_ioctl()
2012-04-11 22:35 [PATCH] staging: vt6656: Don't leak memory in drivers/staging/vt6656/ioctl.c::private_ioctl() Jesper Juhl
@ 2012-04-11 22:46 ` Xi Wang
2012-04-11 22:49 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: Xi Wang @ 2012-04-11 22:46 UTC (permalink / raw)
To: Jesper Juhl
Cc: linux-kernel, devel, Joe Perches, Dan Carpenter,
Greg Kroah-Hartman, Forest Bond
On Apr 11, 2012, at 6:35 PM, Jesper Juhl wrote:
> - pNodeList = (PSNodeList)kmalloc(sizeof(SNodeList) + (sNodeList.uItem * sizeof(SNodeItem)), (int)GFP_ATOMIC);
> + pNodeList = kmalloc(sizeof(SNodeList) + (sNodeList.uItem * sizeof(SNodeItem)), (int)GFP_ATOMIC);
Can you also remove casts like "(int)GFP_ATOMIC"?
The parentheses "(sNodeList.uItem * sizeof(SNodeItem))" are also
pointless..
- xi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: vt6656: Don't leak memory in drivers/staging/vt6656/ioctl.c::private_ioctl()
2012-04-11 22:46 ` Xi Wang
@ 2012-04-11 22:49 ` Greg Kroah-Hartman
2012-04-11 23:17 ` Jesper Juhl
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2012-04-11 22:49 UTC (permalink / raw)
To: Xi Wang
Cc: Jesper Juhl, linux-kernel, devel, Joe Perches, Dan Carpenter,
Forest Bond
On Wed, Apr 11, 2012 at 06:46:33PM -0400, Xi Wang wrote:
> On Apr 11, 2012, at 6:35 PM, Jesper Juhl wrote:
>
> > - pNodeList = (PSNodeList)kmalloc(sizeof(SNodeList) + (sNodeList.uItem * sizeof(SNodeItem)), (int)GFP_ATOMIC);
> > + pNodeList = kmalloc(sizeof(SNodeList) + (sNodeList.uItem * sizeof(SNodeItem)), (int)GFP_ATOMIC);
>
> Can you also remove casts like "(int)GFP_ATOMIC"?
>
> The parentheses "(sNodeList.uItem * sizeof(SNodeItem))" are also
> pointless..
One thing at a time, odds are this is an "automated" patch, right
Jesper?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: vt6656: Don't leak memory in drivers/staging/vt6656/ioctl.c::private_ioctl()
2012-04-11 22:49 ` Greg Kroah-Hartman
@ 2012-04-11 23:17 ` Jesper Juhl
2012-04-11 23:27 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: Jesper Juhl @ 2012-04-11 23:17 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Xi Wang, linux-kernel, devel, Joe Perches, Dan Carpenter,
Forest Bond
On Wed, 11 Apr 2012, Greg Kroah-Hartman wrote:
> On Wed, Apr 11, 2012 at 06:46:33PM -0400, Xi Wang wrote:
> > On Apr 11, 2012, at 6:35 PM, Jesper Juhl wrote:
> >
> > > - pNodeList = (PSNodeList)kmalloc(sizeof(SNodeList) + (sNodeList.uItem * sizeof(SNodeItem)), (int)GFP_ATOMIC);
> > > + pNodeList = kmalloc(sizeof(SNodeList) + (sNodeList.uItem * sizeof(SNodeItem)), (int)GFP_ATOMIC);
> >
> > Can you also remove casts like "(int)GFP_ATOMIC"?
> >
> > The parentheses "(sNodeList.uItem * sizeof(SNodeItem))" are also
> > pointless..
>
> One thing at a time, odds are this is an "automated" patch, right
> Jesper?
>
I'm not quite sure what you mean by "automated patch" Greg. Manually
editing the file in emacs, then running "git format-patch", followed by
importing the patch file into alpine and sending it off surely didn't
feel "automated" ;-)
The only reason I even included the change of the return value cast was
that it was the variable I was focusing on with regards to fixing the
leak, so it felt "sufficiently related" to include in the same patch.
I certainly was not on a mission to clean up the file in general - just
fixing the leak was all that was really on the agenda.
I don't *mind* fixing up other issues in the file - sure, I can do that,
but that's an entirely different set of patches then...
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: vt6656: Don't leak memory in drivers/staging/vt6656/ioctl.c::private_ioctl()
2012-04-11 23:17 ` Jesper Juhl
@ 2012-04-11 23:27 ` Greg Kroah-Hartman
2012-04-11 23:37 ` Jesper Juhl
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2012-04-11 23:27 UTC (permalink / raw)
To: Jesper Juhl
Cc: Xi Wang, linux-kernel, devel, Joe Perches, Dan Carpenter,
Forest Bond
On Thu, Apr 12, 2012 at 01:17:52AM +0200, Jesper Juhl wrote:
> On Wed, 11 Apr 2012, Greg Kroah-Hartman wrote:
>
> > On Wed, Apr 11, 2012 at 06:46:33PM -0400, Xi Wang wrote:
> > > On Apr 11, 2012, at 6:35 PM, Jesper Juhl wrote:
> > >
> > > > - pNodeList = (PSNodeList)kmalloc(sizeof(SNodeList) + (sNodeList.uItem * sizeof(SNodeItem)), (int)GFP_ATOMIC);
> > > > + pNodeList = kmalloc(sizeof(SNodeList) + (sNodeList.uItem * sizeof(SNodeItem)), (int)GFP_ATOMIC);
> > >
> > > Can you also remove casts like "(int)GFP_ATOMIC"?
> > >
> > > The parentheses "(sNodeList.uItem * sizeof(SNodeItem))" are also
> > > pointless..
> >
> > One thing at a time, odds are this is an "automated" patch, right
> > Jesper?
> >
>
> I'm not quite sure what you mean by "automated patch" Greg. Manually
> editing the file in emacs, then running "git format-patch", followed by
> importing the patch file into alpine and sending it off surely didn't
> feel "automated" ;-)
Sorry, I thought this was scripted, my apologies.
> The only reason I even included the change of the return value cast was
> that it was the variable I was focusing on with regards to fixing the
> leak, so it felt "sufficiently related" to include in the same patch.
> I certainly was not on a mission to clean up the file in general - just
> fixing the leak was all that was really on the agenda.
> I don't *mind* fixing up other issues in the file - sure, I can do that,
> but that's an entirely different set of patches then...
I agree, this patch is fine as-is, thanks.
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: vt6656: Don't leak memory in drivers/staging/vt6656/ioctl.c::private_ioctl()
2012-04-11 23:27 ` Greg Kroah-Hartman
@ 2012-04-11 23:37 ` Jesper Juhl
0 siblings, 0 replies; 6+ messages in thread
From: Jesper Juhl @ 2012-04-11 23:37 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Xi Wang, linux-kernel, devel, Joe Perches, Dan Carpenter,
Forest Bond
On Wed, 11 Apr 2012, Greg Kroah-Hartman wrote:
> On Thu, Apr 12, 2012 at 01:17:52AM +0200, Jesper Juhl wrote:
> > On Wed, 11 Apr 2012, Greg Kroah-Hartman wrote:
> >
> > > On Wed, Apr 11, 2012 at 06:46:33PM -0400, Xi Wang wrote:
> > > > On Apr 11, 2012, at 6:35 PM, Jesper Juhl wrote:
> > > >
> > > > > - pNodeList = (PSNodeList)kmalloc(sizeof(SNodeList) + (sNodeList.uItem * sizeof(SNodeItem)), (int)GFP_ATOMIC);
> > > > > + pNodeList = kmalloc(sizeof(SNodeList) + (sNodeList.uItem * sizeof(SNodeItem)), (int)GFP_ATOMIC);
> > > >
> > > > Can you also remove casts like "(int)GFP_ATOMIC"?
> > > >
> > > > The parentheses "(sNodeList.uItem * sizeof(SNodeItem))" are also
> > > > pointless..
> > >
> > > One thing at a time, odds are this is an "automated" patch, right
> > > Jesper?
> > >
> >
> > I'm not quite sure what you mean by "automated patch" Greg. Manually
> > editing the file in emacs, then running "git format-patch", followed by
> > importing the patch file into alpine and sending it off surely didn't
> > feel "automated" ;-)
>
> Sorry, I thought this was scripted, my apologies.
>
No need for apologies. Not at all.
But I'm currious to learn that some people submit entirely
scripted/automated patches - I didn't expect that... Ohh well, you live
and learn ;-)
> > The only reason I even included the change of the return value cast was
> > that it was the variable I was focusing on with regards to fixing the
> > leak, so it felt "sufficiently related" to include in the same patch.
> > I certainly was not on a mission to clean up the file in general - just
> > fixing the leak was all that was really on the agenda.
> > I don't *mind* fixing up other issues in the file - sure, I can do that,
> > but that's an entirely different set of patches then...
>
> I agree, this patch is fine as-is, thanks.
>
You are welcome :-)
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-04-11 23:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-11 22:35 [PATCH] staging: vt6656: Don't leak memory in drivers/staging/vt6656/ioctl.c::private_ioctl() Jesper Juhl
2012-04-11 22:46 ` Xi Wang
2012-04-11 22:49 ` Greg Kroah-Hartman
2012-04-11 23:17 ` Jesper Juhl
2012-04-11 23:27 ` Greg Kroah-Hartman
2012-04-11 23:37 ` Jesper Juhl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox