Patchwork [10/20] ccid-card-passthru: review fixes (v15->v16)

login
register
mail settings
Submitter Alon Levy
Date Feb. 2, 2011, 8:28 p.m.
Message ID <1296678500-19497-11-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/81544/
State New
Headers show

Comments

Alon Levy - Feb. 2, 2011, 8:28 p.m.
For review I hope presenting this as a separate patch (and the
same for the rest of the files) will be easier. For the final round
I'll send it folded into the introducing commit.

There are a few bug fixes here that are not directly from the review.

Behavioral changes:
 * return correct size
 * return error instead of assert if client sent too large ATR
 * don't assert if client sent too large a size, but add asserts for indices to buffer
 * reset vscard_in indices on chardev disconnect
 * handle init from client
 * error if no chardev supplied
 * use ntoh, hton
 * eradicate reader_id_t
 * remove Reconnect usage (removed from VSCARD protocol)
 * send VSC_SUCCESS on card insert/remove and reader add/remove

Style fixes:
 * width of line fix
 * update copyright
 * remove old TODO's
 * update file header comment
 * use macros for debug levels
 * c++ style comment replacement
 * update copyright license
 * fix ATR size comment
 * fix whitespace in struct def
 * fix DPRINTF prefix
 * line width fix

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

Patch

diff --git a/hw/ccid-card-passthru.c b/hw/ccid-card-passthru.c
index 6ec4f21..71d3a53 100644
--- a/hw/ccid-card-passthru.c
+++ b/hw/ccid-card-passthru.c
@@ -1,24 +1,32 @@ 
 /*
- * CCID Card Device emulation
+ * CCID Passthru Card Device emulation
  *
- * Copyright (c) 2010 Red Hat.
+ * Copyright (c) 2011 Red Hat.
  * Written by Alon Levy.
  *
- * This code is licenced under the LGPL.
+ * This code is licenced under the GNU LGPL, version 2 or later.
  */
 
+#include <arpa/inet.h>
+
 #include "qemu-char.h"
 #include "monitor.h"
 #include "hw/ccid.h"
 #include "libcacard/vscard_common.h"
 
-#define DPRINTF(card, lvl, fmt, ...) \
-do { if (lvl <= card->debug) { printf("ccid-card: " fmt , ## __VA_ARGS__); } } while (0)
-
-/* Passthru card */
+#define DPRINTF(card, lvl, fmt, ...)                    \
+do {                                                    \
+    if (lvl <= card->debug) {                           \
+        printf("ccid-card-passthru: " fmt , ## __VA_ARGS__);     \
+    }                                                   \
+} while (0)
 
+#define D_WARN 1
+#define D_INFO 2
+#define D_MORE_INFO 3
+#define D_VERBOSE 4
 
-// TODO: do we still need this?
+/* TODO: do we still need this? */
 uint8_t DEFAULT_ATR[] = {
 /* From some example somewhere
  0x3B, 0xB0, 0x18, 0x00, 0xD1, 0x81, 0x05, 0xB1, 0x40, 0x38, 0x1F, 0x03, 0x28
@@ -27,11 +35,13 @@  uint8_t DEFAULT_ATR[] = {
 /* From an Athena smart card */
  0x3B, 0xD5, 0x18, 0xFF, 0x80, 0x91, 0xFE, 0x1F, 0xC3, 0x80, 0x73, 0xC8, 0x21, 0x13, 0x08
 
-}; /* maximum size of ATR - from 7816-3 */
+};
 
 
 #define PASSTHRU_DEV_NAME "ccid-card-passthru"
 #define VSCARD_IN_SIZE 65536
+
+/* maximum size of ATR - from 7816-3 */
 #define MAX_ATR_SIZE        40
 
 typedef struct PassthruState PassthruState;
@@ -44,7 +54,7 @@  struct PassthruState {
     uint32_t vscard_in_hdr;
     uint8_t  atr[MAX_ATR_SIZE];
     uint8_t  atr_length;
-    uint8_t debug;
+    uint8_t  debug;
 };
 
 /* VSCard protocol over chardev
@@ -52,14 +62,14 @@  struct PassthruState {
  * */
 
 static void ccid_card_vscard_send_msg(
-    PassthruState *s, VSCMsgType type, reader_id_t reader_id,
+    PassthruState *s, VSCMsgType type, uint32_t reader_id,
         const uint8_t* payload, uint32_t length)
 {
     VSCMsgHeader scr_msg_header;
 
-    scr_msg_header.type = type;
-    scr_msg_header.reader_id = reader_id;
-    scr_msg_header.length = length;
+    scr_msg_header.type = htonl(type);
+    scr_msg_header.reader_id = htonl(reader_id);
+    scr_msg_header.length = htonl(length);
     qemu_chr_write(s->cs, (uint8_t*)&scr_msg_header, sizeof(VSCMsgHeader));
     qemu_chr_write(s->cs, payload, length);
 }
@@ -71,16 +81,20 @@  static void ccid_card_vscard_send_apdu(
 }
 
 static void ccid_card_vscard_send_error(
-    PassthruState *s, reader_id_t reader_id, VSCErrorCode code)
+    PassthruState *s, uint32_t reader_id, VSCErrorCode code)
 {
-    VSCMsgError msg = {.code=code};
+    VSCMsgError msg = {.code=htonl(code)};
 
     ccid_card_vscard_send_msg(s, VSC_Error, reader_id, (uint8_t*)&msg, sizeof(msg));
 }
 
 static void ccid_card_vscard_send_init(PassthruState *s)
 {
-    VSCMsgInit msg = {.version=VSCARD_VERSION};
+    VSCMsgInit msg = {
+        .version=htonl(VSCARD_VERSION),
+        .magic=VSCARD_MAGIC,
+        .capabilities={0}
+    };
 
     ccid_card_vscard_send_msg(s, VSC_Init, VSCARD_UNDEFINED_READER_ID,
                          (uint8_t*)&msg, sizeof(msg));
@@ -88,7 +102,33 @@  static void ccid_card_vscard_send_init(PassthruState *s)
 
 static int ccid_card_vscard_can_read(void *opaque)
 {
-    return 65535;
+    PassthruState *card = opaque;
+
+    return VSCARD_IN_SIZE >= card->vscard_in_pos ?
+           VSCARD_IN_SIZE - card->vscard_in_pos : 0;
+}
+
+static void ccid_card_vscard_handle_init(PassthruState *card, VSCMsgHeader *hdr, VSCMsgInit *init)
+{
+    uint32_t *capabilities;
+    int num_capabilities;
+    int i;
+
+    capabilities = init->capabilities;
+    num_capabilities = 1 + ((hdr->length - sizeof(VSCMsgInit)) / sizeof(uint32_t));
+    init->version = ntohl(init->version);
+    for (i = 0 ; i < num_capabilities; ++i) {
+        capabilities[i] = ntohl(capabilities[i]);
+    }
+    if (init->magic != VSCARD_MAGIC) {
+        error_report("wrong magic");
+        /* we can't disconnect the chardev */
+    }
+    if (init->version != VSCARD_VERSION) {
+        DPRINTF(card, D_WARN, "got version %d, have %d", init->version, VSCARD_VERSION);
+    }
+    /* future handling of capabilities, none exist atm */
+    ccid_card_vscard_send_init(card);
 }
 
 static void ccid_card_vscard_handle_message(PassthruState *card,
@@ -98,20 +138,30 @@  static void ccid_card_vscard_handle_message(PassthruState *card,
 
     switch (scr_msg_header->type) {
         case VSC_ATR:
-            DPRINTF(card, 1, "VSC_ATR %d\n", scr_msg_header->length);
-            assert(scr_msg_header->length <= MAX_ATR_SIZE);
+            DPRINTF(card, D_INFO, "VSC_ATR %d\n", scr_msg_header->length);
+            if (scr_msg_header->length > MAX_ATR_SIZE) {
+                error_report("ATR size exceeds spec, ignoring");
+                ccid_card_vscard_send_error(card, scr_msg_header->reader_id,
+                                            VSC_GENERAL_ERROR);
+            }
             memcpy(card->atr, data, scr_msg_header->length);
             card->atr_length = scr_msg_header->length;
             ccid_card_card_inserted(&card->base);
+            ccid_card_vscard_send_error(card, scr_msg_header->reader_id,
+                                        VSC_SUCCESS);
             break;
         case VSC_APDU:
             ccid_card_send_apdu_to_guest(&card->base, data, scr_msg_header->length);
             break;
         case VSC_CardRemove:
-            DPRINTF(card, 1, "VSC_CardRemove\n");
+            DPRINTF(card, D_INFO, "VSC_CardRemove\n");
             ccid_card_card_removed(&card->base);
+            ccid_card_vscard_send_error(card,
+                scr_msg_header->reader_id, VSC_SUCCESS);
             break;
         case VSC_Init:
+            ccid_card_vscard_handle_init(
+                card, scr_msg_header, (VSCMsgInit *)data);
             break;
         case VSC_Error:
             ccid_card_card_error(&card->base, *(uint64_t*)data);
@@ -121,12 +171,14 @@  static void ccid_card_vscard_handle_message(PassthruState *card,
                 ccid_card_vscard_send_error(card, VSCARD_UNDEFINED_READER_ID,
                                           VSC_CANNOT_ADD_MORE_READERS);
             } else {
-                ccid_card_vscard_send_msg(card, VSC_ReaderAddResponse,
-                                             VSCARD_MINIMAL_READER_ID, NULL, 0);
+                ccid_card_vscard_send_error(card, VSCARD_MINIMAL_READER_ID,
+                                            VSC_SUCCESS);
             }
             break;
         case VSC_ReaderRemove:
             ccid_card_ccid_detach(&card->base);
+            ccid_card_vscard_send_error(card,
+                scr_msg_header->reader_id, VSC_SUCCESS);
             break;
         default:
             printf("usb-ccid: chardev: unexpected message of type %X\n",
@@ -136,19 +188,35 @@  static void ccid_card_vscard_handle_message(PassthruState *card,
     }
 }
 
+static void ccid_card_vscard_drop_connection(PassthruState *card)
+{
+    qemu_chr_close(card->cs);
+    card->vscard_in_pos = card->vscard_in_hdr = 0;
+}
+
 static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size)
 {
     PassthruState *card = opaque;
     VSCMsgHeader *hdr;
 
-    assert(card->vscard_in_pos + size <= VSCARD_IN_SIZE);
+    if (card->vscard_in_pos + size > VSCARD_IN_SIZE) {
+        error_report("no room for data: pos %d +  size %d > %d. dropping connection.",
+            card->vscard_in_pos, size, VSCARD_IN_SIZE);
+        ccid_card_vscard_drop_connection(card);
+        return;
+    }
+    assert(card->vscard_in_pos < VSCARD_IN_SIZE);
+    assert(card->vscard_in_hdr < VSCARD_IN_SIZE);
     memcpy(card->vscard_in_data + card->vscard_in_pos, buf, size);
     card->vscard_in_pos += size;
     hdr = (VSCMsgHeader*)(card->vscard_in_data + card->vscard_in_hdr);
 
-    while ((card->vscard_in_pos - card->vscard_in_hdr >= sizeof(VSCMsgHeader)) &&
-           (card->vscard_in_pos - card->vscard_in_hdr - sizeof(VSCMsgHeader) >=
-           hdr->length)) {
+    while ((card->vscard_in_pos - card->vscard_in_hdr >= sizeof(VSCMsgHeader))
+        && (card->vscard_in_pos - card->vscard_in_hdr >=
+                                  sizeof(VSCMsgHeader) + ntohl(hdr->length))) {
+        hdr->reader_id = ntohl(hdr->reader_id);
+        hdr->length = ntohl(hdr->length);
+        hdr->type = ntohl(hdr->type);
         ccid_card_vscard_handle_message(card, hdr);
         card->vscard_in_hdr += hdr->length + sizeof(VSCMsgHeader);
         hdr = (VSCMsgHeader*)(card->vscard_in_data + card->vscard_in_hdr);
@@ -164,11 +232,12 @@  static void ccid_card_vscard_event(void *opaque, int event)
 
     switch (event) {
         case CHR_EVENT_BREAK:
+            card->vscard_in_pos = card->vscard_in_hdr = 0;
             break;
         case CHR_EVENT_FOCUS:
             break;
         case CHR_EVENT_OPENED:
-            DPRINTF(card, 1, "%s: CHR_EVENT_OPENED\n", __func__);
+            DPRINTF(card, D_INFO, "%s: CHR_EVENT_OPENED\n", __func__);
             break;
     }
 }
@@ -201,12 +270,14 @@  static int passthru_initfn(CCIDCardState *base)
     card->vscard_in_pos = 0;
     card->vscard_in_hdr = 0;
     if (card->cs) {
-        DPRINTF(card, 1, "initing chardev\n");
+        DPRINTF(card, D_INFO, "initing chardev\n");
         qemu_chr_add_handlers(card->cs,
             ccid_card_vscard_can_read,
             ccid_card_vscard_read,
             ccid_card_vscard_event, card);
-        ccid_card_vscard_send_init(card);
+    } else {
+        error_report("missing chardev");
+        return -1;
     }
     assert(sizeof(DEFAULT_ATR) <= MAX_ATR_SIZE);
     memcpy(card->atr, DEFAULT_ATR, sizeof(DEFAULT_ATR));
@@ -219,22 +290,10 @@  static int passthru_exitfn(CCIDCardState *base)
     return 0;
 }
 
-static void passthru_pre_save(void *opaque)
-{
-    PassthruState *card = opaque;
-    VSCMsgReconnect reconnect;
-
-    reconnect.ip = 0; // TODO - does the bus keep the target ip? s->migration_target_ip;
-    reconnect.port = 0; // TODO - does the bus keep the target ip? s->migration_target_port;
-    ccid_card_vscard_send_msg(card, VSC_Reconnect, VSCARD_UNDEFINED_READER_ID,
-                         (uint8_t*)&reconnect, sizeof(reconnect));
-}
-
 static VMStateDescription passthru_vmstate = {
     .name = PASSTHRU_DEV_NAME,
     .version_id = 1,
     .minimum_version_id = 1,
-    .pre_save = passthru_pre_save,
     .fields = (VMStateField []) {
         VMSTATE_BUFFER(vscard_in_data, PassthruState),
         VMSTATE_UINT32(vscard_in_pos, PassthruState),
@@ -264,10 +323,6 @@  static CCIDCardInfo passthru_card_info = {
 static void ccid_card_passthru_register_devices(void)
 {
     ccid_card_qdev_register(&passthru_card_info);
-    // TODO: passthru local card (or: just a case of passthru with no chardev
-    //  given and instead some other arguments that would be required for local
-    //  card anyway and can be shared with the emulated local card)
-    // TODO: emulated local card
 }
 
 device_init(ccid_card_passthru_register_devices)