[Qemu-devel] [PATCH 00/26] ccid and libcacard fixes for windows/mingw

[Qemu-devel] [PATCH 00/26] ccid and libcacard fixes for windows/mingw

[Alon Levy]

This series:
1. fixes windows guests to show the ccid device
2. changes libcacard to use glib
3. makes libcacard build under mingw
4. does some cleanups

It contains a few patches already posted to the list (the two Jim Meyering patches) which were already acked.

I'll make a pull request once this had some time to be reviewed.

Tested with a fedora and windows 7 guest.

The main non cleanup patches are:
  hw/usb/dev-smartcard-reader: support windows guest
  libcacard: correct T0 historical bytes size


Alon Levy (15):
  hw/ccid-card-passthru.c: add atr check
  ccid-card-passthru, dev-smartcard-reader: add debug environment
    variables
  hw/usb/dev-smartcard-reader.c: white space fixes
  hw/usb/dev-smartcard-reader.c: nicer debug messages
  hw/usb/dev-smartcard-reader.c: remove aborts (never triggered, but
    just in case)
  hw/usb/dev-smartcard-reader.c: define structs for CCID_Parameter
    internals
  hw/usb/dev-smartcard-reader.c: copy atr's protocol to ccid's
    parameters (adds todo's)
  hw/usb/dev-smartcard-reader.c: dwFeadvertise support for T=0 only
  hw/usb/dev-smartcard-reader: support windows guest
  dev-smartcard-reader: reuse usb.h definitions
  libcacard/vreader: add debugging messages for apdu
  libcacard: change default ATR
  libcacard: move atr setting from macro to function
  dev-smartcard-reader: empty implementation for Mechanical (fail
    correctly)
  libcacard/cac.c: questionable change to single return of big switch
    functions

Jim Meyering (2):
  ccid: make backend_enum_table "static const" and adjust users
  ccid: declare DEFAULT_ATR table to be "static const"

Marc-André Lureau (9):
  libcacard: correct T0 historical bytes size
  ccid-card-emul: do not crash if backend is not provided
  libcacard: use system config directory for nss db on win32
  util: move socket_init() to osdep.c
  build-sys: must link with -fstack-protector
  libcacard: fix mingw64 cross-compilation
  libcacard: split vscclient main() from socket reading
  libcacard: vscclient to use QemuThread for portability
  libcacard: teach vscclient to use GMainLoop for portability

 Makefile                      |   8 +-
 Makefile.objs                 |   1 +
 configure                     |   8 +-
 hw/ccid-card-emulated.c       |   9 +-
 hw/ccid-card-passthru.c       |  77 ++++++-
 hw/usb/dev-smartcard-reader.c | 259 ++++++++++++++++-----
 libcacard/cac.c               |  80 ++++---
 libcacard/cac.h               |   8 +
 libcacard/vcard_emul_nss.c    |  29 ++-
 libcacard/vcardt.c            |  40 ++++
 libcacard/vcardt.h            |   7 +-
 libcacard/vreader.c           |  89 ++++++++
 libcacard/vscclient.c         | 506 +++++++++++++++++++++++++-----------------
 rules.mak                     |   4 +-
 util/osdep.c                  |  23 ++
 util/qemu-sockets.c           |  24 --
 16 files changed, 846 insertions(+), 326 deletions(-)
 create mode 100644 libcacard/vcardt.c

-- 
1.8.1.4

[Qemu-devel] [PATCH 01/26] libcacard: correct T0 historical bytes size

[Alon Levy]

From: Marc-André Lureau <marcandre.lureau@gmail.com>

The VCARD_ATR_PREFIX macro adds a prefix of 6 characters only.

pcsc_scan was complaining before the patch:

+ Historical bytes: 56 43 41 52 44 5F 4E 53 53
 ERROR! ATR is truncated: 2 byte(s) is/are missing
---
 libcacard/vcardt.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libcacard/vcardt.h b/libcacard/vcardt.h
index d3e9522..538bdde 100644
--- a/libcacard/vcardt.h
+++ b/libcacard/vcardt.h
@@ -26,7 +26,7 @@ typedef struct VCardEmulStruct VCardEmul;
 #define MAX_CHANNEL 4
 
 /* create an ATR with appropriate historical bytes */
-#define VCARD_ATR_PREFIX(size) 0x3b, 0x68+(size), 0x00, 0xff, \
+#define VCARD_ATR_PREFIX(size) 0x3b, 0x66+(size), 0x00, 0xff, \
                                'V', 'C', 'A', 'R', 'D', '_'
 
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 02/26] ccid-card-emul: do not crash if backend is not provided

[Alon Levy]

From: Marc-André Lureau <marcandre.lureau@gmail.com>

Program received signal SIGSEGV, Segmentation fault.
__strcmp_sse42 () at ../sysdeps/x86_64/multiarch/strcmp-sse42.S:164
164               movdqu        (%rsi), %xmm2
(gdb) bt
    at /home/elmarco/320g/src/qemu/hw/ccid-card-emulated.c:477
    at /home/elmarco/320g/src/qemu/hw/ccid-card-emulated.c:503
---
 hw/ccid-card-emulated.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/ccid-card-emulated.c b/hw/ccid-card-emulated.c
index c8f8ba3..210433e 100644
--- a/hw/ccid-card-emulated.c
+++ b/hw/ccid-card-emulated.c
@@ -473,6 +473,9 @@ static uint32_t parse_enumeration(char *str,
 {
     uint32_t ret = not_found_value;
 
+    if (str == NULL)
+        return 0;
+
     while (table->name != NULL) {
         if (strcmp(table->name, str) == 0) {
             ret = table->value;
-- 
1.8.1.4

[Qemu-devel] [PATCH 03/26] ccid: make backend_enum_table "static const" and adjust users

[Alon Levy]

From: Jim Meyering <meyering@redhat.com>

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 hw/ccid-card-emulated.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ccid-card-emulated.c b/hw/ccid-card-emulated.c
index 210433e..cb9ef10 100644
--- a/hw/ccid-card-emulated.c
+++ b/hw/ccid-card-emulated.c
@@ -462,14 +462,14 @@ typedef struct EnumTable {
     uint32_t value;
 } EnumTable;
 
-EnumTable backend_enum_table[] = {
+static const EnumTable backend_enum_table[] = {
     {BACKEND_NSS_EMULATED_NAME, BACKEND_NSS_EMULATED},
     {BACKEND_CERTIFICATES_NAME, BACKEND_CERTIFICATES},
     {NULL, 0},
 };
 
 static uint32_t parse_enumeration(char *str,
-    EnumTable *table, uint32_t not_found_value)
+    const EnumTable *table, uint32_t not_found_value)
 {
     uint32_t ret = not_found_value;
 
@@ -490,7 +490,7 @@ static int emulated_initfn(CCIDCardState *base)
 {
     EmulatedState *card = DO_UPCAST(EmulatedState, base, base);
     VCardEmulError ret;
-    EnumTable *ptable;
+    const EnumTable *ptable;
 
     QSIMPLEQ_INIT(&card->event_list);
     QSIMPLEQ_INIT(&card->guest_apdu_list);
-- 
1.8.1.4

[Qemu-devel] [PATCH 04/26] ccid: declare DEFAULT_ATR table to be "static const"

[Alon Levy]

From: Jim Meyering <meyering@redhat.com>

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 hw/ccid-card-passthru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ccid-card-passthru.c b/hw/ccid-card-passthru.c
index 984bd0b..3566e55 100644
--- a/hw/ccid-card-passthru.c
+++ b/hw/ccid-card-passthru.c
@@ -27,7 +27,7 @@ do {                                                    \
 #define D_VERBOSE 4
 
 /* TODO: do we still need this? */
-uint8_t DEFAULT_ATR[] = {
+static const uint8_t DEFAULT_ATR[] = {
 /*
  * From some example somewhere
  * 0x3B, 0xB0, 0x18, 0x00, 0xD1, 0x81, 0x05, 0xB1, 0x40, 0x38, 0x1F, 0x03, 0x28
-- 
1.8.1.4

[Qemu-devel] [PATCH 05/26] libcacard: use system config directory for nss db on win32

[Alon Levy]

From: Marc-André Lureau <marcandre.lureau@gmail.com>

It's a bit nicer to look for default database under
CSIDL_COMMON_APPDATA\pki\nss rather that /etc/pki/nss.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 libcacard/vcard_emul_nss.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index df79476..21d4689 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -893,7 +893,23 @@ vcard_emul_init(const VCardEmulOptions *options)
     if (options->nss_db) {
         rv = NSS_Init(options->nss_db);
     } else {
-        rv = NSS_Init("sql:/etc/pki/nssdb");
+        gchar *path, *db;
+#ifndef _WIN32
+        path = g_strdup("/etc/pki/nssdb");
+#else
+        if (g_get_system_config_dirs() == NULL ||
+            g_get_system_config_dirs()[0] == NULL) {
+            return VCARD_EMUL_FAIL;
+        }
+
+        path = g_build_filename(
+            g_get_system_config_dirs()[0], "pki", "nssdb", NULL);
+#endif
+        db = g_strdup_printf("sql:%s", path);
+
+        rv = NSS_Init(db);
+        g_free(db);
+        g_free(path);
     }
     if (rv != SECSuccess) {
         return VCARD_EMUL_FAIL;
-- 
1.8.1.4

[Qemu-devel] [PATCH 06/26] util: move socket_init() to osdep.c

[Alon Levy]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

vscclient needs to call socket_init() for portability.
Moving to osdep.c since it has no internal dependency.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 util/osdep.c        | 23 +++++++++++++++++++++++
 util/qemu-sockets.c | 24 ------------------------
 2 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/util/osdep.c b/util/osdep.c
index bd59ac9..6ae5aaf 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -406,3 +406,26 @@ bool fips_get_state(void)
     return fips_enabled;
 }
 
+#ifdef _WIN32
+static void socket_cleanup(void)
+{
+    WSACleanup();
+}
+#endif
+
+int socket_init(void)
+{
+#ifdef _WIN32
+    WSADATA Data;
+    int ret, err;
+
+    ret = WSAStartup(MAKEWORD(2, 2), &Data);
+    if (ret != 0) {
+        err = WSAGetLastError();
+        fprintf(stderr, "WSAStartup: %d\n", err);
+        return -1;
+    }
+    atexit(socket_cleanup);
+#endif
+    return 0;
+}
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 83e4e08..f42f0b7 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -973,27 +973,3 @@ int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
     qemu_opts_del(opts);
     return fd;
 }
-
-#ifdef _WIN32
-static void socket_cleanup(void)
-{
-    WSACleanup();
-}
-#endif
-
-int socket_init(void)
-{
-#ifdef _WIN32
-    WSADATA Data;
-    int ret, err;
-
-    ret = WSAStartup(MAKEWORD(2,2), &Data);
-    if (ret != 0) {
-        err = WSAGetLastError();
-        fprintf(stderr, "WSAStartup: %d\n", err);
-        return -1;
-    }
-    atexit(socket_cleanup);
-#endif
-    return 0;
-}
-- 
1.8.1.4

[Qemu-devel] [PATCH 07/26] build-sys: must link with -fstack-protector

[Alon Levy]

From: Marc-André Lureau <marcandre.lureau@gmail.com>

It is needed to give that flag to the linker as well, but latest
libtool 2.4.2 still swallows that argument, so let's pass it with
libtool -Wc argument.

qemu-1.4.0/stubs/arch-query-cpu-def.c:6: undefined reference to `__stack_chk_guard'

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 configure | 8 +++++++-
 rules.mak | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 46a7594..d76866a 100755
--- a/configure
+++ b/configure
@@ -1209,7 +1209,7 @@ fi
 gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
 gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
 gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
-gcc_flags="-fstack-protector-all -Wendif-labels $gcc_flags"
+gcc_flags="-Wendif-labels $gcc_flags"
 gcc_flags="-Wno-initializer-overrides $gcc_flags"
 # Note that we do not add -Werror to gcc_flags here, because that would
 # enable it for all configure tests. If a configure test failed due
@@ -1228,6 +1228,11 @@ for flag in $gcc_flags; do
     fi
 done
 
+if compile_prog "-Werror -fstack-protector-all" "" ; then
+    QEMU_CFLAGS="$QEMU_CFLAGS -fstack-protector-all"
+    LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,-fstack-protector-all"
+fi
+
 # Workaround for http://gcc.gnu.org/PR55489.  Happens with -fPIE/-fPIC and
 # large functions that use global variables.  The bug is in all releases of
 # GCC, but it became particularly acute in 4.6.x and 4.7.x.  It is fixed in
@@ -3890,6 +3895,7 @@ else
   echo "AUTOCONF_HOST := "                             >> $config_host_mak
 fi
 echo "LDFLAGS=$LDFLAGS" >> $config_host_mak
+echo "LIBTOOLFLAGS=$LIBTOOLFLAGS" >> $config_host_mak
 echo "ARLIBS_BEGIN=$arlibs_begin" >> $config_host_mak
 echo "ARLIBS_END=$arlibs_end" >> $config_host_mak
 echo "LIBS+=$LIBS" >> $config_host_mak
diff --git a/rules.mak b/rules.mak
index edc2552..36aba2d 100644
--- a/rules.mak
+++ b/rules.mak
@@ -36,6 +36,7 @@ LINK = $(call quiet-command,\
        $(if $(filter %.lo %.la,$^),$(LIBTOOL) --mode=link --tag=CC \
        )$(CC) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ \
        $(sort $(filter %.o, $1)) $(filter-out %.o, $1) $(version-obj-y) \
+       $(if $(filter %.lo %.la,$^),$(LIBTOOLFLAGS)) \
        $(LIBS),$(if $(filter %.lo %.la,$^),"lt LINK ", "  LINK  ")"$(TARGET_DIR)$@")
 endif
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 08/26] libcacard: fix mingw64 cross-compilation

[Alon Levy]

From: Marc-André Lureau <marcandre.lureau@gmail.com>

Compile and link with version.lo

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 Makefile  | 8 ++++++--
 rules.mak | 3 ++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 6915178..10abfee 100644
--- a/Makefile
+++ b/Makefile
@@ -151,11 +151,15 @@ recurse-all: $(SUBDIR_RULES) $(ROMSUBDIR_RULES)
 
 bt-host.o: QEMU_CFLAGS += $(BLUEZ_CFLAGS)
 
-version.o: $(SRC_PATH)/version.rc config-host.h
+version.o: $(SRC_PATH)/version.rc config-host.h | version.lo
 	$(call quiet-command,$(WINDRES) -I. -o $@ $<,"  RC    $(TARGET_DIR)$@")
+version.lo: $(SRC_PATH)/version.rc config-host.h
+	$(call quiet-command,$(LIBTOOL) --mode=compile --tag=RC $(WINDRES) -I. -o $@ $<,"lt RC   $(TARGET_DIR)$@")
 
 version-obj-$(CONFIG_WIN32) += version.o
-Makefile: $(version-obj-y)
+version-lobj-$(CONFIG_WIN32) += $(if $(LIBTOOL),version.lo)
+Makefile: $(version-obj-y) $(version-lobj-y)
+
 
 ######################################################################
 # Build libraries
diff --git a/rules.mak b/rules.mak
index 36aba2d..292a422 100644
--- a/rules.mak
+++ b/rules.mak
@@ -35,7 +35,8 @@ LIBTOOL += $(if $(V),,--quiet)
 LINK = $(call quiet-command,\
        $(if $(filter %.lo %.la,$^),$(LIBTOOL) --mode=link --tag=CC \
        )$(CC) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ \
-       $(sort $(filter %.o, $1)) $(filter-out %.o, $1) $(version-obj-y) \
+       $(sort $(filter %.o, $1)) $(filter-out %.o, $1) \
+       $(if $(filter %.lo %.la,$^),$(version-lobj-y),$(version-obj-y)) \
        $(if $(filter %.lo %.la,$^),$(LIBTOOLFLAGS)) \
        $(LIBS),$(if $(filter %.lo %.la,$^),"lt LINK ", "  LINK  ")"$(TARGET_DIR)$@")
 endif
-- 
1.8.1.4

[Qemu-devel] [PATCH 09/26] libcacard: split vscclient main() from socket reading

[Alon Levy]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

---
 libcacard/vscclient.c | 314 ++++++++++++++++++++++++++------------------------
 1 file changed, 162 insertions(+), 152 deletions(-)

diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
index 9b744f2..5e00db3 100644
--- a/libcacard/vscclient.c
+++ b/libcacard/vscclient.c
@@ -211,6 +211,166 @@ get_id_from_string(char *string, unsigned int default_id)
     return id;
 }
 
+static int
+on_host_init(VSCMsgHeader *mhHeader, VSCMsgInit *incoming)
+{
+    uint32_t *capabilities = (incoming->capabilities);
+    int num_capabilities =
+        1 + ((mhHeader->length - sizeof(VSCMsgInit)) / sizeof(uint32_t));
+    int i;
+    int rv;
+    pthread_t thread_id;
+
+    incoming->version = ntohl(incoming->version);
+    if (incoming->version != VSCARD_VERSION) {
+        if (verbose > 0) {
+            printf("warning: host has version %d, we have %d\n",
+                verbose, VSCARD_VERSION);
+        }
+    }
+    if (incoming->magic != VSCARD_MAGIC) {
+        printf("unexpected magic: got %d, expected %d\n",
+            incoming->magic, VSCARD_MAGIC);
+        return -1;
+    }
+    for (i = 0 ; i < num_capabilities; ++i) {
+        capabilities[i] = ntohl(capabilities[i]);
+    }
+    /* Future: check capabilities */
+    /* remove whatever reader might be left in qemu,
+     * in case of an unclean previous exit. */
+    send_msg(VSC_ReaderRemove, VSCARD_MINIMAL_READER_ID, NULL, 0);
+    /* launch the event_thread. This will trigger reader adds for all the
+     * existing readers */
+    rv = pthread_create(&thread_id, NULL, event_thread, NULL);
+    if (rv < 0) {
+        perror("pthread_create");
+        return rv;
+    }
+    return 0;
+}
+
+#define APDUBufSize 270
+
+static int
+do_socket_read(void)
+{
+    int rv;
+    int dwSendLength;
+    int dwRecvLength;
+    uint8_t pbRecvBuffer[APDUBufSize];
+    uint8_t pbSendBuffer[APDUBufSize];
+    VReaderStatus reader_status;
+    VReader *reader = NULL;
+    VSCMsgHeader mhHeader;
+    VSCMsgError *error_msg;
+
+    rv = read(sock, &mhHeader, sizeof(mhHeader));
+    if (rv < sizeof(mhHeader)) {
+        /* Error */
+        if (rv < 0) {
+            perror("header read error\n");
+        } else {
+            fprintf(stderr, "header short read %d\n", rv);
+        }
+        return -1;
+    }
+    mhHeader.type = ntohl(mhHeader.type);
+    mhHeader.reader_id = ntohl(mhHeader.reader_id);
+    mhHeader.length = ntohl(mhHeader.length);
+    if (verbose) {
+        printf("Header: type=%d, reader_id=%u length=%d (0x%x)\n",
+               mhHeader.type, mhHeader.reader_id, mhHeader.length,
+               mhHeader.length);
+    }
+    switch (mhHeader.type) {
+    case VSC_APDU:
+    case VSC_Flush:
+    case VSC_Error:
+    case VSC_Init:
+        rv = read(sock, pbSendBuffer, mhHeader.length);
+        break;
+    default:
+        fprintf(stderr, "Unexpected message of type 0x%X\n", mhHeader.type);
+        return -1;
+    }
+    switch (mhHeader.type) {
+    case VSC_APDU:
+        if (rv < 0) {
+            /* Error */
+            fprintf(stderr, "read error\n");
+            close(sock);
+            return -1;
+        }
+        if (verbose) {
+            printf(" recv APDU: ");
+            print_byte_array(pbSendBuffer, mhHeader.length);
+        }
+        /* Transmit received APDU */
+        dwSendLength = mhHeader.length;
+        dwRecvLength = sizeof(pbRecvBuffer);
+        reader = vreader_get_reader_by_id(mhHeader.reader_id);
+        reader_status = vreader_xfr_bytes(reader,
+                                          pbSendBuffer, dwSendLength,
+                                          pbRecvBuffer, &dwRecvLength);
+        if (reader_status == VREADER_OK) {
+            mhHeader.length = dwRecvLength;
+            if (verbose) {
+                printf(" send response: ");
+                print_byte_array(pbRecvBuffer, mhHeader.length);
+            }
+            send_msg(VSC_APDU, mhHeader.reader_id,
+                     pbRecvBuffer, dwRecvLength);
+        } else {
+            rv = reader_status; /* warning: not meaningful */
+            send_msg(VSC_Error, mhHeader.reader_id, &rv, sizeof(uint32_t));
+        }
+        vreader_free(reader);
+        reader = NULL; /* we've freed it, don't use it by accident
+                          again */
+        break;
+    case VSC_Flush:
+        /* TODO: actually flush */
+        send_msg(VSC_FlushComplete, mhHeader.reader_id, NULL, 0);
+        break;
+    case VSC_Error:
+        error_msg = (VSCMsgError *) pbSendBuffer;
+        if (error_msg->code == VSC_SUCCESS) {
+            qemu_mutex_lock(&pending_reader_lock);
+            if (pending_reader) {
+                vreader_set_id(pending_reader, mhHeader.reader_id);
+                vreader_free(pending_reader);
+                pending_reader = NULL;
+                qemu_cond_signal(&pending_reader_condition);
+            }
+            qemu_mutex_unlock(&pending_reader_lock);
+            break;
+        }
+        printf("warning: qemu refused to add reader\n");
+        if (error_msg->code == VSC_CANNOT_ADD_MORE_READERS) {
+            /* clear pending reader, qemu can't handle any more */
+            qemu_mutex_lock(&pending_reader_lock);
+            if (pending_reader) {
+                pending_reader = NULL;
+                /* make sure the event loop doesn't hang */
+                qemu_cond_signal(&pending_reader_condition);
+            }
+            qemu_mutex_unlock(&pending_reader_lock);
+        }
+        break;
+    case VSC_Init:
+        if (on_host_init(&mhHeader, (VSCMsgInit *)pbSendBuffer) < 0) {
+            return -1;
+        }
+        break;
+    default:
+        printf("Default\n");
+        return -1;
+    }
+
+    return 0;
+}
+
 static void
 do_command(void)
 {
@@ -339,8 +499,6 @@ do_command(void)
 }
 
 
-#define APDUBufSize 270
-
 /* just for ease of parsing command line arguments. */
 #define MAX_CERTS 100
 
@@ -385,44 +543,6 @@ connect_to_qemu(
     return sock;
 }
 
-static int on_host_init(VSCMsgHeader *mhHeader, VSCMsgInit *incoming)
-{
-    uint32_t *capabilities = (incoming->capabilities);
-    int num_capabilities =
-        1 + ((mhHeader->length - sizeof(VSCMsgInit)) / sizeof(uint32_t));
-    int i;
-    int rv;
-    pthread_t thread_id;
-
-    incoming->version = ntohl(incoming->version);
-    if (incoming->version != VSCARD_VERSION) {
-        if (verbose > 0) {
-            printf("warning: host has version %d, we have %d\n",
-                verbose, VSCARD_VERSION);
-        }
-    }
-    if (incoming->magic != VSCARD_MAGIC) {
-        printf("unexpected magic: got %d, expected %d\n",
-            incoming->magic, VSCARD_MAGIC);
-        return -1;
-    }
-    for (i = 0 ; i < num_capabilities; ++i) {
-        capabilities[i] = ntohl(capabilities[i]);
-    }
-    /* Future: check capabilities */
-    /* remove whatever reader might be left in qemu,
-     * in case of an unclean previous exit. */
-    send_msg(VSC_ReaderRemove, VSCARD_MINIMAL_READER_ID, NULL, 0);
-    /* launch the event_thread. This will trigger reader adds for all the
-     * existing readers */
-    rv = pthread_create(&thread_id, NULL, event_thread, NULL);
-    if (rv < 0) {
-        perror("pthread_create");
-        return rv;
-    }
-    return 0;
-}
-
 int
 main(
     int argc,
@@ -431,21 +551,13 @@ main(
     char *qemu_host;
     char *qemu_port;
     VSCMsgHeader mhHeader;
-    VSCMsgError *error_msg;
 
-    int rv;
-    int dwSendLength;
-    int dwRecvLength;
-    uint8_t pbRecvBuffer[APDUBufSize];
-    uint8_t pbSendBuffer[APDUBufSize];
-     VReaderStatus reader_status;
-    VReader *reader = NULL;
     VCardEmulOptions *command_line_options = NULL;
 
     char *cert_names[MAX_CERTS];
     char *emul_args = NULL;
     int cert_count = 0;
-    int c;
+    int c, rv;
 
     while ((c = getopt(argc, argv, "c:e:pd:")) != -1) {
         switch (c) {
@@ -548,109 +660,7 @@ main(
         if (!FD_ISSET(sock, &fds)) {
             continue;
         }
-
-        rv = read(sock, &mhHeader, sizeof(mhHeader));
-        if (rv < sizeof(mhHeader)) {
-            /* Error */
-            if (rv < 0) {
-                perror("header read error\n");
-            } else {
-                fprintf(stderr, "header short read %d\n", rv);
-            }
-            return 8;
-        }
-        mhHeader.type = ntohl(mhHeader.type);
-        mhHeader.reader_id = ntohl(mhHeader.reader_id);
-        mhHeader.length = ntohl(mhHeader.length);
-        if (verbose) {
-            printf("Header: type=%d, reader_id=%u length=%d (0x%x)\n",
-                    mhHeader.type, mhHeader.reader_id, mhHeader.length,
-                                               mhHeader.length);
-        }
-        switch (mhHeader.type) {
-        case VSC_APDU:
-        case VSC_Flush:
-        case VSC_Error:
-        case VSC_Init:
-            rv = read(sock, pbSendBuffer, mhHeader.length);
-            break;
-        default:
-            fprintf(stderr, "Unexpected message of type 0x%X\n", mhHeader.type);
-            return 0;
-        }
-        switch (mhHeader.type) {
-        case VSC_APDU:
-            if (rv < 0) {
-                /* Error */
-                fprintf(stderr, "read error\n");
-                close(sock);
-                return 8;
-            }
-            if (verbose) {
-                printf(" recv APDU: ");
-                print_byte_array(pbSendBuffer, mhHeader.length);
-            }
-            /* Transmit received APDU */
-            dwSendLength = mhHeader.length;
-            dwRecvLength = sizeof(pbRecvBuffer);
-            reader = vreader_get_reader_by_id(mhHeader.reader_id);
-            reader_status = vreader_xfr_bytes(reader,
-                pbSendBuffer, dwSendLength,
-                pbRecvBuffer, &dwRecvLength);
-            if (reader_status == VREADER_OK) {
-                mhHeader.length = dwRecvLength;
-                if (verbose) {
-                    printf(" send response: ");
-                    print_byte_array(pbRecvBuffer, mhHeader.length);
-                }
-                send_msg(VSC_APDU, mhHeader.reader_id,
-                         pbRecvBuffer, dwRecvLength);
-            } else {
-                rv = reader_status; /* warning: not meaningful */
-                send_msg(VSC_Error, mhHeader.reader_id, &rv, sizeof(uint32_t));
-            }
-            vreader_free(reader);
-            reader = NULL; /* we've freed it, don't use it by accident
-                              again */
-            break;
-        case VSC_Flush:
-            /* TODO: actually flush */
-            send_msg(VSC_FlushComplete, mhHeader.reader_id, NULL, 0);
-            break;
-        case VSC_Error:
-            error_msg = (VSCMsgError *) pbSendBuffer;
-            if (error_msg->code == VSC_SUCCESS) {
-                qemu_mutex_lock(&pending_reader_lock);
-                if (pending_reader) {
-                    vreader_set_id(pending_reader, mhHeader.reader_id);
-                    vreader_free(pending_reader);
-                    pending_reader = NULL;
-                    qemu_cond_signal(&pending_reader_condition);
-                }
-                qemu_mutex_unlock(&pending_reader_lock);
-                break;
-            }
-            printf("warning: qemu refused to add reader\n");
-            if (error_msg->code == VSC_CANNOT_ADD_MORE_READERS) {
-                /* clear pending reader, qemu can't handle any more */
-                qemu_mutex_lock(&pending_reader_lock);
-                if (pending_reader) {
-                    pending_reader = NULL;
-                    /* make sure the event loop doesn't hang */
-                    qemu_cond_signal(&pending_reader_condition);
-                }
-                qemu_mutex_unlock(&pending_reader_lock);
-            }
-            break;
-        case VSC_Init:
-            if (on_host_init(&mhHeader, (VSCMsgInit *)pbSendBuffer) < 0) {
-                return -1;
-            }
-            break;
-        default:
-            printf("Default\n");
-            return 0;
-        }
+        rv = do_socket_read();
     } while (rv >= 0);
 
     return 0;
-- 
1.8.1.4

[Qemu-devel] [PATCH 10/26] libcacard: vscclient to use QemuThread for portability

[Alon Levy]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

---
 libcacard/vscclient.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
index 5e00db3..5f47634 100644
--- a/libcacard/vscclient.c
+++ b/libcacard/vscclient.c
@@ -218,8 +218,7 @@ on_host_init(VSCMsgHeader *mhHeader, VSCMsgInit *incoming)
     int num_capabilities =
         1 + ((mhHeader->length - sizeof(VSCMsgInit)) / sizeof(uint32_t));
     int i;
-    int rv;
-    pthread_t thread_id;
+    QemuThread thread_id;
 
     incoming->version = ntohl(incoming->version);
     if (incoming->version != VSCARD_VERSION) {
@@ -242,11 +241,7 @@ on_host_init(VSCMsgHeader *mhHeader, VSCMsgInit *incoming)
     send_msg(VSC_ReaderRemove, VSCARD_MINIMAL_READER_ID, NULL, 0);
     /* launch the event_thread. This will trigger reader adds for all the
      * existing readers */
-    rv = pthread_create(&thread_id, NULL, event_thread, NULL);
-    if (rv < 0) {
-        perror("pthread_create");
-        return rv;
-    }
+    qemu_thread_create(&thread_id, event_thread, NULL, 0);
     return 0;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 11/26] libcacard: teach vscclient to use GMainLoop for portability

[Alon Levy]

From: Marc-André Lureau <marcandre.lureau@gmail.com>

This version handles non-blocking sending and receiving from the
socket.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 libcacard/vscclient.c | 391 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 246 insertions(+), 145 deletions(-)

diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
index 5f47634..ac23647 100644
--- a/libcacard/vscclient.c
+++ b/libcacard/vscclient.c
@@ -10,7 +10,10 @@
  * See the COPYING.LIB file in the top-level directory.
  */
 
+#ifndef _WIN32
 #include <netdb.h>
+#endif
+#include <glib.h>
 
 #include "qemu-common.h"
 #include "qemu/thread.h"
@@ -22,9 +25,7 @@
 #include "vcard_emul.h"
 #include "vevent.h"
 
-int verbose;
-
-int sock;
+static int verbose;
 
 static void
 print_byte_array(
@@ -51,7 +52,47 @@ print_usage(void) {
     vcard_emul_usage();
 }
 
-static QemuMutex write_lock;
+static GIOChannel *channel_socket;
+static GByteArray *socket_to_send;
+static QemuMutex socket_to_send_lock;
+static guint socket_tag;
+
+static void
+update_socket_watch(gboolean out);
+
+static gboolean
+do_socket_send(GIOChannel *source,
+               GIOCondition condition,
+               gpointer data)
+{
+    gsize bw;
+    GError *err = NULL;
+
+    g_return_val_if_fail(socket_to_send->len != 0, FALSE);
+    g_return_val_if_fail(condition & G_IO_OUT, FALSE);
+
+    g_io_channel_write_chars(channel_socket,
+        (gchar *)socket_to_send->data, socket_to_send->len, &bw, &err);
+    if (err != NULL) {
+        g_error("Error while sending socket %s", err->message);
+        return FALSE;
+    }
+    g_byte_array_remove_range(socket_to_send, 0, bw);
+
+    if (socket_to_send->len == 0) {
+        update_socket_watch(FALSE);
+        return FALSE;
+    }
+    return TRUE;
+}
+
+static gboolean
+socket_prepare_sending(gpointer user_data)
+{
+    update_socket_watch(TRUE);
+
+    return FALSE;
+}
 
 static int
 send_msg(
@@ -60,10 +101,9 @@ send_msg(
     const void *msg,
     unsigned int length
 ) {
-    int rv;
     VSCMsgHeader mhHeader;
 
-    qemu_mutex_lock(&write_lock);
+    qemu_mutex_lock(&socket_to_send_lock);
 
     if (verbose > 10) {
         printf("sending type=%d id=%u, len =%u (0x%x)\n",
@@ -73,23 +113,11 @@ send_msg(
     mhHeader.type = htonl(type);
     mhHeader.reader_id = 0;
     mhHeader.length = htonl(length);
-    rv = write(sock, &mhHeader, sizeof(mhHeader));
-    if (rv < 0) {
-        /* Error */
-        fprintf(stderr, "write header error\n");
-        close(sock);
-        qemu_mutex_unlock(&write_lock);
-        return 16;
-    }
-    rv = write(sock, msg, length);
-    if (rv < 0) {
-        /* Error */
-        fprintf(stderr, "write error\n");
-        close(sock);
-        qemu_mutex_unlock(&write_lock);
-        return 16;
-    }
-    qemu_mutex_unlock(&write_lock);
+    g_byte_array_append(socket_to_send, (guint8 *)&mhHeader, sizeof(mhHeader));
+    g_byte_array_append(socket_to_send, (guint8 *)msg, length);
+    g_idle_add(socket_prepare_sending, NULL);
+
+    qemu_mutex_unlock(&socket_to_send_lock);
 
     return 0;
 }
@@ -245,139 +273,203 @@ on_host_init(VSCMsgHeader *mhHeader, VSCMsgInit *incoming)
     return 0;
 }
 
+
+enum {
+    STATE_HEADER,
+    STATE_MESSAGE,
+};
+
 #define APDUBufSize 270
 
-static int
-do_socket_read(void)
+static gboolean
+do_socket_read(GIOChannel *source,
+               GIOCondition condition,
+               gpointer data)
 {
     int rv;
     int dwSendLength;
     int dwRecvLength;
     uint8_t pbRecvBuffer[APDUBufSize];
-    uint8_t pbSendBuffer[APDUBufSize];
+    static uint8_t pbSendBuffer[APDUBufSize];
     VReaderStatus reader_status;
     VReader *reader = NULL;
-    VSCMsgHeader mhHeader;
+    static VSCMsgHeader mhHeader;
     VSCMsgError *error_msg;
+    GError *err = NULL;
 
-    rv = read(sock, &mhHeader, sizeof(mhHeader));
-    if (rv < sizeof(mhHeader)) {
-        /* Error */
-        if (rv < 0) {
-            perror("header read error\n");
-        } else {
-            fprintf(stderr, "header short read %d\n", rv);
-        }
-        return -1;
-    }
-    mhHeader.type = ntohl(mhHeader.type);
-    mhHeader.reader_id = ntohl(mhHeader.reader_id);
-    mhHeader.length = ntohl(mhHeader.length);
-    if (verbose) {
-        printf("Header: type=%d, reader_id=%u length=%d (0x%x)\n",
-               mhHeader.type, mhHeader.reader_id, mhHeader.length,
-               mhHeader.length);
-    }
-    switch (mhHeader.type) {
-    case VSC_APDU:
-    case VSC_Flush:
-    case VSC_Error:
-    case VSC_Init:
-        rv = read(sock, pbSendBuffer, mhHeader.length);
-        break;
-    default:
-        fprintf(stderr, "Unexpected message of type 0x%X\n", mhHeader.type);
-        return -1;
+    static gchar *buf;
+    static gsize br, to_read;
+    static int state = STATE_HEADER;
+
+    if (state == STATE_HEADER && to_read == 0) {
+        buf = (gchar *)&mhHeader;
+        to_read = sizeof(mhHeader);
     }
-    switch (mhHeader.type) {
-    case VSC_APDU:
-        if (rv < 0) {
-            /* Error */
-            fprintf(stderr, "read error\n");
-            close(sock);
-            return -1;
+
+    if (to_read > 0) {
+        g_io_channel_read_chars(source, (gchar *)buf, to_read, &br, &err);
+        if (err != NULL) {
+            g_error("error while reading: %s", err->message);
         }
+        buf += br;
+        to_read -= br;
+        if (to_read != 0) {
+            return TRUE;
+        }
+    }
+
+    if (state == STATE_HEADER) {
+        mhHeader.type = ntohl(mhHeader.type);
+        mhHeader.reader_id = ntohl(mhHeader.reader_id);
+        mhHeader.length = ntohl(mhHeader.length);
         if (verbose) {
-            printf(" recv APDU: ");
-            print_byte_array(pbSendBuffer, mhHeader.length);
+            printf("Header: type=%d, reader_id=%u length=%d (0x%x)\n",
+                   mhHeader.type, mhHeader.reader_id, mhHeader.length,
+                   mhHeader.length);
         }
-        /* Transmit received APDU */
-        dwSendLength = mhHeader.length;
-        dwRecvLength = sizeof(pbRecvBuffer);
-        reader = vreader_get_reader_by_id(mhHeader.reader_id);
-        reader_status = vreader_xfr_bytes(reader,
-                                          pbSendBuffer, dwSendLength,
-                                          pbRecvBuffer, &dwRecvLength);
-        if (reader_status == VREADER_OK) {
-            mhHeader.length = dwRecvLength;
+        switch (mhHeader.type) {
+        case VSC_APDU:
+        case VSC_Flush:
+        case VSC_Error:
+        case VSC_Init:
+            buf = (gchar *)pbSendBuffer;
+            to_read = mhHeader.length;
+            state = STATE_MESSAGE;
+            return TRUE;
+        default:
+            fprintf(stderr, "Unexpected message of type 0x%X\n", mhHeader.type);
+            return FALSE;
+        }
+    }
+
+    if (state == STATE_MESSAGE) {
+        switch (mhHeader.type) {
+        case VSC_APDU:
             if (verbose) {
-                printf(" send response: ");
-                print_byte_array(pbRecvBuffer, mhHeader.length);
+                printf(" recv APDU: ");
+                print_byte_array(pbSendBuffer, mhHeader.length);
             }
-            send_msg(VSC_APDU, mhHeader.reader_id,
-                     pbRecvBuffer, dwRecvLength);
-        } else {
-            rv = reader_status; /* warning: not meaningful */
-            send_msg(VSC_Error, mhHeader.reader_id, &rv, sizeof(uint32_t));
-        }
-        vreader_free(reader);
-        reader = NULL; /* we've freed it, don't use it by accident
-                          again */
-        break;
-    case VSC_Flush:
-        /* TODO: actually flush */
-        send_msg(VSC_FlushComplete, mhHeader.reader_id, NULL, 0);
-        break;
-    case VSC_Error:
-        error_msg = (VSCMsgError *) pbSendBuffer;
-        if (error_msg->code == VSC_SUCCESS) {
-            qemu_mutex_lock(&pending_reader_lock);
-            if (pending_reader) {
-                vreader_set_id(pending_reader, mhHeader.reader_id);
-                vreader_free(pending_reader);
-                pending_reader = NULL;
-                qemu_cond_signal(&pending_reader_condition);
+            /* Transmit received APDU */
+            dwSendLength = mhHeader.length;
+            dwRecvLength = sizeof(pbRecvBuffer);
+            reader = vreader_get_reader_by_id(mhHeader.reader_id);
+            reader_status = vreader_xfr_bytes(reader,
+                                              pbSendBuffer, dwSendLength,
+                                              pbRecvBuffer, &dwRecvLength);
+            if (reader_status == VREADER_OK) {
+                mhHeader.length = dwRecvLength;
+                if (verbose) {
+                    printf(" send response: ");
+                    print_byte_array(pbRecvBuffer, mhHeader.length);
+                }
+                send_msg(VSC_APDU, mhHeader.reader_id,
+                         pbRecvBuffer, dwRecvLength);
+            } else {
+                rv = reader_status; /* warning: not meaningful */
+                send_msg(VSC_Error, mhHeader.reader_id, &rv, sizeof(uint32_t));
             }
-            qemu_mutex_unlock(&pending_reader_lock);
+            vreader_free(reader);
+            reader = NULL; /* we've freed it, don't use it by accident
+                              again */
             break;
-        }
-        printf("warning: qemu refused to add reader\n");
-        if (error_msg->code == VSC_CANNOT_ADD_MORE_READERS) {
-            /* clear pending reader, qemu can't handle any more */
-            qemu_mutex_lock(&pending_reader_lock);
-            if (pending_reader) {
-                pending_reader = NULL;
-                /* make sure the event loop doesn't hang */
-                qemu_cond_signal(&pending_reader_condition);
+        case VSC_Flush:
+            /* TODO: actually flush */
+            send_msg(VSC_FlushComplete, mhHeader.reader_id, NULL, 0);
+            break;
+        case VSC_Error:
+            error_msg = (VSCMsgError *) pbSendBuffer;
+            if (error_msg->code == VSC_SUCCESS) {
+                qemu_mutex_lock(&pending_reader_lock);
+                if (pending_reader) {
+                    vreader_set_id(pending_reader, mhHeader.reader_id);
+                    vreader_free(pending_reader);
+                    pending_reader = NULL;
+                    qemu_cond_signal(&pending_reader_condition);
+                }
+                qemu_mutex_unlock(&pending_reader_lock);
+                break;
             }
-            qemu_mutex_unlock(&pending_reader_lock);
+            printf("warning: qemu refused to add reader\n");
+            if (error_msg->code == VSC_CANNOT_ADD_MORE_READERS) {
+                /* clear pending reader, qemu can't handle any more */
+                qemu_mutex_lock(&pending_reader_lock);
+                if (pending_reader) {
+                    pending_reader = NULL;
+                    /* make sure the event loop doesn't hang */
+                    qemu_cond_signal(&pending_reader_condition);
+                }
+                qemu_mutex_unlock(&pending_reader_lock);
+            }
+            break;
+        case VSC_Init:
+            if (on_host_init(&mhHeader, (VSCMsgInit *)pbSendBuffer) < 0) {
+                return FALSE;
+            }
+            break;
+        default:
+            g_warn_if_reached();
+            return FALSE;
         }
-        break;
-    case VSC_Init:
-        if (on_host_init(&mhHeader, (VSCMsgInit *)pbSendBuffer) < 0) {
-            return -1;
+
+        state = STATE_HEADER;
+    }
+
+
+    return TRUE;
+}
+
+static gboolean
+do_socket(GIOChannel *source,
+          GIOCondition condition,
+          gpointer data)
+{
+    /* not sure if two watches work well with a single win32 sources */
+    if (condition & G_IO_OUT) {
+        if (!do_socket_send(source, condition, data)) {
+            return FALSE;
         }
-        break;
-    default:
-        printf("Default\n");
-        return -1;
     }
 
-    return 0;
+    if (condition & G_IO_IN) {
+        if (!do_socket_read(source, condition, data)) {
+            return FALSE;
+        }
+    }
+
+    return TRUE;
 }
 
 static void
-do_command(void)
+update_socket_watch(gboolean out)
+{
+    if (socket_tag != 0) {
+        g_source_remove(socket_tag);
+    }
+
+    socket_tag = g_io_add_watch(channel_socket,
+        G_IO_IN | (out ? G_IO_OUT : 0), do_socket, NULL);
+}
+
+static gboolean
+do_command(GIOChannel *source,
+           GIOCondition condition,
+           gpointer data)
 {
-    char inbuf[255];
     char *string;
     VCardEmulError error;
     static unsigned int default_reader_id;
     unsigned int reader_id;
     VReader *reader = NULL;
+    GError *err = NULL;
+
+    g_assert(condition & G_IO_IN);
 
     reader_id = default_reader_id;
-    string = fgets(inbuf, sizeof(inbuf), stdin);
+    g_io_channel_read_line(source, &string, NULL, NULL, &err);
+    if (err != NULL) {
+        g_error("Error while reading command: %s", err->message);
+    }
+
     if (string != NULL) {
         if (strncmp(string, "exit", 4) == 0) {
             /* remove all the readers */
@@ -491,6 +583,8 @@ do_command(void)
     vreader_free(reader);
     printf("> ");
     fflush(stdout);
+
+    return TRUE;
 }
 
 
@@ -504,7 +598,7 @@ connect_to_qemu(
 ) {
     struct addrinfo hints;
     struct addrinfo *server;
-    int ret;
+    int ret, sock;
 
     sock = qemu_socket(AF_INET, SOCK_STREAM, 0);
     if (sock < 0) {
@@ -543,6 +637,8 @@ main(
     int argc,
     char *argv[]
 ) {
+    GMainLoop *loop;
+    GIOChannel *channel_stdin;
     char *qemu_host;
     char *qemu_port;
     VSCMsgHeader mhHeader;
@@ -552,7 +648,10 @@ main(
     char *cert_names[MAX_CERTS];
     char *emul_args = NULL;
     int cert_count = 0;
-    int c, rv;
+    int c, sock;
+
+    if (socket_init() != 0)
+        return 1;
 
     while ((c = getopt(argc, argv, "c:e:pd:")) != -1) {
         switch (c) {
@@ -618,15 +717,33 @@ main(
         exit(5);
     }
 
-    qemu_mutex_init(&write_lock);
+    socket_to_send = g_byte_array_new();
+    qemu_mutex_init(&socket_to_send_lock);
     qemu_mutex_init(&pending_reader_lock);
     qemu_cond_init(&pending_reader_condition);
 
     vcard_emul_init(command_line_options);
 
+    loop = g_main_loop_new(NULL, true);
+
     printf("> ");
     fflush(stdout);
 
+#ifdef _WIN32
+    channel_stdin = g_io_channel_win32_new_fd(STDIN_FILENO);
+#else
+    channel_stdin = g_io_channel_unix_new(STDIN_FILENO);
+#endif
+    g_io_add_watch(channel_stdin, G_IO_IN, do_command, NULL);
+#ifdef _WIN32
+    channel_socket = g_io_channel_win32_new_socket(sock);
+#else
+    channel_socket = g_io_channel_unix_new(sock);
+#endif
+    g_io_channel_set_encoding(channel_socket, NULL, NULL);
+    /* we buffer ourself for thread safety reasons */
+    g_io_channel_set_buffered(channel_socket, FALSE);
+
     /* Send init message, Host responds (and then we send reader attachments) */
     VSCMsgInit init = {
         .version = htonl(VSCARD_VERSION),
@@ -635,28 +752,12 @@ main(
     };
     send_msg(VSC_Init, mhHeader.reader_id, &init, sizeof(init));
 
-    do {
-        fd_set fds;
-
-        FD_ZERO(&fds);
-        FD_SET(1, &fds);
-        FD_SET(sock, &fds);
+    g_main_loop_run(loop);
+    g_main_loop_unref(loop);
 
-        /* waiting on input from the socket */
-        rv = select(sock+1, &fds, NULL, NULL, NULL);
-        if (rv < 0) {
-            /* handle error */
-            perror("select");
-            return 7;
-        }
-        if (FD_ISSET(1, &fds)) {
-            do_command();
-        }
-        if (!FD_ISSET(sock, &fds)) {
-            continue;
-        }
-        rv = do_socket_read();
-    } while (rv >= 0);
+    g_io_channel_unref(channel_stdin);
+    g_io_channel_unref(channel_socket);
+    g_byte_array_unref(socket_to_send);
 
     return 0;
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH 12/26] hw/ccid-card-passthru.c: add atr check

[Alon Levy]

From: Alon Levy <alon@pobox.com>

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/ccid-card-passthru.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/hw/ccid-card-passthru.c b/hw/ccid-card-passthru.c
index 3566e55..111894f 100644
--- a/hw/ccid-card-passthru.c
+++ b/hw/ccid-card-passthru.c
@@ -138,6 +138,59 @@ static void ccid_card_vscard_handle_init(
     ccid_card_vscard_send_init(card);
 }
 
+static int check_atr(PassthruState *card, uint8_t *data, int len)
+{
+    int historical_length, opt_bytes;
+    int td_count = 0;
+    int td;
+
+    if (len < 2) {
+        return 0;
+    }
+    historical_length = data[1] & 0xf;
+    opt_bytes = 0;
+    if (data[0] != 0x3b && data[0] != 0x3f) {
+        DPRINTF(card, D_WARN, "atr's T0 is 0x%X, not in {0x3b, 0x3f}\n",
+                data[0]);
+        return 0;
+    }
+    td_count = 0;
+    td = data[1] >> 4;
+    while (td && td_count < 2 && opt_bytes + historical_length + 2 < len) {
+        td_count++;
+        if (td & 0x1) {
+            opt_bytes++;
+        }
+        if (td & 0x2) {
+            opt_bytes++;
+        }
+        if (td & 0x4) {
+            opt_bytes++;
+        }
+        if (td & 0x8) {
+            opt_bytes++;
+            td = data[opt_bytes + 2] >> 4;
+        }
+    }
+    if (len < 2 + historical_length + opt_bytes) {
+        DPRINTF(card, D_WARN,
+            "atr too short: len %d, but historical_len %d, T1 0x%X\n",
+            len, historical_length, data[1]);
+        return 0;
+    }
+    if (len > 2 + historical_length + opt_bytes) {
+        DPRINTF(card, D_WARN,
+            "atr too long: len %d, but hist/opt %d/%d, T1 0x%X\n",
+            len, historical_length, opt_bytes, data[1]);
+        /* let it through */
+    }
+    DPRINTF(card, D_VERBOSE,
+            "atr passes check: %d total length, %d historical, %d optional\n",
+            len, historical_length, opt_bytes);
+
+    return 1;
+}
+
 static void ccid_card_vscard_handle_message(PassthruState *card,
     VSCMsgHeader *scr_msg_header)
 {
@@ -152,6 +205,12 @@ static void ccid_card_vscard_handle_message(PassthruState *card,
                                         VSC_GENERAL_ERROR);
             break;
         }
+        if (!check_atr(card, data, scr_msg_header->length)) {
+            error_report("ATR is inconsistent, ignoring");
+            ccid_card_vscard_send_error(card, scr_msg_header->reader_id,
+                                        VSC_GENERAL_ERROR);
+            break;
+        }
         memcpy(card->atr, data, scr_msg_header->length);
         card->atr_length = scr_msg_header->length;
         ccid_card_card_inserted(&card->base);
-- 
1.8.1.4

[Qemu-devel] [PATCH 13/26] ccid-card-passthru, dev-smartcard-reader: add debug environment variables

[Alon Levy]

From: Alon Levy <alon@pobox.com>

This overrides whatever debug value is set on the corresponding devices
from the command line, and is meant to ease the usage with any
management stack. For libvirt you can set environment variables by
extending the dom namespace, i.e:

<domain type='kvm' id='3' xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'>
  <qemu:commandline>
    <qemu:env name='QEMU_CCID_PASSTHRU_DEBUG' value='4'/>
    <qemu:env name='QEMU_CCID_DEBUG' value='4'/>
  </qemu:commandline>
</domain>

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/ccid-card-passthru.c       | 16 ++++++++++++++++
 hw/usb/dev-smartcard-reader.c | 16 ++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/hw/ccid-card-passthru.c b/hw/ccid-card-passthru.c
index 111894f..80fe514 100644
--- a/hw/ccid-card-passthru.c
+++ b/hw/ccid-card-passthru.c
@@ -350,6 +350,22 @@ static int passthru_initfn(CCIDCardState *base)
         error_report("missing chardev");
         return -1;
     }
+    {
+        char *debug_env = getenv("QEMU_CCID_PASSTHRU_DEBUG");
+        char *inv = NULL;
+        int debug;
+        if (debug_env) {
+            debug = strtol(debug_env, &inv, 10);
+            if (inv != debug_env) {
+                if (debug < 0 || debug > D_VERBOSE) {
+                    fprintf(stderr, "warning: QEMU_CCID_PASSTHRU_DEBUG"
+                            " not in [0, %d]", D_VERBOSE);
+                } else {
+                    card->debug = debug;
+                }
+            }
+        }
+    }
     assert(sizeof(DEFAULT_ATR) <= MAX_ATR_SIZE);
     memcpy(card->atr, DEFAULT_ATR, sizeof(DEFAULT_ATR));
     card->atr_length = sizeof(DEFAULT_ATR);
diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index caebc1c..fc950c8 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1201,6 +1201,22 @@ static int ccid_initfn(USBDevice *dev)
     s->bulk_out_pos = 0;
     ccid_reset_parameters(s);
     ccid_reset(s);
+    {
+        char *debug_env = getenv("QEMU_CCID_DEBUG");
+        char *inv = NULL;
+        int debug;
+        if (debug_env) {
+            debug = strtol(debug_env, &inv, 10);
+            if (inv != debug_env) {
+                if (debug < 0 || debug > D_VERBOSE) {
+                    fprintf(stderr, "warning: QEMU_CCID_PASSTHRU_DEBUG"
+                            " not in [0, %d]", D_VERBOSE);
+                } else {
+                    s->debug = debug;
+                }
+            }
+        }
+    }
     return 0;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 14/26] hw/usb/dev-smartcard-reader.c: white space fixes

[Alon Levy]

From: Alon Levy <alon@pobox.com>

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/usb/dev-smartcard-reader.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index fc950c8..7205613 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -471,6 +471,7 @@ static const USBDesc desc_ccid = {
 static const uint8_t *ccid_card_get_atr(CCIDCardState *card, uint32_t *len)
 {
     CCIDCardClass *cc = CCID_CARD_GET_CLASS(card);
+
     if (cc->get_atr) {
         return cc->get_atr(card, len);
     }
@@ -482,6 +483,7 @@ static void ccid_card_apdu_from_guest(CCIDCardState *card,
                                       uint32_t len)
 {
     CCIDCardClass *cc = CCID_CARD_GET_CLASS(card);
+
     if (cc->apdu_from_guest) {
         cc->apdu_from_guest(card, apdu, len);
     }
@@ -490,6 +492,7 @@ static void ccid_card_apdu_from_guest(CCIDCardState *card,
 static int ccid_card_exitfn(CCIDCardState *card)
 {
     CCIDCardClass *cc = CCID_CARD_GET_CLASS(card);
+
     if (cc->exitfn) {
         return cc->exitfn(card);
     }
@@ -499,6 +502,7 @@ static int ccid_card_exitfn(CCIDCardState *card)
 static int ccid_card_initfn(CCIDCardState *card)
 {
     CCIDCardClass *cc = CCID_CARD_GET_CLASS(card);
+
     if (cc->initfn) {
         return cc->initfn(card);
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 15/26] hw/usb/dev-smartcard-reader.c: nicer debug messages

[Alon Levy]

From: Alon Levy <alon@pobox.com>

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/usb/dev-smartcard-reader.c | 69 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 63 insertions(+), 6 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 7205613..4ff0641 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -639,13 +639,47 @@ static void ccid_handle_reset(USBDevice *dev)
     ccid_reset(s);
 }
 
+static const char *ccid_control_to_str(USBCCIDState *s, int request)
+{
+    switch (request) {
+        /* generic - should be factored out if there are other debugees */
+    case DeviceOutRequest | USB_REQ_SET_ADDRESS:
+        return "(generic) set address";
+    case DeviceRequest | USB_REQ_GET_DESCRIPTOR:
+        return "(generic) get descriptor";
+    case DeviceRequest | USB_REQ_GET_CONFIGURATION:
+        return "(generic) get configuration";
+    case DeviceOutRequest | USB_REQ_SET_CONFIGURATION:
+        return "(generic) set configuration";
+    case DeviceRequest | USB_REQ_GET_STATUS:
+        return "(generic) get status";
+    case DeviceOutRequest | USB_REQ_CLEAR_FEATURE:
+        return "(generic) clear feature";
+    case DeviceOutRequest | USB_REQ_SET_FEATURE:
+        return "(generic) set_feature";
+    case InterfaceRequest | USB_REQ_GET_INTERFACE:
+        return "(generic) get interface";
+    case InterfaceOutRequest | USB_REQ_SET_INTERFACE:
+        return "(generic) set interface";
+        /* class requests */
+    case ClassInterfaceOutRequest | CCID_CONTROL_ABORT:
+        return "ABORT";
+    case ClassInterfaceRequest | CCID_CONTROL_GET_CLOCK_FREQUENCIES:
+        return "GET_CLOCK_FREQUENCIES";
+    case ClassInterfaceRequest | CCID_CONTROL_GET_DATA_RATES:
+        return "GET_DATA_RATES";
+    }
+    return "unknown";
+}
+
 static void ccid_handle_control(USBDevice *dev, USBPacket *p, int request,
                                int value, int index, int length, uint8_t *data)
 {
     USBCCIDState *s = DO_UPCAST(USBCCIDState, dev, dev);
     int ret;
 
-    DPRINTF(s, 1, "got control %x, value %x\n", request, value);
+    DPRINTF(s, 1, "%s: got control %s (%x), value %x\n", __func__,
+            ccid_control_to_str(s, request), request, value);
     ret = usb_desc_handle_control(dev, p, request, value, index, length, data);
     if (ret >= 0) {
         return;
@@ -695,7 +729,7 @@ static uint8_t ccid_calc_status(USBCCIDState *s)
      * bmCommandStatus
      */
     uint8_t ret = ccid_card_status(s) | (s->bmCommandStatus << 6);
-    DPRINTF(s, D_VERBOSE, "status = %d\n", ret);
+    DPRINTF(s, D_VERBOSE, "%s: status = %d\n", __func__, ret);
     return ret;
 }
 
@@ -756,7 +790,7 @@ static void ccid_write_data_block(USBCCIDState *s, uint8_t slot, uint8_t seq,
     p->b.bStatus = ccid_calc_status(s);
     p->b.bError = s->bError;
     if (p->b.bError) {
-        DPRINTF(s, D_VERBOSE, "error %d", p->b.bError);
+        DPRINTF(s, D_VERBOSE, "error %d\n", p->b.bError);
     }
     memcpy(p->abData, data, len);
     ccid_reset_error_status(s);
@@ -873,6 +907,28 @@ static void ccid_on_apdu_from_guest(USBCCIDState *s, CCID_XferBlock *recv)
     }
 }
 
+static const char *ccid_message_type_to_str(uint8_t type)
+{
+    switch (type) {
+    case CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOn: return "IccPowerOn";
+    case CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOff: return "IccPowerOff";
+    case CCID_MESSAGE_TYPE_PC_to_RDR_GetSlotStatus: return "GetSlotStatus";
+    case CCID_MESSAGE_TYPE_PC_to_RDR_XfrBlock: return "XfrBlock";
+    case CCID_MESSAGE_TYPE_PC_to_RDR_GetParameters: return "GetParameters";
+    case CCID_MESSAGE_TYPE_PC_to_RDR_ResetParameters: return "ResetParameters";
+    case CCID_MESSAGE_TYPE_PC_to_RDR_SetParameters: return "SetParameters";
+    case CCID_MESSAGE_TYPE_PC_to_RDR_Escape: return "Escape";
+    case CCID_MESSAGE_TYPE_PC_to_RDR_IccClock: return "IccClock";
+    case CCID_MESSAGE_TYPE_PC_to_RDR_T0APDU: return "T0APDU";
+    case CCID_MESSAGE_TYPE_PC_to_RDR_Secure: return "Secure";
+    case CCID_MESSAGE_TYPE_PC_to_RDR_Mechanical: return "Mechanical";
+    case CCID_MESSAGE_TYPE_PC_to_RDR_Abort: return "Abort";
+    case CCID_MESSAGE_TYPE_PC_to_RDR_SetDataRateAndClockFrequency:
+        return "SetDataRateAndClockFrequency";
+    }
+    return "unknown";
+}
+
 static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p)
 {
     CCID_Header *ccid_header;
@@ -895,13 +951,15 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p)
                 "%s: bad USB_TOKEN_OUT length, should be at least 10 bytes\n",
                 __func__);
     } else {
-        DPRINTF(s, D_MORE_INFO, "%s %x\n", __func__, ccid_header->bMessageType);
+        DPRINTF(s, D_MORE_INFO, "%s %x %s\n", __func__,
+                ccid_header->bMessageType,
+                ccid_message_type_to_str(ccid_header->bMessageType));
         switch (ccid_header->bMessageType) {
         case CCID_MESSAGE_TYPE_PC_to_RDR_GetSlotStatus:
             ccid_write_slot_status(s, ccid_header);
             break;
         case CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOn:
-            DPRINTF(s, 1, "PowerOn: %d\n",
+            DPRINTF(s, 1, "%s: PowerOn: %d\n", __func__,
                 ((CCID_IccPowerOn *)(ccid_header))->bPowerSelect);
             s->powered = true;
             if (!ccid_card_inserted(s)) {
@@ -911,7 +969,6 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p)
             ccid_write_data_block_atr(s, ccid_header);
             break;
         case CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOff:
-            DPRINTF(s, 1, "PowerOff\n");
             ccid_reset_error_status(s);
             s->powered = false;
             ccid_write_slot_status(s, ccid_header);
-- 
1.8.1.4

[Qemu-devel] [PATCH 16/26] hw/usb/dev-smartcard-reader.c: remove aborts (never triggered, but just in case)

[Alon Levy]

From: Alon Levy <alon@pobox.com>

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/usb/dev-smartcard-reader.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 4ff0641..8f69a55 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -796,6 +796,12 @@ static void ccid_write_data_block(USBCCIDState *s, uint8_t slot, uint8_t seq,
     ccid_reset_error_status(s);
 }
 
+static void ccid_report_error_failed(USBCCIDState *s, uint8_t error)
+{
+    s->bmCommandStatus = COMMAND_STATUS_FAILED;
+    s->bError = error;
+}
+
 static void ccid_write_data_block_answer(USBCCIDState *s,
     const uint8_t *data, uint32_t len)
 {
@@ -803,7 +809,9 @@ static void ccid_write_data_block_answer(USBCCIDState *s,
     uint8_t slot;
 
     if (!ccid_has_pending_answers(s)) {
-        abort();
+        DPRINTF(s, D_WARN, "error: no pending answer to return to guest\n");
+        ccid_report_error_failed(s, ERROR_ICC_MUTE);
+        return;
     }
     ccid_remove_pending_answer(s, &slot, &seq);
     ccid_write_data_block(s, slot, seq, data, len);
@@ -857,12 +865,6 @@ static void ccid_reset_parameters(USBCCIDState *s)
    memcpy(s->abProtocolDataStructure, abDefaultProtocolDataStructure, len);
 }
 
-static void ccid_report_error_failed(USBCCIDState *s, uint8_t error)
-{
-    s->bmCommandStatus = COMMAND_STATUS_FAILED;
-    s->bError = error;
-}
-
 /* NOTE: only a single slot is supported (SLOT_0) */
 static void ccid_on_slot_change(USBCCIDState *s, bool full)
 {
@@ -1129,7 +1131,9 @@ void ccid_card_send_apdu_to_guest(CCIDCardState *card,
     s->bmCommandStatus = COMMAND_STATUS_NO_ERROR;
     answer = ccid_peek_next_answer(s);
     if (answer == NULL) {
-        abort();
+        DPRINTF(s, D_WARN, "%s: error: unexpected lack of answer\n", __func__);
+        ccid_report_error_failed(s, ERROR_HW_ERROR);
+        return;
     }
     DPRINTF(s, 1, "APDU returned to guest %d (answer seq %d, slot %d)\n",
         len, answer->seq, answer->slot);
-- 
1.8.1.4

[Qemu-devel] [PATCH 17/26] hw/usb/dev-smartcard-reader.c: define structs for CCID_Parameter internals

[Alon Levy]

From: Alon Levy <alon@pobox.com>

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/usb/dev-smartcard-reader.c | 76 +++++++++++++++++++++++++++----------------
 1 file changed, 48 insertions(+), 28 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 8f69a55..6894f36 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -195,10 +195,34 @@ typedef struct QEMU_PACKED CCID_SlotStatus {
     uint8_t     bClockStatus;
 } CCID_SlotStatus;
 
+typedef struct QEMU_PACKED CCID_T0ProtocolDataStructure {
+    uint8_t     bmFindexDindex;
+    uint8_t     bmTCCKST0;
+    uint8_t     bGuardTimeT0;
+    uint8_t     bWaitingIntegerT0;
+    uint8_t     bClockStop;
+} CCID_T0ProtocolDataStructure;
+
+typedef struct QEMU_PACKED CCID_T1ProtocolDataStructure {
+    uint8_t     bmFindexDindex;
+    uint8_t     bmTCCKST1;
+    uint8_t     bGuardTimeT1;
+    uint8_t     bWaitingIntegerT1;
+    uint8_t     bClockStop;
+    uint8_t     bIFSC;
+    uint8_t     bNadValue;
+} CCID_T1ProtocolDataStructure;
+
+typedef union CCID_ProtocolDataStructure {
+    CCID_T0ProtocolDataStructure t0;
+    CCID_T1ProtocolDataStructure t1;
+    uint8_t data[7]; /* must be = max(sizeof(t0), sizeof(t1)) */
+} CCID_ProtocolDataStructure;
+
 typedef struct QEMU_PACKED CCID_Parameter {
     CCID_BULK_IN b;
     uint8_t     bProtocolNum;
-    uint8_t     abProtocolDataStructure[0];
+    CCID_ProtocolDataStructure abProtocolDataStructure;
 } CCID_Parameter;
 
 typedef struct QEMU_PACKED CCID_DataBlock {
@@ -230,7 +254,7 @@ typedef struct QEMU_PACKED CCID_SetParameters {
     CCID_Header hdr;
     uint8_t     bProtocolNum;
     uint16_t   abRFU;
-    uint8_t    abProtocolDataStructure[0];
+    CCID_ProtocolDataStructure abProtocolDataStructure;
 } CCID_SetParameters;
 
 typedef struct CCID_Notify_Slot_Change {
@@ -260,8 +284,6 @@ typedef struct CCIDBus {
     BusState qbus;
 } CCIDBus;
 
-#define MAX_PROTOCOL_SIZE   7
-
 /*
  * powered - defaults to true, changed by PowerOn/PowerOff messages
  */
@@ -285,7 +307,7 @@ typedef struct USBCCIDState {
     uint8_t  bError;
     uint8_t  bmCommandStatus;
     uint8_t  bProtocolNum;
-    uint8_t  abProtocolDataStructure[MAX_PROTOCOL_SIZE];
+    CCID_ProtocolDataStructure abProtocolDataStructure;
     uint32_t ulProtocolDataStructureSize;
     uint32_t state_vmstate;
     uint32_t migration_target_ip;
@@ -771,7 +793,7 @@ static void ccid_write_parameters(USBCCIDState *s, CCID_Header *recv)
     h->b.bStatus = ccid_calc_status(s);
     h->b.bError = s->bError;
     h->bProtocolNum = s->bProtocolNum;
-    memcpy(h->abProtocolDataStructure, s->abProtocolDataStructure, len);
+    h->abProtocolDataStructure = s->abProtocolDataStructure;
     ccid_reset_error_status(s);
 }
 
@@ -831,38 +853,36 @@ static void ccid_write_data_block_atr(USBCCIDState *s, CCID_Header *recv)
 static void ccid_set_parameters(USBCCIDState *s, CCID_Header *recv)
 {
     CCID_SetParameters *ph = (CCID_SetParameters *) recv;
-    uint32_t len = 0;
-    if ((ph->bProtocolNum & 3) == 0) {
-        len = 5;
-    }
-    if ((ph->bProtocolNum & 3) == 1) {
-        len = 7;
-    }
-    if (len == 0) {
-        s->bmCommandStatus = COMMAND_STATUS_FAILED;
-        s->bError = 7; /* Protocol invalid or not supported */
+    uint32_t protocol_num = ph->bProtocolNum & 3;
+
+    if (protocol_num != 0 && protocol_num != 1) {
+        ccid_report_error_failed(s, ERROR_CMD_NOT_SUPPORTED);
         return;
     }
-    s->bProtocolNum = ph->bProtocolNum;
-    memcpy(s->abProtocolDataStructure, ph->abProtocolDataStructure, len);
-    s->ulProtocolDataStructureSize = len;
-    DPRINTF(s, 1, "%s: using len %d\n", __func__, len);
+    s->bProtocolNum = protocol_num;
+    s->abProtocolDataStructure = ph->abProtocolDataStructure;
 }
 
 /*
  * must be 5 bytes for T=0, 7 bytes for T=1
  * See page 52
  */
-static const uint8_t abDefaultProtocolDataStructure[7] = {
-    0x77, 0x00, 0x00, 0x00, 0x00, 0xfe /*IFSC*/, 0x00 /*NAD*/ };
+static const CCID_ProtocolDataStructure defaultProtocolDataStructure = {
+    .t1 = {
+        .bmFindexDindex = 0x77,
+        .bmTCCKST1 = 0x00,
+        .bGuardTimeT1 = 0x00,
+        .bWaitingIntegerT1 = 0x00,
+        .bClockStop = 0x00,
+        .bIFSC = 0xfe,
+        .bNadValue = 0x00,
+    }
+};
 
 static void ccid_reset_parameters(USBCCIDState *s)
 {
-   uint32_t len = sizeof(abDefaultProtocolDataStructure);
-
-   s->bProtocolNum = 1; /* T=1 */
-   s->ulProtocolDataStructureSize = len;
-   memcpy(s->abProtocolDataStructure, abDefaultProtocolDataStructure, len);
+   s->bProtocolNum = 0; /* T=0 */
+   s->abProtocolDataStructure = defaultProtocolDataStructure;
 }
 
 /* NOTE: only a single slot is supported (SLOT_0) */
@@ -1366,7 +1386,7 @@ static VMStateDescription ccid_vmstate = {
         VMSTATE_UINT8(bError, USBCCIDState),
         VMSTATE_UINT8(bmCommandStatus, USBCCIDState),
         VMSTATE_UINT8(bProtocolNum, USBCCIDState),
-        VMSTATE_BUFFER(abProtocolDataStructure, USBCCIDState),
+        VMSTATE_BUFFER(abProtocolDataStructure.data, USBCCIDState),
         VMSTATE_UINT32(ulProtocolDataStructureSize, USBCCIDState),
         VMSTATE_STRUCT_ARRAY(bulk_in_pending, USBCCIDState,
                        BULK_IN_PENDING_NUM, 1, bulk_in_vmstate, BulkIn),
-- 
1.8.1.4

[Qemu-devel] [PATCH 18/26] hw/usb/dev-smartcard-reader.c: copy atr's protocol to ccid's parameters (adds todo's)

[Alon Levy]

From: Alon Levy <alon@pobox.com>

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/usb/dev-smartcard-reader.c | 46 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 6894f36..522fefa 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -839,14 +839,60 @@ static void ccid_write_data_block_answer(USBCCIDState *s,
     ccid_write_data_block(s, slot, seq, data, len);
 }
 
+static uint8_t atr_get_protocol_num(const uint8_t *atr, uint32_t len)
+{
+    int i;
+
+    if (len < 2 || !(atr[1] & 0x80)) {
+        /* too short or TD1 not included */
+        return 0; /* T=0, default */
+    }
+    i = 1 + !!(atr[1] & 0x10) + !!(atr[1] & 0x20) + !!(atr[1] & 0x40);
+    i += !!(atr[1] & 0x80);
+    return atr[i] & 0x0f;
+}
+
 static void ccid_write_data_block_atr(USBCCIDState *s, CCID_Header *recv)
 {
     const uint8_t *atr = NULL;
     uint32_t len = 0;
+    uint8_t atr_protocol_num;
+    CCID_T0ProtocolDataStructure *t0 = &s->abProtocolDataStructure.t0;
+    CCID_T1ProtocolDataStructure *t1 = &s->abProtocolDataStructure.t1;
 
     if (s->card) {
         atr = ccid_card_get_atr(s->card, &len);
     }
+    atr_protocol_num = atr_get_protocol_num(atr, len);
+    DPRINTF(s, D_VERBOSE, "%s: atr contains protocol=%d\n", __func__,
+            atr_protocol_num);
+    /* set parameters from ATR - see spec page 109 */
+    s->bProtocolNum = (atr_protocol_num <= 1 ? atr_protocol_num
+                                             : s->bProtocolNum);
+    if (atr_protocol_num == 0) {
+        DPRINTF(s, D_WARN, "%s: error: unimplemented ATR T0 parameters"
+                " setting\n", __func__);
+        t0->bmFindexDindex = 0;
+        t0->bmTCCKST0 = 0;
+        t0->bGuardTimeT0 = 0;
+        t0->bWaitingIntegerT0 = 0;
+        t0->bClockStop = 0;
+    } else {
+        if (atr_protocol_num != 1) {
+            DPRINTF(s, D_WARN, "%s: error: unsupported ATR protocol %d\n",
+                    __func__, atr_protocol_num);
+        } else {
+            DPRINTF(s, D_WARN, "%s: error: unimplemented ATR T1 parameters"
+                    " setting\n", __func__);
+            t1->bmFindexDindex = 0;
+            t1->bmTCCKST1 = 0;
+            t1->bGuardTimeT1 = 0;
+            t1->bWaitingIntegerT1 = 0;
+            t1->bClockStop = 0;
+            t1->bIFSC = 0;
+            t1->bNadValue = 0;
+        }
+    }
     ccid_write_data_block(s, recv->bSlot, recv->bSeq, atr, len);
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 19/26] hw/usb/dev-smartcard-reader.c: dwFeadvertise support for T=0 only

[Alon Levy]

From: Alon Levy <alon@pobox.com>

changes dwProtocols.PPPP to 0x1 and dwProtocols.RRRR=0 per spec.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/usb/dev-smartcard-reader.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 522fefa..7b319b1 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -341,8 +341,8 @@ static const uint8_t qemu_ccid_descriptor[] = {
                      */
         0x07,       /* u8  bVoltageSupport; 01h - 5.0v, 02h - 3.0, 03 - 1.8 */
 
-        0x03, 0x00, /* u32 dwProtocols; RRRR PPPP. RRRR = 0000h.*/
-        0x00, 0x00, /* PPPP: 0001h = Protocol T=0, 0002h = Protocol T=1 */
+        0x00, 0x00, /* u32 dwProtocols; RRRR PPPP. RRRR = 0000h.*/
+        0x01, 0x00, /* PPPP: 0001h = Protocol T=0, 0002h = Protocol T=1 */
                     /* u32 dwDefaultClock; in kHZ (0x0fa0 is 4 MHz) */
         0xa0, 0x0f, 0x00, 0x00,
                     /* u32 dwMaximumClock; */
-- 
1.8.1.4

[Qemu-devel] [PATCH 20/26] hw/usb/dev-smartcard-reader: support windows guest

[Alon Levy]

From: Alon Levy <alon@pobox.com>

By not advertising USB wakeup support (which we don't).

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/usb/dev-smartcard-reader.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 7b319b1..48bd519 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -381,11 +381,11 @@ static const uint8_t qemu_ccid_descriptor[] = {
                      * 20000 Short APDU level exchange with CCID
                      * 40000 Short and Extended APDU level exchange with CCID
                      *
-                     * + 100000 USB Wake up signaling supported on card
+                     * 100000 USB Wake up signaling supported on card
                      * insertion and removal. Must set bit 5 in bmAttributes
                      * in Configuration descriptor if 100000 is set.
                      */
-        0xfe, 0x04, 0x11, 0x00,
+        0xfe, 0x04, 0x01, 0x00,
                     /*
                      * u32 dwMaxCCIDMessageLength; For extended APDU in
                      * [261 + 10 , 65544 + 10]. Otherwise the minimum is
-- 
1.8.1.4

[Qemu-devel] [PATCH 21/26] dev-smartcard-reader: reuse usb.h definitions

[Alon Levy]

From: Alon Levy <alon@pobox.com>

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/usb/dev-smartcard-reader.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 48bd519..3246fc1 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -68,12 +68,6 @@ do { \
 #define BULK_IN_BUF_SIZE 384
 #define BULK_IN_PENDING_NUM 8
 
-#define InterfaceOutClass \
-    ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE)<<8)
-
-#define InterfaceInClass  \
-    ((USB_DIR_IN  | USB_TYPE_CLASS | USB_RECIP_INTERFACE)<<8)
-
 #define CCID_MAX_PACKET_SIZE                64
 
 #define CCID_CONTROL_ABORT                  0x1
@@ -432,8 +426,8 @@ static const USBDescStrings desc_strings = {
 static const USBDescIface desc_iface0 = {
     .bInterfaceNumber              = 0,
     .bNumEndpoints                 = 3,
-    .bInterfaceClass               = 0x0b,
-    .bInterfaceSubClass            = 0x00,
+    .bInterfaceClass               = USB_CLASS_CSCID,
+    .bInterfaceSubClass            = USB_SUBCLASS_UNDEFINED,
     .bInterfaceProtocol            = 0x00,
     .iInterface                    = STR_INTERFACE,
     .ndesc                         = 1,
@@ -709,15 +703,15 @@ static void ccid_handle_control(USBDevice *dev, USBPacket *p, int request,
 
     switch (request) {
         /* Class specific requests.  */
-    case InterfaceOutClass | CCID_CONTROL_ABORT:
+    case ClassInterfaceOutRequest | CCID_CONTROL_ABORT:
         DPRINTF(s, 1, "ccid_control abort UNIMPLEMENTED\n");
         p->status = USB_RET_STALL;
         break;
-    case InterfaceInClass | CCID_CONTROL_GET_CLOCK_FREQUENCIES:
+    case ClassInterfaceRequest | CCID_CONTROL_GET_CLOCK_FREQUENCIES:
         DPRINTF(s, 1, "ccid_control get clock frequencies UNIMPLEMENTED\n");
         p->status = USB_RET_STALL;
         break;
-    case InterfaceInClass | CCID_CONTROL_GET_DATA_RATES:
+    case ClassInterfaceRequest | CCID_CONTROL_GET_DATA_RATES:
         DPRINTF(s, 1, "ccid_control get data rates UNIMPLEMENTED\n");
         p->status = USB_RET_STALL;
         break;
-- 
1.8.1.4

[Qemu-devel] [PATCH 22/26] libcacard/vreader: add debugging messages for apdu

[Alon Levy]

From: Alon Levy <alon@pobox.com>

controllable by environment variable LIBCACARD_DEBUG.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 libcacard/cac.c     |  7 -----
 libcacard/cac.h     |  8 +++++
 libcacard/vreader.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+), 7 deletions(-)

diff --git a/libcacard/cac.c b/libcacard/cac.c
index 927a4ca..5864539 100644
--- a/libcacard/cac.c
+++ b/libcacard/cac.c
@@ -12,13 +12,6 @@
 #include "vcard_emul.h"
 #include "card_7816.h"
 
-#define CAC_GET_PROPERTIES  0x56
-#define CAC_GET_ACR         0x4c
-#define CAC_READ_BUFFER     0x52
-#define CAC_UPDATE_BUFFER   0x58
-#define CAC_SIGN_DECRYPT    0x42
-#define CAC_GET_CERTIFICATE 0x36
-
 /* private data for PKI applets */
 typedef struct CACPKIAppletDataStruct {
     unsigned char *cert;
diff --git a/libcacard/cac.h b/libcacard/cac.h
index 15a61be..d24a2a8 100644
--- a/libcacard/cac.h
+++ b/libcacard/cac.h
@@ -9,6 +9,14 @@
 #define CAC_H 1
 #include "vcard.h"
 #include "vreader.h"
+
+#define CAC_GET_PROPERTIES  0x56
+#define CAC_GET_ACR         0x4c
+#define CAC_READ_BUFFER     0x52
+#define CAC_UPDATE_BUFFER   0x58
+#define CAC_SIGN_DECRYPT    0x42
+#define CAC_GET_CERTIFICATE 0x36
+
 /*
  * Initialize the cac card. This is the only public function in this file. All
  * the rest are connected through function pointers.
diff --git a/libcacard/vreader.c b/libcacard/vreader.c
index f3efc27..3ab785e 100644
--- a/libcacard/vreader.c
+++ b/libcacard/vreader.c
@@ -13,6 +13,7 @@
 #include "card_7816.h"
 #include "vreader.h"
 #include "vevent.h"
+#include "cac.h" /* just for debugging defines */
 
 struct VReaderStruct {
     int    reference_count;
@@ -24,6 +25,86 @@ struct VReaderStruct {
     VReaderEmulFree reader_private_free;
 };
 
+/*
+ * Debug helpers
+ */
+
+static void debug(const char *fmt, ...)
+{
+    static int debug = -1;
+    static int debug_inited;
+    va_list ap;
+
+    if (!debug_inited) {
+        debug = getenv("LIBCACARD_DEBUG") ? atoi(getenv("LIBCACARD_DEBUG")) : 0;
+        debug_inited = 1;
+    }
+
+    if (debug <= 0) {
+        return;
+    }
+
+    va_start(ap, fmt);
+    vfprintf(stderr, fmt, ap);
+    va_end(ap);
+}
+
+static const char *
+apdu_ins_to_string(int ins)
+{
+    switch (ins) {
+    case VCARD7816_INS_MANAGE_CHANNEL:
+        return "manage channel";
+    case VCARD7816_INS_EXTERNAL_AUTHENTICATE:
+        return "external authenticate";
+    case VCARD7816_INS_GET_CHALLENGE:
+        return "get challenge";
+    case VCARD7816_INS_INTERNAL_AUTHENTICATE:
+        return "internal authenticate";
+    case VCARD7816_INS_ERASE_BINARY:
+        return "erase binary";
+    case VCARD7816_INS_READ_BINARY:
+        return "read binary";
+    case VCARD7816_INS_WRITE_BINARY:
+        return "write binary";
+    case VCARD7816_INS_UPDATE_BINARY:
+        return "update binary";
+    case VCARD7816_INS_READ_RECORD:
+        return "read record";
+    case VCARD7816_INS_WRITE_RECORD:
+        return "write record";
+    case VCARD7816_INS_UPDATE_RECORD:
+        return "update record";
+    case VCARD7816_INS_APPEND_RECORD:
+        return "append record";
+    case VCARD7816_INS_ENVELOPE:
+        return "envelope";
+    case VCARD7816_INS_PUT_DATA:
+        return "put data";
+    case VCARD7816_INS_GET_DATA:
+        return "get data";
+    case VCARD7816_INS_SELECT_FILE:
+        return "select file";
+    case VCARD7816_INS_VERIFY:
+        return "verify";
+    case VCARD7816_INS_GET_RESPONSE:
+        return "get response";
+    case CAC_GET_PROPERTIES:
+        return "get properties";
+    case CAC_GET_ACR:
+        return "get acr";
+    case CAC_READ_BUFFER:
+        return "read buffer";
+    case CAC_UPDATE_BUFFER:
+        return "update buffer";
+    case CAC_SIGN_DECRYPT:
+        return "sign decrypt";
+    case CAC_GET_CERTIFICATE:
+        return "get certificate";
+    }
+    return "unknown";
+}
+
 /* manage locking */
 static inline void
 vreader_lock(VReader *reader)
@@ -204,7 +285,15 @@ vreader_xfr_bytes(VReader *reader,
         response = vcard_make_response(status);
         card_status = VCARD_DONE;
     } else {
+        debug("%s: CLS=0x%x,INS=0x%x,P1=0x%x,P2=0x%x,Lc=%d,Le=%d %s\n",
+              __func__, apdu->a_cla, apdu->a_ins, apdu->a_p1, apdu->a_p2,
+              apdu->a_Lc, apdu->a_Le, apdu_ins_to_string(apdu->a_ins));
         card_status = vcard_process_apdu(card, apdu, &response);
+        if (response) {
+            debug("%s: status=%d sw1=0x%x sw2=0x%x len=%d (total=%d)\n",
+                  __func__, response->b_status, response->b_sw1,
+                  response->b_sw2, response->b_len, response->b_total_len);
+        }
     }
     assert(card_status == VCARD_DONE);
     if (card_status == VCARD_DONE) {
-- 
1.8.1.4

[Qemu-devel] [PATCH 23/26] libcacard: change default ATR

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vcardt.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/libcacard/vcardt.h b/libcacard/vcardt.h
index 538bdde..3b9a619 100644
--- a/libcacard/vcardt.h
+++ b/libcacard/vcardt.h
@@ -26,9 +26,17 @@ typedef struct VCardEmulStruct VCardEmul;
 #define MAX_CHANNEL 4
 
 /* create an ATR with appropriate historical bytes */
-#define VCARD_ATR_PREFIX(size) 0x3b, 0x66+(size), 0x00, 0xff, \
-                               'V', 'C', 'A', 'R', 'D', '_'
+#define TS_DIRECT_CONVENTION 0x3b
+#define TA_PRESENT 0x10
+#define TB_PRESENT 0x20
+#define TC_PRESENT 0x40
+#define TD_PRESENT 0x80
 
+#define VCARD_ATR_PREFIX(size) \
+    TS_DIRECT_CONVENTION, \
+    TD_PRESENT + (6 + size), \
+    0x00, \
+    'V', 'C', 'A', 'R', 'D', '_'
 
 typedef enum {
     VCARD_DONE,
-- 
1.8.1.4

[Qemu-devel] [PATCH 24/26] libcacard: move atr setting from macro to function

[Alon Levy]

Only because qemu's checkpatch complains about it.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 Makefile.objs              |  1 +
 libcacard/vcard_emul_nss.c | 11 ++++++++---
 libcacard/vcardt.c         | 40 ++++++++++++++++++++++++++++++++++++++++
 libcacard/vcardt.h         | 15 ++-------------
 4 files changed, 51 insertions(+), 16 deletions(-)
 create mode 100644 libcacard/vcardt.c

diff --git a/Makefile.objs b/Makefile.objs
index f99841c..6d47567 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -41,6 +41,7 @@ libcacard-y += libcacard/vcard.o libcacard/vreader.o
 libcacard-y += libcacard/vcard_emul_nss.o
 libcacard-y += libcacard/vcard_emul_type.o
 libcacard-y += libcacard/card_7816.o
+libcacard-y += libcacard/vcardt.o
 
 ######################################################################
 # Target independent part of system emulation. The long term path is to
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 21d4689..6bad0b9 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -519,18 +519,23 @@ vcard_emul_reader_get_slot(VReader *vreader)
 }
 
 /*
- *  Card ATR's map to physical cards. VCARD_ATR_PREFIX will set appropriate
+ *  Card ATR's map to physical cards. vcard_alloc_atr will set appropriate
  *  historical bytes for any software emulated card. The remaining bytes can be
  *  used to indicate the actual emulator
  */
-static const unsigned char nss_atr[] = { VCARD_ATR_PREFIX(3), 'N', 'S', 'S' };
+static unsigned char *nss_atr;
+static int nss_atr_len;
 
 void
 vcard_emul_get_atr(VCard *card, unsigned char *atr, int *atr_len)
 {
-    int len = MIN(sizeof(nss_atr), *atr_len);
+    int len;
     assert(atr != NULL);
 
+    if (nss_atr == NULL) {
+        nss_atr = vcard_alloc_atr("NSS", &nss_atr_len);
+    }
+    len = MIN(nss_atr_len, *atr_len);
     memcpy(atr, nss_atr, len);
     *atr_len = len;
 }
diff --git a/libcacard/vcardt.c b/libcacard/vcardt.c
new file mode 100644
index 0000000..f64c343
--- /dev/null
+++ b/libcacard/vcardt.c
@@ -0,0 +1,40 @@
+#include <stdlib.h>
+#include <string.h>
+#include <glib.h>
+
+#include "libcacard/vcardt.h"
+
+/* create an ATR with appropriate historical bytes */
+#define ATR_TS_DIRECT_CONVENTION 0x3b
+#define ATR_TA_PRESENT 0x10
+#define ATR_TB_PRESENT 0x20
+#define ATR_TC_PRESENT 0x40
+#define ATR_TD_PRESENT 0x80
+
+unsigned char *vcard_alloc_atr(const char *postfix, int *atr_len)
+{
+    int postfix_len;
+    const char prefix[] = "VCARD_";
+    const int prefix_len = strlen(prefix);
+    int total_len;
+    unsigned char *atr;
+
+    if (postfix == NULL) {
+        postfix_len = 0;
+    } else {
+        postfix_len = strlen(postfix);
+    }
+    total_len = 3 + prefix_len + postfix_len;
+    atr = g_malloc(total_len);
+    atr[0] = ATR_TS_DIRECT_CONVENTION;
+    atr[1] = ATR_TD_PRESENT + prefix_len + postfix_len;
+    atr[2] = 0x00;
+    memcpy(&atr[3], prefix, prefix_len);
+    if (postfix) {
+        memcpy(&atr[3 + prefix_len], postfix, postfix_len);
+    }
+    if (atr_len) {
+        *atr_len = total_len;
+    }
+    return atr;
+}
diff --git a/libcacard/vcardt.h b/libcacard/vcardt.h
index 3b9a619..e8e651f 100644
--- a/libcacard/vcardt.h
+++ b/libcacard/vcardt.h
@@ -25,19 +25,6 @@ typedef struct VCardEmulStruct VCardEmul;
 
 #define MAX_CHANNEL 4
 
-/* create an ATR with appropriate historical bytes */
-#define TS_DIRECT_CONVENTION 0x3b
-#define TA_PRESENT 0x10
-#define TB_PRESENT 0x20
-#define TC_PRESENT 0x40
-#define TD_PRESENT 0x80
-
-#define VCARD_ATR_PREFIX(size) \
-    TS_DIRECT_CONVENTION, \
-    TD_PRESENT + (6 + size), \
-    0x00, \
-    'V', 'C', 'A', 'R', 'D', '_'
-
 typedef enum {
     VCARD_DONE,
     VCARD_NEXT,
@@ -69,4 +56,6 @@ struct VCardBufferResponseStruct {
     int len;
 };
 
+unsigned char *vcard_alloc_atr(const char *postfix, int *atr_len);
+
 #endif
-- 
1.8.1.4

[Qemu-devel] [PATCH 25/26] dev-smartcard-reader: empty implementation for Mechanical (fail correctly)

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/usb/dev-smartcard-reader.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 3246fc1..f53956e 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1052,6 +1052,10 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p)
             ccid_reset_error_status(s);
             ccid_write_parameters(s, ccid_header);
             break;
+        case CCID_MESSAGE_TYPE_PC_to_RDR_Mechanical:
+            ccid_report_error_failed(s, 0);
+            ccid_write_slot_status(s, ccid_header);
+            break;
         default:
             DPRINTF(s, 1,
                 "handle_data: ERROR: unhandled message type %Xh\n",
-- 
1.8.1.4

[Qemu-devel] [PATCH 26/26] libcacard/cac.c: questionable change to single return of big switch functions

[Alon Levy]

From: Alon Levy <alon@pobox.com>

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 libcacard/cac.c | 73 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 21 deletions(-)

diff --git a/libcacard/cac.c b/libcacard/cac.c
index 5864539..7a06b5a 100644
--- a/libcacard/cac.c
+++ b/libcacard/cac.c
@@ -40,41 +40,51 @@ static VCardStatus
 cac_common_process_apdu(VCard *card, VCardAPDU *apdu, VCardResponse **response)
 {
     int ef;
+    VCardStatus ret = VCARD_FAIL;
 
     switch (apdu->a_ins) {
     case VCARD7816_INS_SELECT_FILE:
         if (apdu->a_p1 != 0x02) {
             /* let the 7816 code handle applet switches */
-            return VCARD_NEXT;
+            ret = VCARD_NEXT;
+            break;
         }
         /* handle file id setting */
         if (apdu->a_Lc != 2) {
             *response = vcard_make_response(
                 VCARD7816_STATUS_ERROR_DATA_INVALID);
-            return VCARD_DONE;
+            ret = VCARD_DONE;
+            break;
         }
         /* CAC 1.0 only supports ef = 0 */
         ef = apdu->a_body[0] | (apdu->a_body[1] << 8);
         if (ef != 0) {
             *response = vcard_make_response(
                 VCARD7816_STATUS_ERROR_FILE_NOT_FOUND);
-            return VCARD_DONE;
+            ret = VCARD_DONE;
+            break;
         }
         *response = vcard_make_response(VCARD7816_STATUS_SUCCESS);
-        return VCARD_DONE;
+        ret = VCARD_DONE;
+        break;
     case VCARD7816_INS_GET_RESPONSE:
     case VCARD7816_INS_VERIFY:
         /* let the 7816 code handle these */
-        return VCARD_NEXT;
+        ret = VCARD_NEXT;
+        break;
     case CAC_GET_PROPERTIES:
     case CAC_GET_ACR:
         /* skip these for now, this will probably be needed */
         *response = vcard_make_response(VCARD7816_STATUS_ERROR_P1_P2_INCORRECT);
-        return VCARD_DONE;
+        ret = VCARD_DONE;
+        break;
+    default:
+        *response = vcard_make_response(
+            VCARD7816_STATUS_ERROR_COMMAND_NOT_SUPPORTED);
+        ret = VCARD_DONE;
+        break;
     }
-    *response = vcard_make_response(
-        VCARD7816_STATUS_ERROR_COMMAND_NOT_SUPPORTED);
-    return VCARD_DONE;
+    return ret;
 }
 
 /*
@@ -108,6 +118,7 @@ cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu,
     int size, next;
     unsigned char *sign_buffer;
     vcard_7816_status_t status;
+    VCardStatus ret = VCARD_FAIL;
 
     applet_private = vcard_get_current_applet_private(card, apdu->a_channel);
     assert(applet_private);
@@ -117,7 +128,8 @@ cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu,
     case CAC_UPDATE_BUFFER:
         *response = vcard_make_response(
             VCARD7816_STATUS_ERROR_CONDITION_NOT_SATISFIED);
-        return VCARD_DONE;
+        ret = VCARD_DONE;
+        break;
     case CAC_GET_CERTIFICATE:
         if ((apdu->a_p2 != 0) || (apdu->a_p1 != 0)) {
             *response = vcard_make_response(
@@ -147,7 +159,8 @@ cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu,
             *response = vcard_make_response(
                             VCARD7816_STATUS_EXC_ERROR_MEMORY_FAILURE);
         }
-        return VCARD_DONE;
+        ret = VCARD_DONE;
+        break;
     case CAC_SIGN_DECRYPT:
         if (apdu->a_p2 != 0) {
             *response = vcard_make_response(
@@ -164,7 +177,8 @@ cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu,
             pki_applet->sign_buffer_len = 0;
             *response = vcard_make_response(
                             VCARD7816_STATUS_EXC_ERROR_MEMORY_FAILURE);
-            return VCARD_DONE;
+            ret = VCARD_DONE;
+            break;
         }
         memcpy(sign_buffer+pki_applet->sign_buffer_len, apdu->a_body, size);
         size += pki_applet->sign_buffer_len;
@@ -175,7 +189,8 @@ cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu,
             pki_applet->sign_buffer = sign_buffer;
             pki_applet->sign_buffer_len = size;
             *response = vcard_make_response(VCARD7816_STATUS_SUCCESS);
-            return VCARD_DONE;
+            ret = VCARD_DONE;
+            break;
         case 0x00:
             /* we now have the whole buffer, do the operation, result will be
              * in the sign_buffer */
@@ -200,15 +215,20 @@ cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu,
         g_free(sign_buffer);
         pki_applet->sign_buffer = NULL;
         pki_applet->sign_buffer_len = 0;
-        return VCARD_DONE;
+        ret = VCARD_DONE;
+        break;
     case CAC_READ_BUFFER:
         /* new CAC call, go ahead and use the old version for now */
         /* TODO: implement */
         *response = vcard_make_response(
                                 VCARD7816_STATUS_ERROR_COMMAND_NOT_SUPPORTED);
-        return VCARD_DONE;
+        ret = VCARD_DONE;
+        break;
+    default:
+        ret = cac_common_process_apdu(card, apdu, response);
+        break;
     }
-    return cac_common_process_apdu(card, apdu, response);
+    return ret;
 }
 
 
@@ -216,19 +236,26 @@ static VCardStatus
 cac_applet_id_process_apdu(VCard *card, VCardAPDU *apdu,
                            VCardResponse **response)
 {
+    VCardStatus ret = VCARD_FAIL;
+
     switch (apdu->a_ins) {
     case CAC_UPDATE_BUFFER:
         *response = vcard_make_response(
                         VCARD7816_STATUS_ERROR_CONDITION_NOT_SATISFIED);
-        return VCARD_DONE;
+        ret = VCARD_DONE;
+        break;
     case CAC_READ_BUFFER:
         /* new CAC call, go ahead and use the old version for now */
         /* TODO: implement */
         *response = vcard_make_response(
                         VCARD7816_STATUS_ERROR_COMMAND_NOT_SUPPORTED);
-        return VCARD_DONE;
+        ret = VCARD_DONE;
+        break;
+    default:
+        ret = cac_common_process_apdu(card, apdu, response);
+        break;
     }
-    return cac_common_process_apdu(card, apdu, response);
+    return ret;
 }
 
 
@@ -240,16 +267,20 @@ static VCardStatus
 cac_applet_container_process_apdu(VCard *card, VCardAPDU *apdu,
                                   VCardResponse **response)
 {
+    VCardStatus ret = VCARD_FAIL;
+
     switch (apdu->a_ins) {
     case CAC_READ_BUFFER:
     case CAC_UPDATE_BUFFER:
         *response = vcard_make_response(
                         VCARD7816_STATUS_ERROR_COMMAND_NOT_SUPPORTED);
-        return VCARD_DONE;
+        ret = VCARD_DONE;
+        break;
     default:
+        ret = cac_common_process_apdu(card, apdu, response);
         break;
     }
-    return cac_common_process_apdu(card, apdu, response);
+    return ret;
 }
 
 /*
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 00/26] ccid and libcacard fixes for windows/mingw

[Alon Levy]

> This series:
> 1. fixes windows guests to show the ccid device
> 2. changes libcacard to use glib
> 3. makes libcacard build under mingw
> 4. does some cleanups
5. adds three debugging environment variables: QEMU_CCID_PASSTHRU_DEBUG, QEMU_CCID_DEBUG and LIBCACARD_DEBUG
The reason for using those is that it makes it easier to use with libvirt, which already supports environment variables on qemu machines via an extended xml namespace.

Additionally, please ignore 26/26, I'll drop it, didn't mean to send it. (it's a nop anyway).

> 
> It contains a few patches already posted to the list (the two Jim
> Meyering patches) which were already acked.
> 
> I'll make a pull request once this had some time to be reviewed.
> 
> Tested with a fedora and windows 7 guest.
> 
> The main non cleanup patches are:
>   hw/usb/dev-smartcard-reader: support windows guest
>   libcacard: correct T0 historical bytes size
> 
> 
> Alon Levy (15):
>   hw/ccid-card-passthru.c: add atr check
>   ccid-card-passthru, dev-smartcard-reader: add debug environment
>     variables
>   hw/usb/dev-smartcard-reader.c: white space fixes
>   hw/usb/dev-smartcard-reader.c: nicer debug messages
>   hw/usb/dev-smartcard-reader.c: remove aborts (never triggered, but
>     just in case)
>   hw/usb/dev-smartcard-reader.c: define structs for CCID_Parameter
>     internals
>   hw/usb/dev-smartcard-reader.c: copy atr's protocol to ccid's
>     parameters (adds todo's)
>   hw/usb/dev-smartcard-reader.c: dwFeadvertise support for T=0 only
>   hw/usb/dev-smartcard-reader: support windows guest
>   dev-smartcard-reader: reuse usb.h definitions
>   libcacard/vreader: add debugging messages for apdu
>   libcacard: change default ATR
>   libcacard: move atr setting from macro to function
>   dev-smartcard-reader: empty implementation for Mechanical (fail
>     correctly)
>   libcacard/cac.c: questionable change to single return of big switch
>     functions
> 
> Jim Meyering (2):
>   ccid: make backend_enum_table "static const" and adjust users
>   ccid: declare DEFAULT_ATR table to be "static const"
> 
> Marc-André Lureau (9):
>   libcacard: correct T0 historical bytes size
>   ccid-card-emul: do not crash if backend is not provided
>   libcacard: use system config directory for nss db on win32
>   util: move socket_init() to osdep.c
>   build-sys: must link with -fstack-protector
>   libcacard: fix mingw64 cross-compilation
>   libcacard: split vscclient main() from socket reading
>   libcacard: vscclient to use QemuThread for portability
>   libcacard: teach vscclient to use GMainLoop for portability
> 
>  Makefile                      |   8 +-
>  Makefile.objs                 |   1 +
>  configure                     |   8 +-
>  hw/ccid-card-emulated.c       |   9 +-
>  hw/ccid-card-passthru.c       |  77 ++++++-
>  hw/usb/dev-smartcard-reader.c | 259 ++++++++++++++++-----
>  libcacard/cac.c               |  80 ++++---
>  libcacard/cac.h               |   8 +
>  libcacard/vcard_emul_nss.c    |  29 ++-
>  libcacard/vcardt.c            |  40 ++++
>  libcacard/vcardt.h            |   7 +-
>  libcacard/vreader.c           |  89 ++++++++
>  libcacard/vscclient.c         | 506
>  +++++++++++++++++++++++++-----------------
>  rules.mak                     |   4 +-
>  util/osdep.c                  |  23 ++
>  util/qemu-sockets.c           |  24 --
>  16 files changed, 846 insertions(+), 326 deletions(-)
>  create mode 100644 libcacard/vcardt.c
> 
> --
> 1.8.1.4
> 
> 
> 

Re: [Qemu-devel] [PATCH 08/26] libcacard: fix mingw64 cross-compilation

[Paolo Bonzini]

Il 18/03/2013 14:10, Alon Levy ha scritto:
> From: Marc-André Lureau <marcandre.lureau@gmail.com>
> 
> Compile and link with version.lo
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  Makefile  | 8 ++++++--
>  rules.mak | 3 ++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 6915178..10abfee 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -151,11 +151,15 @@ recurse-all: $(SUBDIR_RULES) $(ROMSUBDIR_RULES)
>  
>  bt-host.o: QEMU_CFLAGS += $(BLUEZ_CFLAGS)
>  
> -version.o: $(SRC_PATH)/version.rc config-host.h
> +version.o: $(SRC_PATH)/version.rc config-host.h | version.lo
>  	$(call quiet-command,$(WINDRES) -I. -o $@ $<,"  RC    $(TARGET_DIR)$@")
> +version.lo: $(SRC_PATH)/version.rc config-host.h
> +	$(call quiet-command,$(LIBTOOL) --mode=compile --tag=RC $(WINDRES) -I. -o $@ $<,"lt RC   $(TARGET_DIR)$@")
>  
>  version-obj-$(CONFIG_WIN32) += version.o
> -Makefile: $(version-obj-y)
> +version-lobj-$(CONFIG_WIN32) += $(if $(LIBTOOL),version.lo)
> +Makefile: $(version-obj-y) $(version-lobj-y)
> +
>  
>  ######################################################################
>  # Build libraries
> diff --git a/rules.mak b/rules.mak
> index 36aba2d..292a422 100644
> --- a/rules.mak
> +++ b/rules.mak
> @@ -35,7 +35,8 @@ LIBTOOL += $(if $(V),,--quiet)
>  LINK = $(call quiet-command,\
>         $(if $(filter %.lo %.la,$^),$(LIBTOOL) --mode=link --tag=CC \
>         )$(CC) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ \
> -       $(sort $(filter %.o, $1)) $(filter-out %.o, $1) $(version-obj-y) \
> +       $(sort $(filter %.o, $1)) $(filter-out %.o, $1) \
> +       $(if $(filter %.lo %.la,$^),$(version-lobj-y),$(version-obj-y)) \
>         $(if $(filter %.lo %.la,$^),$(LIBTOOLFLAGS)) \
>         $(LIBS),$(if $(filter %.lo %.la,$^),"lt LINK ", "  LINK  ")"$(TARGET_DIR)$@")
>  endif
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 26/26] libcacard/cac.c: questionable change to single return of big switch functions

[Marc-André Lureau]

(that one was nack by you in 00/26, but it's a nice clean up anyway)

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 25/26] dev-smartcard-reader: empty implementation for Mechanical (fail correctly)

[Marc-André Lureau]

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 24/26] libcacard: move atr setting from macro to function

[Marc-André Lureau]

Hi,

review below

On Mon, Mar 18, 2013 at 2:11 PM, Alon Levy <alevy@redhat.com> wrote:
> Only because qemu's checkpatch complains about it.
>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  Makefile.objs              |  1 +
>  libcacard/vcard_emul_nss.c | 11 ++++++++---
>  libcacard/vcardt.c         | 40 ++++++++++++++++++++++++++++++++++++++++
>  libcacard/vcardt.h         | 15 ++-------------
>  4 files changed, 51 insertions(+), 16 deletions(-)
>  create mode 100644 libcacard/vcardt.c
>
> diff --git a/Makefile.objs b/Makefile.objs
> index f99841c..6d47567 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -41,6 +41,7 @@ libcacard-y += libcacard/vcard.o libcacard/vreader.o
>  libcacard-y += libcacard/vcard_emul_nss.o
>  libcacard-y += libcacard/vcard_emul_type.o
>  libcacard-y += libcacard/card_7816.o
> +libcacard-y += libcacard/vcardt.o
>
>  ######################################################################
>  # Target independent part of system emulation. The long term path is to
> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
> index 21d4689..6bad0b9 100644
> --- a/libcacard/vcard_emul_nss.c
> +++ b/libcacard/vcard_emul_nss.c
> @@ -519,18 +519,23 @@ vcard_emul_reader_get_slot(VReader *vreader)
>  }
>
>  /*
> - *  Card ATR's map to physical cards. VCARD_ATR_PREFIX will set appropriate
> + *  Card ATR's map to physical cards. vcard_alloc_atr will set appropriate
>   *  historical bytes for any software emulated card. The remaining bytes can be
>   *  used to indicate the actual emulator
>   */
> -static const unsigned char nss_atr[] = { VCARD_ATR_PREFIX(3), 'N', 'S', 'S' };
> +static unsigned char *nss_atr;
> +static int nss_atr_len;
>
>  void
>  vcard_emul_get_atr(VCard *card, unsigned char *atr, int *atr_len)
>  {
> -    int len = MIN(sizeof(nss_atr), *atr_len);
> +    int len;
>      assert(atr != NULL);
>
> +    if (nss_atr == NULL) {
> +        nss_atr = vcard_alloc_atr("NSS", &nss_atr_len);
> +    }
> +    len = MIN(nss_atr_len, *atr_len);
>      memcpy(atr, nss_atr, len);
>      *atr_len = len;
>  }
> diff --git a/libcacard/vcardt.c b/libcacard/vcardt.c
> new file mode 100644
> index 0000000..f64c343
> --- /dev/null
> +++ b/libcacard/vcardt.c
> @@ -0,0 +1,40 @@
> +#include <stdlib.h>
> +#include <string.h>
> +#include <glib.h>
> +
> +#include "libcacard/vcardt.h"
> +
> +/* create an ATR with appropriate historical bytes */
> +#define ATR_TS_DIRECT_CONVENTION 0x3b
> +#define ATR_TA_PRESENT 0x10
> +#define ATR_TB_PRESENT 0x20
> +#define ATR_TC_PRESENT 0x40
> +#define ATR_TD_PRESENT 0x80
> +
> +unsigned char *vcard_alloc_atr(const char *postfix, int *atr_len)
> +{
> +    int postfix_len;
> +    const char prefix[] = "VCARD_";
> +    const int prefix_len = strlen(prefix);

or sizeof(prefix) -1

> +    int total_len;
> +    unsigned char *atr;
> +
> +    if (postfix == NULL) {

I would make postfix mandatory.

> +        postfix_len = 0;
> +    } else {
> +        postfix_len = strlen(postfix);
> +    }
> +    total_len = 3 + prefix_len + postfix_len;
> +    atr = g_malloc(total_len);
> +    atr[0] = ATR_TS_DIRECT_CONVENTION;
> +    atr[1] = ATR_TD_PRESENT + prefix_len + postfix_len;
> +    atr[2] = 0x00;
> +    memcpy(&atr[3], prefix, prefix_len);
> +    if (postfix) {
> +        memcpy(&atr[3 + prefix_len], postfix, postfix_len);
> +    }
> +    if (atr_len) {
> +        *atr_len = total_len;
> +    }
> +    return atr;
> +}
> diff --git a/libcacard/vcardt.h b/libcacard/vcardt.h
> index 3b9a619..e8e651f 100644
> --- a/libcacard/vcardt.h
> +++ b/libcacard/vcardt.h
> @@ -25,19 +25,6 @@ typedef struct VCardEmulStruct VCardEmul;
>
>  #define MAX_CHANNEL 4
>
> -/* create an ATR with appropriate historical bytes */
> -#define TS_DIRECT_CONVENTION 0x3b
> -#define TA_PRESENT 0x10
> -#define TB_PRESENT 0x20
> -#define TC_PRESENT 0x40
> -#define TD_PRESENT 0x80
> -
> -#define VCARD_ATR_PREFIX(size) \
> -    TS_DIRECT_CONVENTION, \
> -    TD_PRESENT + (6 + size), \
> -    0x00, \
> -    'V', 'C', 'A', 'R', 'D', '_'
> -
>  typedef enum {
>      VCARD_DONE,
>      VCARD_NEXT,
> @@ -69,4 +56,6 @@ struct VCardBufferResponseStruct {
>      int len;
>  };
>
> +unsigned char *vcard_alloc_atr(const char *postfix, int *atr_len);
> +

If the function is exposed in public header, it should also be
exported (in libcacard.syms), but I am not sure it's useful for
libcacard user.


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 23/26] libcacard: change default ATR

[Marc-André Lureau]

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 22/26] libcacard/vreader: add debugging messages for apdu

[Marc-André Lureau]

Since libcacard uses glib, why not just use g_debug() with a LOG_DOMAIN?

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 21/26] dev-smartcard-reader: reuse usb.h definitions

[Marc-André Lureau]

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 20/26] hw/usb/dev-smartcard-reader: support windows guest

[Marc-André Lureau]

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 19/26] hw/usb/dev-smartcard-reader.c: dwFeadvertise support for T=0 only

[Marc-André Lureau]

Here you could merge with the one where you changed default T=0.

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 18/26] hw/usb/dev-smartcard-reader.c: copy atr's protocol to ccid's parameters (adds todo's)

[Marc-André Lureau]

Hi,


On Mon, Mar 18, 2013 at 2:11 PM, Alon Levy <alevy@redhat.com> wrote:
> +    if (atr_protocol_num == 0) {
> +        DPRINTF(s, D_WARN, "%s: error: unimplemented ATR T0 parameters"
> +                " setting\n", __func__);
> +        t0->bmFindexDindex = 0;
> +        t0->bmTCCKST0 = 0;
> +        t0->bGuardTimeT0 = 0;
> +        t0->bWaitingIntegerT0 = 0;
> +        t0->bClockStop = 0;
> +    } else {
> +        if (atr_protocol_num != 1) {
> +            DPRINTF(s, D_WARN, "%s: error: unsupported ATR protocol %d\n",
> +                    __func__, atr_protocol_num);
> +        } else {
> +            DPRINTF(s, D_WARN, "%s: error: unimplemented ATR T1 parameters"
> +                    " setting\n", __func__);
> +            t1->bmFindexDindex = 0;
> +            t1->bmTCCKST1 = 0;
> +            t1->bGuardTimeT1 = 0;
> +            t1->bWaitingIntegerT1 = 0;
> +            t1->bClockStop = 0;
> +            t1->bIFSC = 0;
> +            t1->bNadValue = 0;
> +        }
> +    }

Those blocks could be at the same indentation level, or perhaps use a switch?

Is it sensible to warn in all cases?

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 17/26] hw/usb/dev-smartcard-reader.c: define structs for CCID_Parameter internals

[Marc-André Lureau]

Hi

On Mon, Mar 18, 2013 at 2:11 PM, Alon Levy <alevy@redhat.com> wrote:
>  static void ccid_reset_parameters(USBCCIDState *s)
>  {
> -   uint32_t len = sizeof(abDefaultProtocolDataStructure);
> -
> -   s->bProtocolNum = 1; /* T=1 */
> -   s->ulProtocolDataStructureSize = len;
> -   memcpy(s->abProtocolDataStructure, abDefaultProtocolDataStructure, len);
> +   s->bProtocolNum = 0; /* T=0 */
> +   s->abProtocolDataStructure = defaultProtocolDataStructure;
>  }

You changed default protocol, even if it's a correct change imho, why
not make it explicit with a seperate patch?

Other than that, it looks great.


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 16/26] hw/usb/dev-smartcard-reader.c: remove aborts (never triggered, but just in case)

[Marc-André Lureau]

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 15/26] hw/usb/dev-smartcard-reader.c: nicer debug messages

[Marc-André Lureau]

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 14/26] hw/usb/dev-smartcard-reader.c: white space fixes

[Marc-André Lureau]

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 13/26] ccid-card-passthru, dev-smartcard-reader: add debug environment variables

[Marc-André Lureau]

Hi

On Mon, Mar 18, 2013 at 2:10 PM, Alon Levy <alevy@redhat.com> wrote:
> From: Alon Levy <alon@pobox.com>
>
> This overrides whatever debug value is set on the corresponding devices
> from the command line, and is meant to ease the usage with any
> management stack. For libvirt you can set environment variables by
> extending the dom namespace, i.e:
>
> <domain type='kvm' id='3' xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'>
>   <qemu:commandline>
>     <qemu:env name='QEMU_CCID_PASSTHRU_DEBUG' value='4'/>
>     <qemu:env name='QEMU_CCID_DEBUG' value='4'/>
>   </qemu:commandline>
> </domain>
>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  hw/ccid-card-passthru.c       | 16 ++++++++++++++++
>  hw/usb/dev-smartcard-reader.c | 16 ++++++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/hw/ccid-card-passthru.c b/hw/ccid-card-passthru.c
> index 111894f..80fe514 100644
> --- a/hw/ccid-card-passthru.c
> +++ b/hw/ccid-card-passthru.c
> @@ -350,6 +350,22 @@ static int passthru_initfn(CCIDCardState *base)
>          error_report("missing chardev");
>          return -1;
>      }
> +    {
> +        char *debug_env = getenv("QEMU_CCID_PASSTHRU_DEBUG");
> +        char *inv = NULL;
> +        int debug;
> +        if (debug_env) {
> +            debug = strtol(debug_env, &inv, 10);
> +            if (inv != debug_env) {
> +                if (debug < 0 || debug > D_VERBOSE) {
> +                    fprintf(stderr, "warning: QEMU_CCID_PASSTHRU_DEBUG"
> +                            " not in [0, %d]", D_VERBOSE);
> +                } else {
> +                    card->debug = debug;
> +                }
> +            }
> +        }
> +    }
>      assert(sizeof(DEFAULT_ATR) <= MAX_ATR_SIZE);
>      memcpy(card->atr, DEFAULT_ATR, sizeof(DEFAULT_ATR));
>      card->atr_length = sizeof(DEFAULT_ATR);
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index caebc1c..fc950c8 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -1201,6 +1201,22 @@ static int ccid_initfn(USBDevice *dev)
>      s->bulk_out_pos = 0;
>      ccid_reset_parameters(s);
>      ccid_reset(s);
> +    {
> +        char *debug_env = getenv("QEMU_CCID_DEBUG");
> +        char *inv = NULL;
> +        int debug;
> +        if (debug_env) {
> +            debug = strtol(debug_env, &inv, 10);
> +            if (inv != debug_env) {
> +                if (debug < 0 || debug > D_VERBOSE) {
> +                    fprintf(stderr, "warning: QEMU_CCID_PASSTHRU_DEBUG"
> +                            " not in [0, %d]", D_VERBOSE);
> +                } else {
> +                    s->debug = debug;
> +                }
> +            }
> +        }
> +    }
>      return 0;
>  }
>
> --
> 1.8.1.4
>
>

If not generalized (in util/cutils?) it could probably share the same
code in a function parse_debug_env("QEMU_CCID_PASSTHRU_DEBUG",
&s->debug)

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 12/26] hw/ccid-card-passthru.c: add atr check

[Marc-André Lureau]

On Mon, Mar 18, 2013 at 2:10 PM, Alon Levy <alevy@redhat.com> wrote:
> +    if (len > 2 + historical_length + opt_bytes) {
> +        DPRINTF(card, D_WARN,
> +            "atr too long: len %d, but hist/opt %d/%d, T1 0x%X\n",
> +            len, historical_length, opt_bytes, data[1]);
> +        /* let it through */

Why "let it through" if it's too long?


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 07/26] build-sys: must link with -fstack-protector

[Paolo Bonzini]

Il 18/03/2013 14:10, Alon Levy ha scritto:
> From: Marc-André Lureau <marcandre.lureau@gmail.com>
> 
> It is needed to give that flag to the linker as well, but latest
> libtool 2.4.2 still swallows that argument, so let's pass it with
> libtool -Wc argument.
> 
> qemu-1.4.0/stubs/arch-query-cpu-def.c:6: undefined reference to `__stack_chk_guard'
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  configure | 8 +++++++-
>  rules.mak | 1 +
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 46a7594..d76866a 100755
> --- a/configure
> +++ b/configure
> @@ -1209,7 +1209,7 @@ fi
>  gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
>  gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
>  gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
> -gcc_flags="-fstack-protector-all -Wendif-labels $gcc_flags"
> +gcc_flags="-Wendif-labels $gcc_flags"
>  gcc_flags="-Wno-initializer-overrides $gcc_flags"
>  # Note that we do not add -Werror to gcc_flags here, because that would
>  # enable it for all configure tests. If a configure test failed due
> @@ -1228,6 +1228,11 @@ for flag in $gcc_flags; do
>      fi
>  done
>  
> +if compile_prog "-Werror -fstack-protector-all" "" ; then
> +    QEMU_CFLAGS="$QEMU_CFLAGS -fstack-protector-all"
> +    LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,-fstack-protector-all"
> +fi
> +
>  # Workaround for http://gcc.gnu.org/PR55489.  Happens with -fPIE/-fPIC and
>  # large functions that use global variables.  The bug is in all releases of
>  # GCC, but it became particularly acute in 4.6.x and 4.7.x.  It is fixed in
> @@ -3890,6 +3895,7 @@ else
>    echo "AUTOCONF_HOST := "                             >> $config_host_mak
>  fi
>  echo "LDFLAGS=$LDFLAGS" >> $config_host_mak
> +echo "LIBTOOLFLAGS=$LIBTOOLFLAGS" >> $config_host_mak
>  echo "ARLIBS_BEGIN=$arlibs_begin" >> $config_host_mak
>  echo "ARLIBS_END=$arlibs_end" >> $config_host_mak
>  echo "LIBS+=$LIBS" >> $config_host_mak
> diff --git a/rules.mak b/rules.mak
> index edc2552..36aba2d 100644
> --- a/rules.mak
> +++ b/rules.mak
> @@ -36,6 +36,7 @@ LINK = $(call quiet-command,\
>         $(if $(filter %.lo %.la,$^),$(LIBTOOL) --mode=link --tag=CC \
>         )$(CC) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ \
>         $(sort $(filter %.o, $1)) $(filter-out %.o, $1) $(version-obj-y) \
> +       $(if $(filter %.lo %.la,$^),$(LIBTOOLFLAGS)) \
>         $(LIBS),$(if $(filter %.lo %.la,$^),"lt LINK ", "  LINK  ")"$(TARGET_DIR)$@")
>  endif
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 12/26] hw/ccid-card-passthru.c: add atr check

[Alon Levy]

> On Mon, Mar 18, 2013 at 2:10 PM, Alon Levy <alevy@redhat.com> wrote:
> > +    if (len > 2 + historical_length + opt_bytes) {
> > +        DPRINTF(card, D_WARN,
> > +            "atr too long: len %d, but hist/opt %d/%d, T1 0x%X\n",
> > +            len, historical_length, opt_bytes, data[1]);
> > +        /* let it through */
> 
> Why "let it through" if it's too long?

I know windows doesn't like too short, but no idea about too long. Haven't seen this in practice yet (well, we control the other end too, but I saw no reason to fail it at this point).

> 
> 
> --
> Marc-André Lureau
> 
> 

Re: [Qemu-devel] [PATCH 24/26] libcacard: move atr setting from macro to function

[Alon Levy]

> Hi,
> 
> review below
> 
> On Mon, Mar 18, 2013 at 2:11 PM, Alon Levy <alevy@redhat.com> wrote:
> > Only because qemu's checkpatch complains about it.
> >
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> >  Makefile.objs              |  1 +
> >  libcacard/vcard_emul_nss.c | 11 ++++++++---
> >  libcacard/vcardt.c         | 40
> >  ++++++++++++++++++++++++++++++++++++++++
> >  libcacard/vcardt.h         | 15 ++-------------
> >  4 files changed, 51 insertions(+), 16 deletions(-)
> >  create mode 100644 libcacard/vcardt.c
> >
> > diff --git a/Makefile.objs b/Makefile.objs
> > index f99841c..6d47567 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -41,6 +41,7 @@ libcacard-y += libcacard/vcard.o
> > libcacard/vreader.o
> >  libcacard-y += libcacard/vcard_emul_nss.o
> >  libcacard-y += libcacard/vcard_emul_type.o
> >  libcacard-y += libcacard/card_7816.o
> > +libcacard-y += libcacard/vcardt.o
> >
> >  ######################################################################
> >  # Target independent part of system emulation. The long term path
> >  is to
> > diff --git a/libcacard/vcard_emul_nss.c
> > b/libcacard/vcard_emul_nss.c
> > index 21d4689..6bad0b9 100644
> > --- a/libcacard/vcard_emul_nss.c
> > +++ b/libcacard/vcard_emul_nss.c
> > @@ -519,18 +519,23 @@ vcard_emul_reader_get_slot(VReader *vreader)
> >  }
> >
> >  /*
> > - *  Card ATR's map to physical cards. VCARD_ATR_PREFIX will set
> > appropriate
> > + *  Card ATR's map to physical cards. vcard_alloc_atr will set
> > appropriate
> >   *  historical bytes for any software emulated card. The remaining
> >   bytes can be
> >   *  used to indicate the actual emulator
> >   */
> > -static const unsigned char nss_atr[] = { VCARD_ATR_PREFIX(3), 'N',
> > 'S', 'S' };
> > +static unsigned char *nss_atr;
> > +static int nss_atr_len;
> >
> >  void
> >  vcard_emul_get_atr(VCard *card, unsigned char *atr, int *atr_len)
> >  {
> > -    int len = MIN(sizeof(nss_atr), *atr_len);
> > +    int len;
> >      assert(atr != NULL);
> >
> > +    if (nss_atr == NULL) {
> > +        nss_atr = vcard_alloc_atr("NSS", &nss_atr_len);
> > +    }
> > +    len = MIN(nss_atr_len, *atr_len);
> >      memcpy(atr, nss_atr, len);
> >      *atr_len = len;
> >  }
> > diff --git a/libcacard/vcardt.c b/libcacard/vcardt.c
> > new file mode 100644
> > index 0000000..f64c343
> > --- /dev/null
> > +++ b/libcacard/vcardt.c
> > @@ -0,0 +1,40 @@
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <glib.h>
> > +
> > +#include "libcacard/vcardt.h"
> > +
> > +/* create an ATR with appropriate historical bytes */
> > +#define ATR_TS_DIRECT_CONVENTION 0x3b
> > +#define ATR_TA_PRESENT 0x10
> > +#define ATR_TB_PRESENT 0x20
> > +#define ATR_TC_PRESENT 0x40
> > +#define ATR_TD_PRESENT 0x80
> > +
> > +unsigned char *vcard_alloc_atr(const char *postfix, int *atr_len)
> > +{
> > +    int postfix_len;
> > +    const char prefix[] = "VCARD_";
> > +    const int prefix_len = strlen(prefix);
> 
> or sizeof(prefix) -1
> 
> > +    int total_len;
> > +    unsigned char *atr;
> > +
> > +    if (postfix == NULL) {
> 
> I would make postfix mandatory.
> 
> > +        postfix_len = 0;
> > +    } else {
> > +        postfix_len = strlen(postfix);
> > +    }
> > +    total_len = 3 + prefix_len + postfix_len;
> > +    atr = g_malloc(total_len);
> > +    atr[0] = ATR_TS_DIRECT_CONVENTION;
> > +    atr[1] = ATR_TD_PRESENT + prefix_len + postfix_len;
> > +    atr[2] = 0x00;
> > +    memcpy(&atr[3], prefix, prefix_len);
> > +    if (postfix) {
> > +        memcpy(&atr[3 + prefix_len], postfix, postfix_len);
> > +    }
> > +    if (atr_len) {
> > +        *atr_len = total_len;
> > +    }
> > +    return atr;
> > +}
> > diff --git a/libcacard/vcardt.h b/libcacard/vcardt.h
> > index 3b9a619..e8e651f 100644
> > --- a/libcacard/vcardt.h
> > +++ b/libcacard/vcardt.h
> > @@ -25,19 +25,6 @@ typedef struct VCardEmulStruct VCardEmul;
> >
> >  #define MAX_CHANNEL 4
> >
> > -/* create an ATR with appropriate historical bytes */
> > -#define TS_DIRECT_CONVENTION 0x3b
> > -#define TA_PRESENT 0x10
> > -#define TB_PRESENT 0x20
> > -#define TC_PRESENT 0x40
> > -#define TD_PRESENT 0x80
> > -
> > -#define VCARD_ATR_PREFIX(size) \
> > -    TS_DIRECT_CONVENTION, \
> > -    TD_PRESENT + (6 + size), \
> > -    0x00, \
> > -    'V', 'C', 'A', 'R', 'D', '_'
> > -
> >  typedef enum {
> >      VCARD_DONE,
> >      VCARD_NEXT,
> > @@ -69,4 +56,6 @@ struct VCardBufferResponseStruct {
> >      int len;
> >  };
> >
> > +unsigned char *vcard_alloc_atr(const char *postfix, int *atr_len);
> > +
> 
> If the function is exposed in public header, it should also be
> exported (in libcacard.syms), but I am not sure it's useful for
> libcacard user.

It is purely internal, I could add a private header for this.

> 
> 
> --
> Marc-André Lureau
> 
> 

Re: [Qemu-devel] [PATCH 18/26] hw/usb/dev-smartcard-reader.c: copy atr's protocol to ccid's parameters (adds todo's)

[Alon Levy]

On Fri, Mar 22, 2013 at 03:23:15PM +0100, Marc-André Lureau wrote:
> Hi,
> 
> 
> On Mon, Mar 18, 2013 at 2:11 PM, Alon Levy <alevy@redhat.com> wrote:
> > +    if (atr_protocol_num == 0) {
> > +        DPRINTF(s, D_WARN, "%s: error: unimplemented ATR T0 parameters"
> > +                " setting\n", __func__);
> > +        t0->bmFindexDindex = 0;
> > +        t0->bmTCCKST0 = 0;
> > +        t0->bGuardTimeT0 = 0;
> > +        t0->bWaitingIntegerT0 = 0;
> > +        t0->bClockStop = 0;
> > +    } else {
> > +        if (atr_protocol_num != 1) {
> > +            DPRINTF(s, D_WARN, "%s: error: unsupported ATR protocol %d\n",
> > +                    __func__, atr_protocol_num);
> > +        } else {
> > +            DPRINTF(s, D_WARN, "%s: error: unimplemented ATR T1 parameters"
> > +                    " setting\n", __func__);
> > +            t1->bmFindexDindex = 0;
> > +            t1->bmTCCKST1 = 0;
> > +            t1->bGuardTimeT1 = 0;
> > +            t1->bWaitingIntegerT1 = 0;
> > +            t1->bClockStop = 0;
> > +            t1->bIFSC = 0;
> > +            t1->bNadValue = 0;
> > +        }
> > +    }
> 
> Those blocks could be at the same indentation level, or perhaps use a switch?

A switch is a good idea.

> 
> Is it sensible to warn in all cases?

Turning them into TODO comments instead (except for the now default
case).

> 
> -- 
> Marc-André Lureau
>