linux-kernel - Re: [PATCH v3 1/3] usb: gadget: ccid: add support for USB CCID Gadget Device

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]

Message-ID: <20180530122019.GA28045@kroah.com>
Date:   Wed, 30 May 2018 14:20:19 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Marcus Folkesson <marcus.folkesson@...il.com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Jonathan Corbet <corbet@....net>,
        Felipe Balbi <balbi@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        Ruslan Bilovol <ruslan.bilovol@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Kate Stewart <kstewart@...uxfoundation.org>,
        USB <linux-usb@...r.kernel.org>,
        Linux Documentation List <linux-doc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] usb: gadget: ccid: add support for USB CCID
 Gadget Device

On Wed, May 30, 2018 at 02:13:57PM +0200, Marcus Folkesson wrote:
> Hi Greg,
> 
> On Wed, May 30, 2018 at 01:30:26PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, May 30, 2018 at 01:24:59PM +0200, Marcus Folkesson wrote:
> > > Hi Andy,
> > > 
> > > Thank you for your comments!
> > > Many good catches here!
> > > 
> > > On Wed, May 30, 2018 at 03:55:39AM +0300, Andy Shevchenko wrote:
> > > > On Tue, May 29, 2018 at 9:50 PM, Marcus Folkesson
> > > > <marcus.folkesson@...il.com> wrote:
> > > > > Chip Card Interface Device (CCID) protocol is a USB protocol that
> > > > > allows a smartcard device to be connected to a computer via a card
> > > > > reader using a standard USB interface, without the need for each manufacturer
> > > > > of smartcards to provide its own reader or protocol.
> > > > >
> > > > > This gadget driver makes Linux show up as a CCID device to the host and let a
> > > > > userspace daemon act as the smartcard.
> > > > >
> > > > > This is useful when the Linux gadget itself should act as a cryptographic
> > > > > device or forward APDUs to an embedded smartcard device.
> > > > 
> > > > > + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@...il.com>
> > > > 
> > > > > + *
> > > > 
> > > > Redundant line
> > > > 
> > > 
> > > Yep
> > > 
> > > > > +static DEFINE_IDA(ccidg_ida);
> > > > 
> > > > Where is it destroyed?
> > > 
> > > Hm, I'm not sure it needs to be destroyed. From lib/idr.c:
> > > 
> > >  * You can also use ida_get_new_above() if you need an ID to be allocated
> > >  * above a particular number.  ida_destroy() can be used to dispose of an
> > >  * IDA without needing to free the individual IDs in it.  You can use
> > >  * ida_is_empty() to find out whether the IDA has any IDs currently allocated.
> > > 
> > > 
> > > An empty ccidg_ida is the indication that we should clean up our
> > > mess:
> > > 
> > > static void ccidg_free_inst(struct usb_function_instance *f)
> > > ...
> > > 	if (ida_is_empty(&ccidg_ida))
> > > 		ccidg_cleanup();
> > > 
> > > If the IDA is empty, should I call ida_destroy() anyway?
> > > Other similiar drivers does not seems to do that.
> > > 
> > > I must say that I'm not very familiar with the IDA API.
> > 
> > When your module is removed, you need to clean up any remaining memory
> > that the ida used.  It's not obvious at all, and is a pain as you would
> > think that if you statically allocate one, like you have here, it would
> > not be needed.  You need to just call:
> > 	ida_destroy(&ccidg_ida);
> > in your module exit function.
> > 
> > Hope this helps,
> 
> Thank you for making it clear.
> 
> Maybe I should use
> #define DECLARE_USB_FUNCTION(_name, _inst_alloc, _func_alloc)		\
> 
> instead of 
> #define DECLARE_USB_FUNCTION_INIT(_name, _inst_alloc, _func_alloc)	\
> 
> and provide my own module_init/module_exit functions then?

Probably, yes.