* Re: [Line6linux-devel] [PATCH] line6: Use kmemdup rather than duplicating its implementation
@ 2012-12-03 16:34 ` Stefan Hajnoczi
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2012-12-03 16:34 UTC (permalink / raw)
To: Laurent Navet
Cc: devel, Greg Kroah-Hartman, line6linux-devel@lists.sourceforge.net,
kernel-janitors, linux-kernel
On Mon, Dec 3, 2012 at 2:20 PM, Laurent Navet <laurent.navet@gmail.com> wrote:
> staging: line6: driver.c
> The semantic patch that makes this output is available
> in scripts/coccinelle/api/memdup.cocci.
>
> Signed-off-by: Laurent Navet <laurent.navet@gmail.com>
> ---
> drivers/staging/line6/driver.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/line6/driver.c b/drivers/staging/line6/driver.c
> index f5c19b2..e1d6241 100644
> --- a/drivers/staging/line6/driver.c
> +++ b/drivers/staging/line6/driver.c
> @@ -331,14 +331,13 @@ int line6_version_request_async(struct usb_line6 *line6)
> char *buffer;
> int retval;
>
> - buffer = kmalloc(sizeof(line6_request_version), GFP_ATOMIC);
> + buffer = kmemdup(line6_request_version,
> + sizeof(line6_request_version), GFP_ATOMIC);
> if (buffer == NULL) {
> dev_err(line6->ifcdev, "Out of memory");
> return -ENOMEM;
> }
>
> - memcpy(buffer, line6_request_version, sizeof(line6_request_version));
> -
> retval = line6_send_raw_message_async(line6, buffer,
> sizeof(line6_request_version));
> kfree(buffer);
> --
> 1.7.10.4
Your change is fine but I'm not sure whether we should allocate memory
in the first place:
line6_send_raw_message_async() returns before the transfer is
complete. It submits one or more URBs but I cannot see a guarantee
that the buffer is no longer needed. It seems unsafe to kfree(buffer)
before the request is complete.
Since we already have const char line6_request_version[] we should
pass it directly without a temporary kmemdup() buffer.
Stefan
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Line6linux-devel] [PATCH] line6: Use kmemdup rather than duplicating its implementation
2012-12-03 16:34 ` Stefan Hajnoczi
@ 2012-12-04 21:22 ` Markus Grabner
-1 siblings, 0 replies; 10+ messages in thread
From: Markus Grabner @ 2012-12-04 21:22 UTC (permalink / raw)
To: line6linux-devel
Cc: Stefan Hajnoczi, Laurent Navet, devel, Greg Kroah-Hartman,
kernel-janitors, linux-kernel
Am Montag, 3. Dezember 2012, 17:34:07 schrieb Stefan Hajnoczi:
> On Mon, Dec 3, 2012 at 2:20 PM, Laurent Navet <laurent.navet@gmail.com>
wrote:
> > staging: line6: driver.c
> >
> > The semantic patch that makes this output is available
> > in scripts/coccinelle/api/memdup.cocci.
> >
> > Signed-off-by: Laurent Navet <laurent.navet@gmail.com>
> > ---
> >
> > drivers/staging/line6/driver.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/line6/driver.c
> > b/drivers/staging/line6/driver.c index f5c19b2..e1d6241 100644
> > --- a/drivers/staging/line6/driver.c
> > +++ b/drivers/staging/line6/driver.c
> > @@ -331,14 +331,13 @@ int line6_version_request_async(struct usb_line6
> > *line6)>
> > char *buffer;
> > int retval;
> >
> > - buffer = kmalloc(sizeof(line6_request_version), GFP_ATOMIC);
> > + buffer = kmemdup(line6_request_version,
> > + sizeof(line6_request_version), GFP_ATOMIC);
> >
> > if (buffer = NULL) {
> >
> > dev_err(line6->ifcdev, "Out of memory");
> > return -ENOMEM;
> >
> > }
> >
> > - memcpy(buffer, line6_request_version,
> > sizeof(line6_request_version)); -
> >
> > retval = line6_send_raw_message_async(line6, buffer,
> >
> > sizeof(line6_request_version
> > ));
> >
> > kfree(buffer);
> >
> > --
> > 1.7.10.4
>
> Your change is fine but I'm not sure whether we should allocate memory
> in the first place:
I can't remember the precise reason for this copy operation, it was related to
which type of memory is allowed for a URB data block, and memory declared with
"static const char[]" at global scope in the driver is not allowed. I just
verified on my system (kernel 3.4.11) that requesting the device's firmware
version doesn't work when passing the line6_request_version pointer directly
(instead of its kmemdup copy), so I think the kmemdup is necessary here. It's
a bit unsatisfactory to make a copy just because the original data is not
accessible for whatever reason, but I don't know of a better solution. Maybe
somebody else can clarify this or propose an alternative method?
Kind regards,
Markus
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Line6linux-devel] [PATCH] line6: Use kmemdup rather than duplicating its implementation
@ 2012-12-04 21:22 ` Markus Grabner
0 siblings, 0 replies; 10+ messages in thread
From: Markus Grabner @ 2012-12-04 21:22 UTC (permalink / raw)
To: line6linux-devel
Cc: Stefan Hajnoczi, Laurent Navet, devel, Greg Kroah-Hartman,
kernel-janitors, linux-kernel
Am Montag, 3. Dezember 2012, 17:34:07 schrieb Stefan Hajnoczi:
> On Mon, Dec 3, 2012 at 2:20 PM, Laurent Navet <laurent.navet@gmail.com>
wrote:
> > staging: line6: driver.c
> >
> > The semantic patch that makes this output is available
> > in scripts/coccinelle/api/memdup.cocci.
> >
> > Signed-off-by: Laurent Navet <laurent.navet@gmail.com>
> > ---
> >
> > drivers/staging/line6/driver.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/line6/driver.c
> > b/drivers/staging/line6/driver.c index f5c19b2..e1d6241 100644
> > --- a/drivers/staging/line6/driver.c
> > +++ b/drivers/staging/line6/driver.c
> > @@ -331,14 +331,13 @@ int line6_version_request_async(struct usb_line6
> > *line6)>
> > char *buffer;
> > int retval;
> >
> > - buffer = kmalloc(sizeof(line6_request_version), GFP_ATOMIC);
> > + buffer = kmemdup(line6_request_version,
> > + sizeof(line6_request_version), GFP_ATOMIC);
> >
> > if (buffer == NULL) {
> >
> > dev_err(line6->ifcdev, "Out of memory");
> > return -ENOMEM;
> >
> > }
> >
> > - memcpy(buffer, line6_request_version,
> > sizeof(line6_request_version)); -
> >
> > retval = line6_send_raw_message_async(line6, buffer,
> >
> > sizeof(line6_request_version
> > ));
> >
> > kfree(buffer);
> >
> > --
> > 1.7.10.4
>
> Your change is fine but I'm not sure whether we should allocate memory
> in the first place:
I can't remember the precise reason for this copy operation, it was related to
which type of memory is allowed for a URB data block, and memory declared with
"static const char[]" at global scope in the driver is not allowed. I just
verified on my system (kernel 3.4.11) that requesting the device's firmware
version doesn't work when passing the line6_request_version pointer directly
(instead of its kmemdup copy), so I think the kmemdup is necessary here. It's
a bit unsatisfactory to make a copy just because the original data is not
accessible for whatever reason, but I don't know of a better solution. Maybe
somebody else can clarify this or propose an alternative method?
Kind regards,
Markus
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Line6linux-devel] [PATCH] line6: Use kmemdup rather than duplicating its implementation
2012-12-04 21:22 ` Markus Grabner
@ 2012-12-04 21:29 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2012-12-04 21:29 UTC (permalink / raw)
To: Markus Grabner
Cc: line6linux-devel, devel, Stefan Hajnoczi, kernel-janitors,
linux-kernel, Laurent Navet
On Tue, Dec 04, 2012 at 10:22:12PM +0100, Markus Grabner wrote:
> Am Montag, 3. Dezember 2012, 17:34:07 schrieb Stefan Hajnoczi:
> > On Mon, Dec 3, 2012 at 2:20 PM, Laurent Navet <laurent.navet@gmail.com>
> wrote:
> > > staging: line6: driver.c
> > >
> > > The semantic patch that makes this output is available
> > > in scripts/coccinelle/api/memdup.cocci.
> > >
> > > Signed-off-by: Laurent Navet <laurent.navet@gmail.com>
> > > ---
> > >
> > > drivers/staging/line6/driver.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/staging/line6/driver.c
> > > b/drivers/staging/line6/driver.c index f5c19b2..e1d6241 100644
> > > --- a/drivers/staging/line6/driver.c
> > > +++ b/drivers/staging/line6/driver.c
> > > @@ -331,14 +331,13 @@ int line6_version_request_async(struct usb_line6
> > > *line6)>
> > > char *buffer;
> > > int retval;
> > >
> > > - buffer = kmalloc(sizeof(line6_request_version), GFP_ATOMIC);
> > > + buffer = kmemdup(line6_request_version,
> > > + sizeof(line6_request_version), GFP_ATOMIC);
> > >
> > > if (buffer = NULL) {
> > >
> > > dev_err(line6->ifcdev, "Out of memory");
> > > return -ENOMEM;
> > >
> > > }
> > >
> > > - memcpy(buffer, line6_request_version,
> > > sizeof(line6_request_version)); -
> > >
> > > retval = line6_send_raw_message_async(line6, buffer,
> > >
> > > sizeof(line6_request_version
> > > ));
> > >
> > > kfree(buffer);
> > >
> > > --
> > > 1.7.10.4
> >
> > Your change is fine but I'm not sure whether we should allocate memory
> > in the first place:
> I can't remember the precise reason for this copy operation, it was related to
> which type of memory is allowed for a URB data block, and memory declared with
> "static const char[]" at global scope in the driver is not allowed. I just
> verified on my system (kernel 3.4.11) that requesting the device's firmware
> version doesn't work when passing the line6_request_version pointer directly
> (instead of its kmemdup copy), so I think the kmemdup is necessary here. It's
> a bit unsatisfactory to make a copy just because the original data is not
> accessible for whatever reason, but I don't know of a better solution. Maybe
> somebody else can clarify this or propose an alternative method?
Yes, all data sent to the USB bus must be dynamically created, so
kmemdup is correct to use here.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Line6linux-devel] [PATCH] line6: Use kmemdup rather than duplicating its implementation
@ 2012-12-04 21:29 ` Greg Kroah-Hartman
0 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2012-12-04 21:29 UTC (permalink / raw)
To: Markus Grabner
Cc: line6linux-devel, devel, Stefan Hajnoczi, kernel-janitors,
linux-kernel, Laurent Navet
On Tue, Dec 04, 2012 at 10:22:12PM +0100, Markus Grabner wrote:
> Am Montag, 3. Dezember 2012, 17:34:07 schrieb Stefan Hajnoczi:
> > On Mon, Dec 3, 2012 at 2:20 PM, Laurent Navet <laurent.navet@gmail.com>
> wrote:
> > > staging: line6: driver.c
> > >
> > > The semantic patch that makes this output is available
> > > in scripts/coccinelle/api/memdup.cocci.
> > >
> > > Signed-off-by: Laurent Navet <laurent.navet@gmail.com>
> > > ---
> > >
> > > drivers/staging/line6/driver.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/staging/line6/driver.c
> > > b/drivers/staging/line6/driver.c index f5c19b2..e1d6241 100644
> > > --- a/drivers/staging/line6/driver.c
> > > +++ b/drivers/staging/line6/driver.c
> > > @@ -331,14 +331,13 @@ int line6_version_request_async(struct usb_line6
> > > *line6)>
> > > char *buffer;
> > > int retval;
> > >
> > > - buffer = kmalloc(sizeof(line6_request_version), GFP_ATOMIC);
> > > + buffer = kmemdup(line6_request_version,
> > > + sizeof(line6_request_version), GFP_ATOMIC);
> > >
> > > if (buffer == NULL) {
> > >
> > > dev_err(line6->ifcdev, "Out of memory");
> > > return -ENOMEM;
> > >
> > > }
> > >
> > > - memcpy(buffer, line6_request_version,
> > > sizeof(line6_request_version)); -
> > >
> > > retval = line6_send_raw_message_async(line6, buffer,
> > >
> > > sizeof(line6_request_version
> > > ));
> > >
> > > kfree(buffer);
> > >
> > > --
> > > 1.7.10.4
> >
> > Your change is fine but I'm not sure whether we should allocate memory
> > in the first place:
> I can't remember the precise reason for this copy operation, it was related to
> which type of memory is allowed for a URB data block, and memory declared with
> "static const char[]" at global scope in the driver is not allowed. I just
> verified on my system (kernel 3.4.11) that requesting the device's firmware
> version doesn't work when passing the line6_request_version pointer directly
> (instead of its kmemdup copy), so I think the kmemdup is necessary here. It's
> a bit unsatisfactory to make a copy just because the original data is not
> accessible for whatever reason, but I don't know of a better solution. Maybe
> somebody else can clarify this or propose an alternative method?
Yes, all data sent to the USB bus must be dynamically created, so
kmemdup is correct to use here.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Line6linux-devel] [PATCH] line6: Use kmemdup rather than duplicating its implementation
2012-12-03 16:34 ` Stefan Hajnoczi
@ 2012-12-04 22:25 ` Dan Carpenter
-1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2012-12-04 22:25 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Laurent Navet, devel, Greg Kroah-Hartman,
line6linux-devel@lists.sourceforge.net, kernel-janitors,
linux-kernel
On Mon, Dec 03, 2012 at 05:34:07PM +0100, Stefan Hajnoczi wrote:
> On Mon, Dec 3, 2012 at 2:20 PM, Laurent Navet <laurent.navet@gmail.com> wrote:
> > staging: line6: driver.c
> > The semantic patch that makes this output is available
> > in scripts/coccinelle/api/memdup.cocci.
> >
> > Signed-off-by: Laurent Navet <laurent.navet@gmail.com>
> > ---
> > drivers/staging/line6/driver.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/line6/driver.c b/drivers/staging/line6/driver.c
> > index f5c19b2..e1d6241 100644
> > --- a/drivers/staging/line6/driver.c
> > +++ b/drivers/staging/line6/driver.c
> > @@ -331,14 +331,13 @@ int line6_version_request_async(struct usb_line6 *line6)
> > char *buffer;
> > int retval;
> >
> > - buffer = kmalloc(sizeof(line6_request_version), GFP_ATOMIC);
> > + buffer = kmemdup(line6_request_version,
> > + sizeof(line6_request_version), GFP_ATOMIC);
> > if (buffer = NULL) {
> > dev_err(line6->ifcdev, "Out of memory");
> > return -ENOMEM;
> > }
> >
> > - memcpy(buffer, line6_request_version, sizeof(line6_request_version));
> > -
> > retval = line6_send_raw_message_async(line6, buffer,
> > sizeof(line6_request_version));
> > kfree(buffer);
> > --
> > 1.7.10.4
>
> Your change is fine but I'm not sure whether we should allocate memory
> in the first place:
>
> line6_send_raw_message_async() returns before the transfer is
> complete. It submits one or more URBs but I cannot see a guarantee
> that the buffer is no longer needed. It seems unsafe to kfree(buffer)
> before the request is complete.
>
As Greg pointed out we do need to allocate the memory to make DMA
work. But you're right that it is a use after free bug. We should
move the kfree(msg->buffer) to inside line6_async_request_sent().
I can send a fix for this tomorrow or if someone else wants to do it
while I'm sleeping that's fine too. :)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Line6linux-devel] [PATCH] line6: Use kmemdup rather than duplicating its implementation
@ 2012-12-04 22:25 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2012-12-04 22:25 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Laurent Navet, devel, Greg Kroah-Hartman,
line6linux-devel@lists.sourceforge.net, kernel-janitors,
linux-kernel
On Mon, Dec 03, 2012 at 05:34:07PM +0100, Stefan Hajnoczi wrote:
> On Mon, Dec 3, 2012 at 2:20 PM, Laurent Navet <laurent.navet@gmail.com> wrote:
> > staging: line6: driver.c
> > The semantic patch that makes this output is available
> > in scripts/coccinelle/api/memdup.cocci.
> >
> > Signed-off-by: Laurent Navet <laurent.navet@gmail.com>
> > ---
> > drivers/staging/line6/driver.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/line6/driver.c b/drivers/staging/line6/driver.c
> > index f5c19b2..e1d6241 100644
> > --- a/drivers/staging/line6/driver.c
> > +++ b/drivers/staging/line6/driver.c
> > @@ -331,14 +331,13 @@ int line6_version_request_async(struct usb_line6 *line6)
> > char *buffer;
> > int retval;
> >
> > - buffer = kmalloc(sizeof(line6_request_version), GFP_ATOMIC);
> > + buffer = kmemdup(line6_request_version,
> > + sizeof(line6_request_version), GFP_ATOMIC);
> > if (buffer == NULL) {
> > dev_err(line6->ifcdev, "Out of memory");
> > return -ENOMEM;
> > }
> >
> > - memcpy(buffer, line6_request_version, sizeof(line6_request_version));
> > -
> > retval = line6_send_raw_message_async(line6, buffer,
> > sizeof(line6_request_version));
> > kfree(buffer);
> > --
> > 1.7.10.4
>
> Your change is fine but I'm not sure whether we should allocate memory
> in the first place:
>
> line6_send_raw_message_async() returns before the transfer is
> complete. It submits one or more URBs but I cannot see a guarantee
> that the buffer is no longer needed. It seems unsafe to kfree(buffer)
> before the request is complete.
>
As Greg pointed out we do need to allocate the memory to make DMA
work. But you're right that it is a use after free bug. We should
move the kfree(msg->buffer) to inside line6_async_request_sent().
I can send a fix for this tomorrow or if someone else wants to do it
while I'm sleeping that's fine too. :)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread