linux-kernel - Re: [Line6linux-devel] [PATCH] line6: Use kmemdup rather than duplicating its implementation

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:	Tue, 4 Dec 2012 13:29:27 -0800
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Markus Grabner <grabner@....tugraz.at>
Cc:	line6linux-devel@...ts.sourceforge.net, devel@...verdev.osuosl.org,
	Stefan Hajnoczi <stefanha@...il.com>,
	kernel-janitors@...r.kernel.org,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Laurent Navet <laurent.navet@...il.com>
Subject: Re: [Line6linux-devel] [PATCH] line6: Use kmemdup rather than
 duplicating its implementation

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@...il.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@...il.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
--
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/