From: Julia Lawall <[email protected]>
The label fail frees dev->buf, but kbuf hasn't yet been stored there at
this point.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
@exists@
local idexpression x;
statement S;
expression E;
expression *ptr != NULL;
@@
x = memdup_user(...);
...
if (IS_ERR(x)) S
... when != x
when != x = E
(
return \(0\|<+...x...+>\|ptr\);
|
*return ...;
)
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
I wonder if it is really correct to free dev->buf at fail in this case.
Because it is freeing the previously allocated value of dev->buf, not the
current one as done in subsequent cases.
drivers/usb/gadget/inode.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
index a01383f..316547a 100644
--- a/drivers/usb/gadget/inode.c
+++ b/drivers/usb/gadget/inode.c
@@ -1870,8 +1870,10 @@ dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
spin_lock_irq (&dev->lock);
value = -EINVAL;
- if (dev->buf)
+ if (dev->buf) {
+ kfree(kbuf);
goto fail;
+ }
dev->buf = kbuf;
/* full or low speed config */
On Thu, 2 Jun 2011, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> The label fail frees dev->buf, but kbuf hasn't yet been stored there at
> this point.
...
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> I wonder if it is really correct to free dev->buf at fail in this case.
> Because it is freeing the previously allocated value of dev->buf, not the
> current one as done in subsequent cases.
I don't think it matters. This check is probably there only for
safety; it should not be possible for dev->buf to be non-NULL at this
point. You ought to be able to remove the entire
if (dev->buf)
goto fail;
statement with no ill effects.
Alan Stern
On Thu, 2 Jun 2011, Alan Stern wrote:
> On Thu, 2 Jun 2011, Julia Lawall wrote:
>
> > From: Julia Lawall <[email protected]>
> >
> > The label fail frees dev->buf, but kbuf hasn't yet been stored there at
> > this point.
>
> ...
>
> > Signed-off-by: Julia Lawall <[email protected]>
> >
> > ---
> > I wonder if it is really correct to free dev->buf at fail in this case.
> > Because it is freeing the previously allocated value of dev->buf, not the
> > current one as done in subsequent cases.
>
> I don't think it matters. This check is probably there only for
> safety; it should not be possible for dev->buf to be non-NULL at this
> point. You ought to be able to remove the entire
>
> if (dev->buf)
> goto fail;
>
> statement with no ill effects.
I can send such a patch if you think it is OK. But I have no way to test
it.
julia
On Thu, 2 Jun 2011, Julia Lawall wrote:
> > I don't think it matters. This check is probably there only for
> > safety; it should not be possible for dev->buf to be non-NULL at this
> > point. You ought to be able to remove the entire
> >
> > if (dev->buf)
> > goto fail;
> >
> > statement with no ill effects.
>
> I can send such a patch if you think it is OK. But I have no way to test
> it.
I don't either, since as far as I know the "if" condition can never be
true. You may be able to verify this just by reading the code. Note
that dev_config is used only as a field in dev_init_operations, which
is used only in the gadgetfs_fill_super routine and therefore only when
a file in this filesystem is opened.
Note also that the only time dev->buf becomes non-NULL is when
dev_config() succeeds, at which point fd->f_op is switched from
dev_init_operations to ep0_io_operations. Therefore dev_config()
cannot be called successfully more than once for the same open file.
Alan Stern