Patchwork [2/7] ccid: add passthru card device

login
register
mail settings
Submitter Alon Levy
Date Jan. 11, 2011, 8:42 a.m.
Message ID <1294735359-4009-3-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/78316/
State New
Headers show

Comments

Alon Levy - Jan. 11, 2011, 8:42 a.m.
The passthru ccid card is a device sitting on the usb-ccid bus and
using a chardevice to communicate with a remote device using the
VSCard protocol defined in libcacard/vscard_common.h

Usage docs available in following patch in docs/ccid.txt

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 Makefile.objs             |    2 +-
 hw/ccid-card-passthru.c   |  273 +++++++++++++++++++++++++++++++++++++++++++++
 libcacard/vscard_common.h |  130 +++++++++++++++++++++
 3 files changed, 404 insertions(+), 1 deletions(-)
 create mode 100644 hw/ccid-card-passthru.c
 create mode 100644 libcacard/vscard_common.h
Anthony Liguori - Jan. 25, 2011, 2:17 p.m.
On 01/11/2011 02:42 AM, Alon Levy wrote:
> diff --git a/libcacard/vscard_common.h b/libcacard/vscard_common.h
> new file mode 100644
> index 0000000..9ff1295
> --- /dev/null
> +++ b/libcacard/vscard_common.h
>    

This file (and the .c file) need a coding style pass to fixup comments 
and the use of _ as a prefix but I want to focus on the protocol itself.

First, let's get a written spec into the wiki.  I think it's important 
that all of our compatibility protocols are documented in a more formal 
way such that can be reviewed by a wider audience.

> @@ -0,0 +1,130 @@
> +/* Virtual Smart Card protocol definition
> + *
> + * This protocol is between a host implementing a group of virtual smart card
> + * reader, and a client implementing a virtual smart card, or passthrough to
> + * a real card.
> + *
> + * The current implementation passes the raw APDU's from 7816 and additionally
> + * contains messages to setup and teardown readers, handle insertion and
> + * removal of cards, negotiate the protocol and provide for error responses.
> + *
> + * Copyright (c) 2010 Red Hat.
> + *
> + * This code is licensed under the LGPL.
> + */
> +
> +#ifndef _VSCARD_COMMON_H
> +#define _VSCARD_COMMON_H
> +
> +#include<stdint.h>
> +
> +#define VERSION_MAJOR_BITS 11
> +#define VERSION_MIDDLE_BITS 11
> +#define VERSION_MINOR_BITS 10
>    

Distros make versioning not enough.  Inevitably, someone wants to back 
port a bug fix or a feature for some RHEL7.2 release or something like that.

Feature negotiation has worked pretty well for us and I'd suggest using 
it within the protocol.

> +#define MAKE_VERSION(major, middle, minor) \
> +     (  (major<<  (VERSION_MINOR_BITS + VERSION_MIDDLE_BITS)) \
> +      | (middle<<   VERSION_MINOR_BITS) \
> +      | (minor)  )
> +
> +/** IMPORTANT NOTE on VERSION
> + *
> + * The version below MUST be changed whenever a change in this file is made.
> + *
> + * The last digit, the minor, is for bug fix changes only.
> + *
> + * The middle digit is for backward / forward compatible changes, updates
> + * to the existing messages, addition of fields.
> + *
> + * The major digit is for a breaking change of protocol, presumably
> + * something that cannot be accomodated with the existing protocol.
> + */
> +
> +#define VSCARD_VERSION MAKE_VERSION(0,0,1)
> +
> +typedef enum {
> +    VSC_Init,
> +    VSC_Error,
> +    VSC_ReaderAdd,
> +    VSC_ReaderAddResponse,
> +    VSC_ReaderRemove,
> +    VSC_ATR,
> +    VSC_CardRemove,
> +    VSC_APDU,
> +    VSC_Reconnect
> +} VSCMsgType;
>    

Should number the enum to be specific at least.

> +
> +typedef enum {
> +    VSC_GENERAL_ERROR=1,
> +    VSC_CANNOT_ADD_MORE_READERS,
> +} VSCErrorCode;
> +
> +typedef uint32_t reader_id_t;
>    

This namespace is reserved by C.

> +#define VSCARD_UNDEFINED_READER_ID 0xffffffff
> +#define VSCARD_MINIMAL_READER_ID    0
> +
> +typedef struct VSCMsgHeader {
> +    VSCMsgType type;
> +    reader_id_t   reader_id;
> +    uint32_t   length;
>    

Is length just the data length or the whole message length?

> +    uint8_t    data[0];
> +} VSCMsgHeader;
> +
> +/* VSCMsgInit               Client<->  Host
> + * Host replies with allocated reader id in ReaderAddResponse
> + * */
> +typedef struct VSCMsgInit {
> +    uint32_t   version;
> +} VSCMsgInit;
> +
> +/* VSCMsgError              Client<->  Host
> + * */
> +typedef struct VSCMsgError {
> +    uint32_t   code;
> +} VSCMsgError;
> +
> +/* VSCMsgReaderAdd          Client ->  Host
> + * Host replies with allocated reader id in ReaderAddResponse
> + * name - name of the reader on client side.
> + * */
> +typedef struct VSCMsgReaderAdd {
> +    uint8_t    name[0];
>    

Is this a string?

> +} VSCMsgReaderAdd;
> +
> +/* VSCMsgReaderAddResponse  Host ->  Client
> + * Reply to ReaderAdd
> + * */
> +typedef struct VSCMsgReaderAddResponse {
> +} VSCMsgReaderAddResponse;
> +
> +/* VSCMsgReaderRemove       Client ->  Host
> + * */
> +typedef struct VSCMsgReaderRemove {
> +} VSCMsgReaderRemove;
> +
> +/* VSCMsgATR                Client ->  Host
> + * Answer to reset. Sent for card insertion or card reset.
> + * */
> +typedef struct VSCMsgATR {
> +    uint8_t     atr[0];
> +} VSCMsgATR;
> +
> +/* VSCMsgCardRemove         Client ->  Host
> + * */
> +typedef struct VSCMsgCardRemove {
> +} VSCMsgCardRemove;
> +
> +/* VSCMsgAPDU               Client<->  Host
> + * */
> +typedef struct VSCMsgAPDU {
> +    uint8_t    data[0];
> +} VSCMsgAPDU;
> +
> +/* VSCMsgReconnect          Host ->  Client
> + * */
> +typedef struct VSCMsgReconnect {
> +    uint32_t   ip;
>    

This is not ipv6 friendly.  Two strings would be a better choice.

Regards,

Anthony Liguori

> +    uint16_t   port;
> +} VSCMsgReconnect;
> +
> +#endif
>
Alon Levy - Jan. 25, 2011, 4:21 p.m.
On Tue, Jan 25, 2011 at 08:17:32AM -0600, Anthony Liguori wrote:
> On 01/11/2011 02:42 AM, Alon Levy wrote:
> >diff --git a/libcacard/vscard_common.h b/libcacard/vscard_common.h
> >new file mode 100644
> >index 0000000..9ff1295
> >--- /dev/null
> >+++ b/libcacard/vscard_common.h
> 
> This file (and the .c file) need a coding style pass to fixup
> comments and the use of _ as a prefix but I want to focus on the
> protocol itself.
> 
> First, let's get a written spec into the wiki.  I think it's
> important that all of our compatibility protocols are documented in
> a more formal way such that can be reviewed by a wider audience.

ok, I'll create Features/Smartcard/Protocol

> 
> >@@ -0,0 +1,130 @@
> >+/* Virtual Smart Card protocol definition
> >+ *
> >+ * This protocol is between a host implementing a group of virtual smart card
> >+ * reader, and a client implementing a virtual smart card, or passthrough to
> >+ * a real card.
> >+ *
> >+ * The current implementation passes the raw APDU's from 7816 and additionally
> >+ * contains messages to setup and teardown readers, handle insertion and
> >+ * removal of cards, negotiate the protocol and provide for error responses.
> >+ *
> >+ * Copyright (c) 2010 Red Hat.
> >+ *
> >+ * This code is licensed under the LGPL.
> >+ */
> >+
> >+#ifndef _VSCARD_COMMON_H
> >+#define _VSCARD_COMMON_H
> >+
> >+#include<stdint.h>
> >+
> >+#define VERSION_MAJOR_BITS 11
> >+#define VERSION_MIDDLE_BITS 11
> >+#define VERSION_MINOR_BITS 10
> 
> Distros make versioning not enough.  Inevitably, someone wants to
> back port a bug fix or a feature for some RHEL7.2 release or
> something like that.
> 
> Feature negotiation has worked pretty well for us and I'd suggest
> using it within the protocol.
> 

Suggestion accepted.

> >+#define MAKE_VERSION(major, middle, minor) \
> >+     (  (major<<  (VERSION_MINOR_BITS + VERSION_MIDDLE_BITS)) \
> >+      | (middle<<   VERSION_MINOR_BITS) \
> >+      | (minor)  )
> >+
> >+/** IMPORTANT NOTE on VERSION
> >+ *
> >+ * The version below MUST be changed whenever a change in this file is made.
> >+ *
> >+ * The last digit, the minor, is for bug fix changes only.
> >+ *
> >+ * The middle digit is for backward / forward compatible changes, updates
> >+ * to the existing messages, addition of fields.
> >+ *
> >+ * The major digit is for a breaking change of protocol, presumably
> >+ * something that cannot be accomodated with the existing protocol.
> >+ */
> >+
> >+#define VSCARD_VERSION MAKE_VERSION(0,0,1)
> >+
> >+typedef enum {
> >+    VSC_Init,
> >+    VSC_Error,
> >+    VSC_ReaderAdd,
> >+    VSC_ReaderAddResponse,
> >+    VSC_ReaderRemove,
> >+    VSC_ATR,
> >+    VSC_CardRemove,
> >+    VSC_APDU,
> >+    VSC_Reconnect
> >+} VSCMsgType;
> 
> Should number the enum to be specific at least.

will fix.

> 
> >+
> >+typedef enum {
> >+    VSC_GENERAL_ERROR=1,
> >+    VSC_CANNOT_ADD_MORE_READERS,
> >+} VSCErrorCode;
> >+
> >+typedef uint32_t reader_id_t;
> 
> This namespace is reserved by C.

reader_id_t is reserved?

> 
> >+#define VSCARD_UNDEFINED_READER_ID 0xffffffff
> >+#define VSCARD_MINIMAL_READER_ID    0
> >+
> >+typedef struct VSCMsgHeader {
> >+    VSCMsgType type;
> >+    reader_id_t   reader_id;
> >+    uint32_t   length;
> 
> Is length just the data length or the whole message length?
> 

data length, I'll add a comment.

> >+    uint8_t    data[0];
> >+} VSCMsgHeader;
> >+
> >+/* VSCMsgInit               Client<->  Host
> >+ * Host replies with allocated reader id in ReaderAddResponse
> >+ * */
> >+typedef struct VSCMsgInit {
> >+    uint32_t   version;
> >+} VSCMsgInit;
> >+
> >+/* VSCMsgError              Client<->  Host
> >+ * */
> >+typedef struct VSCMsgError {
> >+    uint32_t   code;
> >+} VSCMsgError;
> >+
> >+/* VSCMsgReaderAdd          Client ->  Host
> >+ * Host replies with allocated reader id in ReaderAddResponse
> >+ * name - name of the reader on client side.
> >+ * */
> >+typedef struct VSCMsgReaderAdd {
> >+    uint8_t    name[0];
> 
> Is this a string?
> 

Yes. You expect char?

> >+} VSCMsgReaderAdd;
> >+
> >+/* VSCMsgReaderAddResponse  Host ->  Client
> >+ * Reply to ReaderAdd
> >+ * */
> >+typedef struct VSCMsgReaderAddResponse {
> >+} VSCMsgReaderAddResponse;
> >+
> >+/* VSCMsgReaderRemove       Client ->  Host
> >+ * */
> >+typedef struct VSCMsgReaderRemove {
> >+} VSCMsgReaderRemove;
> >+
> >+/* VSCMsgATR                Client ->  Host
> >+ * Answer to reset. Sent for card insertion or card reset.
> >+ * */
> >+typedef struct VSCMsgATR {
> >+    uint8_t     atr[0];
> >+} VSCMsgATR;
> >+
> >+/* VSCMsgCardRemove         Client ->  Host
> >+ * */
> >+typedef struct VSCMsgCardRemove {
> >+} VSCMsgCardRemove;
> >+
> >+/* VSCMsgAPDU               Client<->  Host
> >+ * */
> >+typedef struct VSCMsgAPDU {
> >+    uint8_t    data[0];
> >+} VSCMsgAPDU;
> >+
> >+/* VSCMsgReconnect          Host ->  Client
> >+ * */
> >+typedef struct VSCMsgReconnect {
> >+    uint32_t   ip;
> 
> This is not ipv6 friendly.  Two strings would be a better choice.
> 

Will fix.

> Regards,
> 
> Anthony Liguori
> 
> >+    uint16_t   port;
> >+} VSCMsgReconnect;
> >+
> >+#endif
>
Anthony Liguori - Jan. 25, 2011, 4:24 p.m.
On 01/25/2011 10:21 AM, Alon Levy wrote:
> On Tue, Jan 25, 2011 at 08:17:32AM -0600, Anthony Liguori wrote:
>    
>> On 01/11/2011 02:42 AM, Alon Levy wrote:
>>      
>>> diff --git a/libcacard/vscard_common.h b/libcacard/vscard_common.h
>>> new file mode 100644
>>> index 0000000..9ff1295
>>> --- /dev/null
>>> +++ b/libcacard/vscard_common.h
>>>        
>> This file (and the .c file) need a coding style pass to fixup
>> comments and the use of _ as a prefix but I want to focus on the
>> protocol itself.
>>
>> First, let's get a written spec into the wiki.  I think it's
>> important that all of our compatibility protocols are documented in
>> a more formal way such that can be reviewed by a wider audience.
>>      
> ok, I'll create Features/Smartcard/Protocol
>
>    
>>      
>>> @@ -0,0 +1,130 @@
>>> +/* Virtual Smart Card protocol definition
>>> + *
>>> + * This protocol is between a host implementing a group of virtual smart card
>>> + * reader, and a client implementing a virtual smart card, or passthrough to
>>> + * a real card.
>>> + *
>>> + * The current implementation passes the raw APDU's from 7816 and additionally
>>> + * contains messages to setup and teardown readers, handle insertion and
>>> + * removal of cards, negotiate the protocol and provide for error responses.
>>> + *
>>> + * Copyright (c) 2010 Red Hat.
>>> + *
>>> + * This code is licensed under the LGPL.
>>> + */
>>> +
>>> +#ifndef _VSCARD_COMMON_H
>>> +#define _VSCARD_COMMON_H
>>> +
>>> +#include<stdint.h>
>>> +
>>> +#define VERSION_MAJOR_BITS 11
>>> +#define VERSION_MIDDLE_BITS 11
>>> +#define VERSION_MINOR_BITS 10
>>>        
>> Distros make versioning not enough.  Inevitably, someone wants to
>> back port a bug fix or a feature for some RHEL7.2 release or
>> something like that.
>>
>> Feature negotiation has worked pretty well for us and I'd suggest
>> using it within the protocol.
>>
>>      
> Suggestion accepted.
>
>    
>>> +#define MAKE_VERSION(major, middle, minor) \
>>> +     (  (major<<   (VERSION_MINOR_BITS + VERSION_MIDDLE_BITS)) \
>>> +      | (middle<<    VERSION_MINOR_BITS) \
>>> +      | (minor)  )
>>> +
>>> +/** IMPORTANT NOTE on VERSION
>>> + *
>>> + * The version below MUST be changed whenever a change in this file is made.
>>> + *
>>> + * The last digit, the minor, is for bug fix changes only.
>>> + *
>>> + * The middle digit is for backward / forward compatible changes, updates
>>> + * to the existing messages, addition of fields.
>>> + *
>>> + * The major digit is for a breaking change of protocol, presumably
>>> + * something that cannot be accomodated with the existing protocol.
>>> + */
>>> +
>>> +#define VSCARD_VERSION MAKE_VERSION(0,0,1)
>>> +
>>> +typedef enum {
>>> +    VSC_Init,
>>> +    VSC_Error,
>>> +    VSC_ReaderAdd,
>>> +    VSC_ReaderAddResponse,
>>> +    VSC_ReaderRemove,
>>> +    VSC_ATR,
>>> +    VSC_CardRemove,
>>> +    VSC_APDU,
>>> +    VSC_Reconnect
>>> +} VSCMsgType;
>>>        
>> Should number the enum to be specific at least.
>>      
> will fix.
>
>    
>>      
>>> +
>>> +typedef enum {
>>> +    VSC_GENERAL_ERROR=1,
>>> +    VSC_CANNOT_ADD_MORE_READERS,
>>> +} VSCErrorCode;
>>> +
>>> +typedef uint32_t reader_id_t;
>>>        
>> This namespace is reserved by C.
>>      
> reader_id_t is reserved?
>    

Anything with the suffix '_t' is reserved by the standard library.

It's a widely violated rule, but we have run into problems from not 
obeying it.

>>> +/* VSCMsgReaderAdd          Client ->   Host
>>> + * Host replies with allocated reader id in ReaderAddResponse
>>> + * name - name of the reader on client side.
>>> + * */
>>> +typedef struct VSCMsgReaderAdd {
>>> +    uint8_t    name[0];
>>>        
>> Is this a string?
>>
>>      
> Yes. You expect char?
>    

Yes, also, what's the encoding (UTF-8)?

Regards,

Anthony Liguori
Alon Levy - Jan. 25, 2011, 4:50 p.m.
On Tue, Jan 25, 2011 at 10:24:53AM -0600, Anthony Liguori wrote:
> On 01/25/2011 10:21 AM, Alon Levy wrote:
> >On Tue, Jan 25, 2011 at 08:17:32AM -0600, Anthony Liguori wrote:
> >>On 01/11/2011 02:42 AM, Alon Levy wrote:
> >>>diff --git a/libcacard/vscard_common.h b/libcacard/vscard_common.h
> >>>new file mode 100644
> >>>index 0000000..9ff1295
> >>>--- /dev/null
> >>>+++ b/libcacard/vscard_common.h
> >>This file (and the .c file) need a coding style pass to fixup
> >>comments and the use of _ as a prefix but I want to focus on the
> >>protocol itself.
> >>
> >>First, let's get a written spec into the wiki.  I think it's
> >>important that all of our compatibility protocols are documented in
> >>a more formal way such that can be reviewed by a wider audience.
> >ok, I'll create Features/Smartcard/Protocol
> >
> >>>@@ -0,0 +1,130 @@
> >>>+/* Virtual Smart Card protocol definition
> >>>+ *
> >>>+ * This protocol is between a host implementing a group of virtual smart card
> >>>+ * reader, and a client implementing a virtual smart card, or passthrough to
> >>>+ * a real card.
> >>>+ *
> >>>+ * The current implementation passes the raw APDU's from 7816 and additionally
> >>>+ * contains messages to setup and teardown readers, handle insertion and
> >>>+ * removal of cards, negotiate the protocol and provide for error responses.
> >>>+ *
> >>>+ * Copyright (c) 2010 Red Hat.
> >>>+ *
> >>>+ * This code is licensed under the LGPL.
> >>>+ */
> >>>+
> >>>+#ifndef _VSCARD_COMMON_H
> >>>+#define _VSCARD_COMMON_H
> >>>+
> >>>+#include<stdint.h>
> >>>+
> >>>+#define VERSION_MAJOR_BITS 11
> >>>+#define VERSION_MIDDLE_BITS 11
> >>>+#define VERSION_MINOR_BITS 10
> >>Distros make versioning not enough.  Inevitably, someone wants to
> >>back port a bug fix or a feature for some RHEL7.2 release or
> >>something like that.
> >>
> >>Feature negotiation has worked pretty well for us and I'd suggest
> >>using it within the protocol.
> >>
> >Suggestion accepted.
> >
> >>>+#define MAKE_VERSION(major, middle, minor) \
> >>>+     (  (major<<   (VERSION_MINOR_BITS + VERSION_MIDDLE_BITS)) \
> >>>+      | (middle<<    VERSION_MINOR_BITS) \
> >>>+      | (minor)  )
> >>>+
> >>>+/** IMPORTANT NOTE on VERSION
> >>>+ *
> >>>+ * The version below MUST be changed whenever a change in this file is made.
> >>>+ *
> >>>+ * The last digit, the minor, is for bug fix changes only.
> >>>+ *
> >>>+ * The middle digit is for backward / forward compatible changes, updates
> >>>+ * to the existing messages, addition of fields.
> >>>+ *
> >>>+ * The major digit is for a breaking change of protocol, presumably
> >>>+ * something that cannot be accomodated with the existing protocol.
> >>>+ */
> >>>+
> >>>+#define VSCARD_VERSION MAKE_VERSION(0,0,1)
> >>>+
> >>>+typedef enum {
> >>>+    VSC_Init,
> >>>+    VSC_Error,
> >>>+    VSC_ReaderAdd,
> >>>+    VSC_ReaderAddResponse,
> >>>+    VSC_ReaderRemove,
> >>>+    VSC_ATR,
> >>>+    VSC_CardRemove,
> >>>+    VSC_APDU,
> >>>+    VSC_Reconnect
> >>>+} VSCMsgType;
> >>Should number the enum to be specific at least.
> >will fix.
> >
> >>>+
> >>>+typedef enum {
> >>>+    VSC_GENERAL_ERROR=1,
> >>>+    VSC_CANNOT_ADD_MORE_READERS,
> >>>+} VSCErrorCode;
> >>>+
> >>>+typedef uint32_t reader_id_t;
> >>This namespace is reserved by C.
> >reader_id_t is reserved?
> 
> Anything with the suffix '_t' is reserved by the standard library.
> 
> It's a widely violated rule, but we have run into problems from not
> obeying it.

I thought qemu coding style said something explicitly about using _t for
types that alias basic types - this is actually a change I did to comply..

> 
> >>>+/* VSCMsgReaderAdd          Client ->   Host
> >>>+ * Host replies with allocated reader id in ReaderAddResponse
> >>>+ * name - name of the reader on client side.
> >>>+ * */
> >>>+typedef struct VSCMsgReaderAdd {
> >>>+    uint8_t    name[0];
> >>Is this a string?
> >>
> >Yes. You expect char?
> 
> Yes, also, what's the encoding (UTF-8)?

It's not actually printed anywhere right now, so I'd say unspecififed. I'll
document UTF-8.

> 
> Regards,
> 
> Anthony Liguori
>
Alon Levy - Jan. 27, 2011, 9:13 p.m.
On Tue, Jan 25, 2011 at 08:17:32AM -0600, Anthony Liguori wrote:
> On 01/11/2011 02:42 AM, Alon Levy wrote:
> >diff --git a/libcacard/vscard_common.h b/libcacard/vscard_common.h
> >new file mode 100644
> >index 0000000..9ff1295
> >--- /dev/null
> >+++ b/libcacard/vscard_common.h
> 
> This file (and the .c file) need a coding style pass to fixup
> comments and the use of _ as a prefix but I want to focus on the
> protocol itself.
> 
> First, let's get a written spec into the wiki.  I think it's
> important that all of our compatibility protocols are documented in
> a more formal way such that can be reviewed by a wider audience.
> 
> >@@ -0,0 +1,130 @@
> >+/* Virtual Smart Card protocol definition
> >+ *
> >+ * This protocol is between a host implementing a group of virtual smart card
> >+ * reader, and a client implementing a virtual smart card, or passthrough to
> >+ * a real card.
> >+ *
> >+ * The current implementation passes the raw APDU's from 7816 and additionally
> >+ * contains messages to setup and teardown readers, handle insertion and
> >+ * removal of cards, negotiate the protocol and provide for error responses.
> >+ *
> >+ * Copyright (c) 2010 Red Hat.
> >+ *
> >+ * This code is licensed under the LGPL.
> >+ */
> >+
> >+#ifndef _VSCARD_COMMON_H
> >+#define _VSCARD_COMMON_H
> >+
> >+#include<stdint.h>
> >+
> >+#define VERSION_MAJOR_BITS 11
> >+#define VERSION_MIDDLE_BITS 11
> >+#define VERSION_MINOR_BITS 10
> 
> Distros make versioning not enough.  Inevitably, someone wants to
> back port a bug fix or a feature for some RHEL7.2 release or
> something like that.
> 
> Feature negotiation has worked pretty well for us and I'd suggest
> using it within the protocol.
> 
> >+#define MAKE_VERSION(major, middle, minor) \
> >+     (  (major<<  (VERSION_MINOR_BITS + VERSION_MIDDLE_BITS)) \
> >+      | (middle<<   VERSION_MINOR_BITS) \
> >+      | (minor)  )
> >+
> >+/** IMPORTANT NOTE on VERSION
> >+ *
> >+ * The version below MUST be changed whenever a change in this file is made.
> >+ *
> >+ * The last digit, the minor, is for bug fix changes only.
> >+ *
> >+ * The middle digit is for backward / forward compatible changes, updates
> >+ * to the existing messages, addition of fields.
> >+ *
> >+ * The major digit is for a breaking change of protocol, presumably
> >+ * something that cannot be accomodated with the existing protocol.
> >+ */
> >+
> >+#define VSCARD_VERSION MAKE_VERSION(0,0,1)
> >+
> >+typedef enum {
> >+    VSC_Init,
> >+    VSC_Error,
> >+    VSC_ReaderAdd,
> >+    VSC_ReaderAddResponse,
> >+    VSC_ReaderRemove,
> >+    VSC_ATR,
> >+    VSC_CardRemove,
> >+    VSC_APDU,
> >+    VSC_Reconnect
> >+} VSCMsgType;
> 
> Should number the enum to be specific at least.
> 
> >+
> >+typedef enum {
> >+    VSC_GENERAL_ERROR=1,
> >+    VSC_CANNOT_ADD_MORE_READERS,
> >+} VSCErrorCode;
> >+
> >+typedef uint32_t reader_id_t;
> 
> This namespace is reserved by C.
> 
> >+#define VSCARD_UNDEFINED_READER_ID 0xffffffff
> >+#define VSCARD_MINIMAL_READER_ID    0
> >+
> >+typedef struct VSCMsgHeader {
> >+    VSCMsgType type;
> >+    reader_id_t   reader_id;
> >+    uint32_t   length;
> 
> Is length just the data length or the whole message length?
> 

The data length. Is this enough to document?

> >+    uint8_t    data[0];
> >+} VSCMsgHeader;
> >+
> >+/* VSCMsgInit               Client<->  Host
> >+ * Host replies with allocated reader id in ReaderAddResponse
> >+ * */
> >+typedef struct VSCMsgInit {
> >+    uint32_t   version;
> >+} VSCMsgInit;
> >+
> >+/* VSCMsgError              Client<->  Host
> >+ * */
> >+typedef struct VSCMsgError {
> >+    uint32_t   code;
> >+} VSCMsgError;
> >+
> >+/* VSCMsgReaderAdd          Client ->  Host
> >+ * Host replies with allocated reader id in ReaderAddResponse
> >+ * name - name of the reader on client side.
> >+ * */
> >+typedef struct VSCMsgReaderAdd {
> >+    uint8_t    name[0];
> 
> Is this a string?
> 
> >+} VSCMsgReaderAdd;
> >+
> >+/* VSCMsgReaderAddResponse  Host ->  Client
> >+ * Reply to ReaderAdd
> >+ * */
> >+typedef struct VSCMsgReaderAddResponse {
> >+} VSCMsgReaderAddResponse;
> >+
> >+/* VSCMsgReaderRemove       Client ->  Host
> >+ * */
> >+typedef struct VSCMsgReaderRemove {
> >+} VSCMsgReaderRemove;
> >+
> >+/* VSCMsgATR                Client ->  Host
> >+ * Answer to reset. Sent for card insertion or card reset.
> >+ * */
> >+typedef struct VSCMsgATR {
> >+    uint8_t     atr[0];
> >+} VSCMsgATR;
> >+
> >+/* VSCMsgCardRemove         Client ->  Host
> >+ * */
> >+typedef struct VSCMsgCardRemove {
> >+} VSCMsgCardRemove;
> >+
> >+/* VSCMsgAPDU               Client<->  Host
> >+ * */
> >+typedef struct VSCMsgAPDU {
> >+    uint8_t    data[0];
> >+} VSCMsgAPDU;
> >+
> >+/* VSCMsgReconnect          Host ->  Client
> >+ * */
> >+typedef struct VSCMsgReconnect {
> >+    uint32_t   ip;
> 
> This is not ipv6 friendly.  Two strings would be a better choice.

A string for host makes sense, why for port? isn't a 32 bit port enough?

> 
> Regards,
> 
> Anthony Liguori
> 
> >+    uint16_t   port;
> >+} VSCMsgReconnect;
> >+
> >+#endif
>
Anthony Liguori - Jan. 27, 2011, 9:42 p.m.
On 01/27/2011 03:13 PM, Alon Levy wrote:
>> This is not ipv6 friendly.  Two strings would be a better choice.
>>      
> A string for host makes sense, why for port? isn't a 32 bit port enough?
>    

For an protocol, an integer is probably fine.  For an API, a string is 
nice to allow service names too.

Regards,

Anthony Liguori

>> Regards,
>>
>> Anthony Liguori
>>
>>      
>>> +    uint16_t   port;
>>> +} VSCMsgReconnect;
>>> +
>>> +#endif
>>>        
>>
Alon Levy - Jan. 30, 2011, 5:35 p.m.
On Tue, Jan 25, 2011 at 08:17:32AM -0600, Anthony Liguori wrote:
> On 01/11/2011 02:42 AM, Alon Levy wrote:
> >diff --git a/libcacard/vscard_common.h b/libcacard/vscard_common.h
> >new file mode 100644
> >index 0000000..9ff1295
> >--- /dev/null
> >+++ b/libcacard/vscard_common.h
> 
> This file (and the .c file) need a coding style pass to fixup
> comments and the use of _ as a prefix but I want to focus on the
> protocol itself.
> 
> First, let's get a written spec into the wiki.  I think it's
> important that all of our compatibility protocols are documented in
> a more formal way such that can be reviewed by a wider audience.

http://wiki.qeum.org/Features/Smartcard

I'm still working on the rest, but you can review and comment on it. I've
done a number of changes from the submitted here. I guess the idea is that
iterations on the wiki can be faster? The changes done to the protocol:

 Removed Reconnect - doesn't scale easily, the same work should be done
  by whomever is initiating the migration, or via other mechanisms (i.e.
  spice)
 Added Flush/FlushComplete - still need to be able to tell client to wrap
  up the outstanding operations in any way. I'm planning on implementing
  this using register_savevm_live.
 Fixes suggested by you - set the enum, removed _ from surrounding #ifdef
  (btw - why does no one use #pragma once? IIUC it's supported by gcc?)

The major issue I haven't tackled yet is the thread removal in ccid-card-emulated.c
is that a blocker for integration? Can it be tackled later?

Alon

[snip]

Patch

diff --git a/Makefile.objs b/Makefile.objs
index 7da4771..274db5e 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -197,7 +197,7 @@  hw-obj-$(CONFIG_FDC) += fdc.o
 hw-obj-$(CONFIG_ACPI) += acpi.o acpi_piix4.o
 hw-obj-$(CONFIG_APM) += pm_smbus.o apm.o
 hw-obj-$(CONFIG_DMA) += dma.o
-hw-obj-$(CONFIG_SMARTCARD) += usb-ccid.o
+hw-obj-$(CONFIG_SMARTCARD) += usb-ccid.o ccid-card-passthru.o
 
 # PPC devices
 hw-obj-$(CONFIG_OPENPIC) += openpic.o
diff --git a/hw/ccid-card-passthru.c b/hw/ccid-card-passthru.c
new file mode 100644
index 0000000..6ec4f21
--- /dev/null
+++ b/hw/ccid-card-passthru.c
@@ -0,0 +1,273 @@ 
+/*
+ * CCID Card Device emulation
+ *
+ * Copyright (c) 2010 Red Hat.
+ * Written by Alon Levy.
+ *
+ * This code is licenced under the LGPL.
+ */
+
+#include "qemu-char.h"
+#include "monitor.h"
+#include "hw/ccid.h"
+#include "libcacard/vscard_common.h"
+
+#define DPRINTF(card, lvl, fmt, ...) \
+do { if (lvl <= card->debug) { printf("ccid-card: " fmt , ## __VA_ARGS__); } } while (0)
+
+/* Passthru card */
+
+
+// TODO: do we still need this?
+uint8_t DEFAULT_ATR[] = {
+/* From some example somewhere
+ 0x3B, 0xB0, 0x18, 0x00, 0xD1, 0x81, 0x05, 0xB1, 0x40, 0x38, 0x1F, 0x03, 0x28
+ */
+
+/* From an Athena smart card */
+ 0x3B, 0xD5, 0x18, 0xFF, 0x80, 0x91, 0xFE, 0x1F, 0xC3, 0x80, 0x73, 0xC8, 0x21, 0x13, 0x08
+
+}; /* maximum size of ATR - from 7816-3 */
+
+
+#define PASSTHRU_DEV_NAME "ccid-card-passthru"
+#define VSCARD_IN_SIZE 65536
+#define MAX_ATR_SIZE        40
+
+typedef struct PassthruState PassthruState;
+
+struct PassthruState {
+    CCIDCardState base;
+    CharDriverState *cs;
+    uint8_t  vscard_in_data[VSCARD_IN_SIZE];
+    uint32_t vscard_in_pos;
+    uint32_t vscard_in_hdr;
+    uint8_t  atr[MAX_ATR_SIZE];
+    uint8_t  atr_length;
+    uint8_t debug;
+};
+
+/* VSCard protocol over chardev
+ * This code should not depend on the card type.
+ * */
+
+static void ccid_card_vscard_send_msg(
+    PassthruState *s, VSCMsgType type, reader_id_t reader_id,
+        const uint8_t* payload, uint32_t length)
+{
+    VSCMsgHeader scr_msg_header;
+
+    scr_msg_header.type = type;
+    scr_msg_header.reader_id = reader_id;
+    scr_msg_header.length = length;
+    qemu_chr_write(s->cs, (uint8_t*)&scr_msg_header, sizeof(VSCMsgHeader));
+    qemu_chr_write(s->cs, payload, length);
+}
+
+static void ccid_card_vscard_send_apdu(
+    PassthruState *s, const uint8_t* apdu, uint32_t length)
+{
+    ccid_card_vscard_send_msg(s, VSC_APDU, VSCARD_MINIMAL_READER_ID, apdu, length);
+}
+
+static void ccid_card_vscard_send_error(
+    PassthruState *s, reader_id_t reader_id, VSCErrorCode code)
+{
+    VSCMsgError msg = {.code=code};
+
+    ccid_card_vscard_send_msg(s, VSC_Error, reader_id, (uint8_t*)&msg, sizeof(msg));
+}
+
+static void ccid_card_vscard_send_init(PassthruState *s)
+{
+    VSCMsgInit msg = {.version=VSCARD_VERSION};
+
+    ccid_card_vscard_send_msg(s, VSC_Init, VSCARD_UNDEFINED_READER_ID,
+                         (uint8_t*)&msg, sizeof(msg));
+}
+
+static int ccid_card_vscard_can_read(void *opaque)
+{
+    return 65535;
+}
+
+static void ccid_card_vscard_handle_message(PassthruState *card,
+    VSCMsgHeader* scr_msg_header)
+{
+    uint8_t *data = (uint8_t*)&scr_msg_header[1];
+
+    switch (scr_msg_header->type) {
+        case VSC_ATR:
+            DPRINTF(card, 1, "VSC_ATR %d\n", scr_msg_header->length);
+            assert(scr_msg_header->length <= MAX_ATR_SIZE);
+            memcpy(card->atr, data, scr_msg_header->length);
+            card->atr_length = scr_msg_header->length;
+            ccid_card_card_inserted(&card->base);
+            break;
+        case VSC_APDU:
+            ccid_card_send_apdu_to_guest(&card->base, data, scr_msg_header->length);
+            break;
+        case VSC_CardRemove:
+            DPRINTF(card, 1, "VSC_CardRemove\n");
+            ccid_card_card_removed(&card->base);
+            break;
+        case VSC_Init:
+            break;
+        case VSC_Error:
+            ccid_card_card_error(&card->base, *(uint64_t*)data);
+            break;
+        case VSC_ReaderAdd:
+            if (ccid_card_ccid_attach(&card->base) < 0) {
+                ccid_card_vscard_send_error(card, VSCARD_UNDEFINED_READER_ID,
+                                          VSC_CANNOT_ADD_MORE_READERS);
+            } else {
+                ccid_card_vscard_send_msg(card, VSC_ReaderAddResponse,
+                                             VSCARD_MINIMAL_READER_ID, NULL, 0);
+            }
+            break;
+        case VSC_ReaderRemove:
+            ccid_card_ccid_detach(&card->base);
+            break;
+        default:
+            printf("usb-ccid: chardev: unexpected message of type %X\n",
+                   scr_msg_header->type);
+            ccid_card_vscard_send_error(card, scr_msg_header->reader_id,
+                VSC_GENERAL_ERROR);
+    }
+}
+
+static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size)
+{
+    PassthruState *card = opaque;
+    VSCMsgHeader *hdr;
+
+    assert(card->vscard_in_pos + size <= VSCARD_IN_SIZE);
+    memcpy(card->vscard_in_data + card->vscard_in_pos, buf, size);
+    card->vscard_in_pos += size;
+    hdr = (VSCMsgHeader*)(card->vscard_in_data + card->vscard_in_hdr);
+
+    while ((card->vscard_in_pos - card->vscard_in_hdr >= sizeof(VSCMsgHeader)) &&
+           (card->vscard_in_pos - card->vscard_in_hdr - sizeof(VSCMsgHeader) >=
+           hdr->length)) {
+        ccid_card_vscard_handle_message(card, hdr);
+        card->vscard_in_hdr += hdr->length + sizeof(VSCMsgHeader);
+        hdr = (VSCMsgHeader*)(card->vscard_in_data + card->vscard_in_hdr);
+    }
+    if (card->vscard_in_hdr == card->vscard_in_pos) {
+        card->vscard_in_pos = card->vscard_in_hdr = 0;
+    }
+}
+
+static void ccid_card_vscard_event(void *opaque, int event)
+{
+    PassthruState *card = opaque;
+
+    switch (event) {
+        case CHR_EVENT_BREAK:
+            break;
+        case CHR_EVENT_FOCUS:
+            break;
+        case CHR_EVENT_OPENED:
+            DPRINTF(card, 1, "%s: CHR_EVENT_OPENED\n", __func__);
+            break;
+    }
+}
+
+/* End VSCard handling */
+
+static void passthru_apdu_from_guest(CCIDCardState *base, const uint8_t *apdu, uint32_t len)
+{
+    PassthruState *card = DO_UPCAST(PassthruState, base, base);
+
+    if (!card->cs) {
+        printf("ccid-passthru: no chardev, discarding apdu length %d\n", len);
+        return;
+    }
+    ccid_card_vscard_send_apdu(card, apdu, len);
+}
+
+static const uint8_t* passthru_get_atr(CCIDCardState *base, uint32_t *len)
+{
+    PassthruState *card = DO_UPCAST(PassthruState, base, base);
+
+    *len = card->atr_length;
+    return card->atr;
+}
+
+static int passthru_initfn(CCIDCardState *base)
+{
+    PassthruState *card = DO_UPCAST(PassthruState, base, base);
+
+    card->vscard_in_pos = 0;
+    card->vscard_in_hdr = 0;
+    if (card->cs) {
+        DPRINTF(card, 1, "initing chardev\n");
+        qemu_chr_add_handlers(card->cs,
+            ccid_card_vscard_can_read,
+            ccid_card_vscard_read,
+            ccid_card_vscard_event, card);
+        ccid_card_vscard_send_init(card);
+    }
+    assert(sizeof(DEFAULT_ATR) <= MAX_ATR_SIZE);
+    memcpy(card->atr, DEFAULT_ATR, sizeof(DEFAULT_ATR));
+    card->atr_length = sizeof(DEFAULT_ATR);
+    return 0;
+}
+
+static int passthru_exitfn(CCIDCardState *base)
+{
+    return 0;
+}
+
+static void passthru_pre_save(void *opaque)
+{
+    PassthruState *card = opaque;
+    VSCMsgReconnect reconnect;
+
+    reconnect.ip = 0; // TODO - does the bus keep the target ip? s->migration_target_ip;
+    reconnect.port = 0; // TODO - does the bus keep the target ip? s->migration_target_port;
+    ccid_card_vscard_send_msg(card, VSC_Reconnect, VSCARD_UNDEFINED_READER_ID,
+                         (uint8_t*)&reconnect, sizeof(reconnect));
+}
+
+static VMStateDescription passthru_vmstate = {
+    .name = PASSTHRU_DEV_NAME,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .pre_save = passthru_pre_save,
+    .fields = (VMStateField []) {
+        VMSTATE_BUFFER(vscard_in_data, PassthruState),
+        VMSTATE_UINT32(vscard_in_pos, PassthruState),
+        VMSTATE_UINT32(vscard_in_hdr, PassthruState),
+        VMSTATE_BUFFER(atr, PassthruState),
+        VMSTATE_UINT8(atr_length, PassthruState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static CCIDCardInfo passthru_card_info = {
+    .qdev.name = PASSTHRU_DEV_NAME,
+    .qdev.size = sizeof(PassthruState),
+    .qdev.vmsd = &passthru_vmstate,
+    .initfn = passthru_initfn,
+    .exitfn = passthru_exitfn,
+    .get_atr = passthru_get_atr,
+    .apdu_from_guest = passthru_apdu_from_guest,
+    .qdev.unplug    = qdev_simple_unplug_cb,
+    .qdev.props     = (Property[]) {
+        DEFINE_PROP_CHR("chardev", PassthruState, cs),
+        DEFINE_PROP_UINT8("debug", PassthruState, debug, 0),
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static void ccid_card_passthru_register_devices(void)
+{
+    ccid_card_qdev_register(&passthru_card_info);
+    // TODO: passthru local card (or: just a case of passthru with no chardev
+    //  given and instead some other arguments that would be required for local
+    //  card anyway and can be shared with the emulated local card)
+    // TODO: emulated local card
+}
+
+device_init(ccid_card_passthru_register_devices)
diff --git a/libcacard/vscard_common.h b/libcacard/vscard_common.h
new file mode 100644
index 0000000..9ff1295
--- /dev/null
+++ b/libcacard/vscard_common.h
@@ -0,0 +1,130 @@ 
+/* Virtual Smart Card protocol definition
+ *
+ * This protocol is between a host implementing a group of virtual smart card
+ * reader, and a client implementing a virtual smart card, or passthrough to
+ * a real card.
+ *
+ * The current implementation passes the raw APDU's from 7816 and additionally
+ * contains messages to setup and teardown readers, handle insertion and
+ * removal of cards, negotiate the protocol and provide for error responses.
+ *
+ * Copyright (c) 2010 Red Hat.
+ *
+ * This code is licensed under the LGPL.
+ */
+
+#ifndef _VSCARD_COMMON_H
+#define _VSCARD_COMMON_H
+
+#include <stdint.h>
+
+#define VERSION_MAJOR_BITS 11
+#define VERSION_MIDDLE_BITS 11
+#define VERSION_MINOR_BITS 10
+
+#define MAKE_VERSION(major, middle, minor) \
+     (  (major  << (VERSION_MINOR_BITS + VERSION_MIDDLE_BITS)) \
+      | (middle <<  VERSION_MINOR_BITS) \
+      | (minor)  )
+
+/** IMPORTANT NOTE on VERSION
+ *
+ * The version below MUST be changed whenever a change in this file is made.
+ *
+ * The last digit, the minor, is for bug fix changes only.
+ *
+ * The middle digit is for backward / forward compatible changes, updates
+ * to the existing messages, addition of fields.
+ *
+ * The major digit is for a breaking change of protocol, presumably
+ * something that cannot be accomodated with the existing protocol.
+ */
+
+#define VSCARD_VERSION MAKE_VERSION(0,0,1)
+
+typedef enum {
+    VSC_Init,
+    VSC_Error,
+    VSC_ReaderAdd,
+    VSC_ReaderAddResponse,
+    VSC_ReaderRemove,
+    VSC_ATR,
+    VSC_CardRemove,
+    VSC_APDU,
+    VSC_Reconnect
+} VSCMsgType;
+
+typedef enum {
+    VSC_GENERAL_ERROR=1,
+    VSC_CANNOT_ADD_MORE_READERS,
+} VSCErrorCode;
+
+typedef uint32_t reader_id_t;
+#define VSCARD_UNDEFINED_READER_ID 0xffffffff
+#define VSCARD_MINIMAL_READER_ID    0
+
+typedef struct VSCMsgHeader {
+    VSCMsgType type;
+    reader_id_t   reader_id;
+    uint32_t   length;
+    uint8_t    data[0];
+} VSCMsgHeader;
+
+/* VSCMsgInit               Client <-> Host
+ * Host replies with allocated reader id in ReaderAddResponse
+ * */
+typedef struct VSCMsgInit {
+    uint32_t   version;
+} VSCMsgInit;
+
+/* VSCMsgError              Client <-> Host
+ * */
+typedef struct VSCMsgError {
+    uint32_t   code;
+} VSCMsgError;
+
+/* VSCMsgReaderAdd          Client -> Host
+ * Host replies with allocated reader id in ReaderAddResponse
+ * name - name of the reader on client side.
+ * */
+typedef struct VSCMsgReaderAdd {
+    uint8_t    name[0];
+} VSCMsgReaderAdd;
+
+/* VSCMsgReaderAddResponse  Host -> Client
+ * Reply to ReaderAdd
+ * */
+typedef struct VSCMsgReaderAddResponse {
+} VSCMsgReaderAddResponse;
+
+/* VSCMsgReaderRemove       Client -> Host
+ * */
+typedef struct VSCMsgReaderRemove {
+} VSCMsgReaderRemove;
+
+/* VSCMsgATR                Client -> Host
+ * Answer to reset. Sent for card insertion or card reset.
+ * */
+typedef struct VSCMsgATR {
+    uint8_t     atr[0];
+} VSCMsgATR;
+
+/* VSCMsgCardRemove         Client -> Host
+ * */
+typedef struct VSCMsgCardRemove {
+} VSCMsgCardRemove;
+
+/* VSCMsgAPDU               Client <-> Host
+ * */
+typedef struct VSCMsgAPDU {
+    uint8_t    data[0];
+} VSCMsgAPDU;
+
+/* VSCMsgReconnect          Host -> Client
+ * */
+typedef struct VSCMsgReconnect {
+    uint32_t   ip;
+    uint16_t   port;
+} VSCMsgReconnect;
+
+#endif