| Message ID | 1294735359-4009-3-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 >
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
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 >
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 >
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 >>> >>
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]
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
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