[Qemu-devel] [PATCH v2 0/8] usb-ccid: Misc fixes and T=1 support

[Qemu-devel] [PATCH v2 0/8] usb-ccid: Misc fixes and T=1 support

[Stefan Fritsch]

From: Stefan Fritsch <stefan_fritsch@genua.de>

Sorry, I have accidentally sent an old patchset that was missing one
fix.

This set of changes contains fixes and extensions to allow to use ccid
card passthru with a physical cardos smartcard using the T=1 protocol.

Unfortunately, our vscard client program ist not public, therefore it
is difficult for other people to test the functionality. But maybe you
are still interested to include some bits of it in qemu?


v2: Fix 'usb-ccid: Fix chaining fields in CCID USB messages' not
    compiling.


Stefan Fritsch (8):
  usb-ccid: Add support to dump all USB packets
  usb-ccid: Fix USB packet generation for 64-Bytes sized packets.
  usb-ccid: Set protocol parameters based on card ATR
  usb-ccid: Fix ATR parsing
  usb-ccid: Fix USB descriptor
  usb-ccid: Fix chaining fields in CCID USB messages
  usb-ccid: Increase ccid max APDU size
  usb-ccid: Reduce logging at level WARN

 hw/usb/ccid-card-passthru.c   |  34 +++++++---
 hw/usb/dev-smartcard-reader.c | 148 ++++++++++++++++++++++++++++++++----------
 2 files changed, 138 insertions(+), 44 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v2 1/8] usb-ccid: Add support to dump all USB packets

[Stefan Fritsch]

From: Stefan Fritsch <stefan_fritsch@genua.de>

The dump can be activated by the debug command line option for the
device like this "qemu ... -dev usb,ccid,debug=5"

While there move the short read debug message to a higher debug level.
It triggers very often and makes debug output unreadable.

Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
Signed-off-by: Christian Ehrhardt <christian_ehrhardt@genua.de>
---
 hw/usb/dev-smartcard-reader.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index bef1f03c42..624dc2c447 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -54,9 +54,25 @@ do { \
 #define D_INFO 2
 #define D_MORE_INFO 3
 #define D_VERBOSE 4
+#define D_TRACE 5
+#define D_REMOTEIO 10
 
 #define CCID_DEV_NAME "usb-ccid"
 #define USB_CCID_DEV(obj) OBJECT_CHECK(USBCCIDState, (obj), CCID_DEV_NAME)
+
+static void usb_packet_dump(int lvl, const char *dir, uint8_t *buf, size_t len)
+{
+    int i;
+    if (lvl < D_TRACE) {
+        return;
+    }
+    printf("usb-ccid: usb packet(%s/%zd):", dir, len);
+    for (i = 0; i < len; ++i) {
+        printf(" %02x", buf[i]);
+    }
+    printf("\n");
+}
+
 /*
  * The two options for variable sized buffers:
  * make them constant size, for large enough constant,
@@ -1007,6 +1023,8 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p)
         goto err;
     }
     usb_packet_copy(p, s->bulk_out_data + s->bulk_out_pos, p->iov.size);
+    usb_packet_dump(s->debug, "out", s->bulk_out_data + s->bulk_out_pos,
+                    p->iov.size);
     s->bulk_out_pos += p->iov.size;
     if (s->bulk_out_pos < 10) {
         DPRINTF(s, 1, "%s: header incomplete\n", __func__);
@@ -1102,6 +1120,9 @@ static void ccid_bulk_in_copy_to_guest(USBCCIDState *s, USBPacket *p)
                   p->iov.size);
         usb_packet_copy(p, s->current_bulk_in->data +
                         s->current_bulk_in->pos, len);
+        usb_packet_dump(s->debug, " in",
+                        s->current_bulk_in->data + s->current_bulk_in->pos,
+                        len);
         s->current_bulk_in->pos += len;
         if (s->current_bulk_in->pos == s->current_bulk_in->len) {
             ccid_bulk_in_release(s);
@@ -1116,7 +1137,7 @@ static void ccid_bulk_in_copy_to_guest(USBCCIDState *s, USBPacket *p)
                 __func__, p->iov.size, len);
     }
     if (len < p->iov.size) {
-        DPRINTF(s, 1,
+        DPRINTF(s, D_REMOTEIO,
                 "%s: returning short (EREMOTEIO) %d < %zd\n",
                 __func__, len, p->iov.size);
     }
@@ -1143,6 +1164,7 @@ static void ccid_handle_data(USBDevice *dev, USBPacket *p)
                 buf[0] = CCID_MESSAGE_TYPE_RDR_to_PC_NotifySlotChange;
                 buf[1] = s->bmSlotICCState;
                 usb_packet_copy(p, buf, 2);
+                usb_packet_dump(s->debug, "irq", buf, 2);
                 s->notify_slot_change = false;
                 s->bmSlotICCState &= ~SLOT_0_CHANGED_MASK;
                 DPRINTF(s, D_INFO,
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 1/8] usb-ccid: Add support to dump all USB packets

[Gerd Hoffmann]

> index bef1f03c42..624dc2c447 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -54,9 +54,25 @@ do { \
>  #define D_INFO 2
>  #define D_MORE_INFO 3
>  #define D_VERBOSE 4
> +#define D_TRACE 5
> +#define D_REMOTEIO 10

Considered converting all DPRINTFs into tracepoints instead?

Then you can turn then on/off individually as needed.

>  #define CCID_DEV_NAME "usb-ccid"
>  #define USB_CCID_DEV(obj) OBJECT_CHECK(USBCCIDState, (obj),
> CCID_DEV_NAME)
> +
> +static void usb_packet_dump(int lvl, const char *dir, uint8_t *buf,
> size_t len)
> +{
> +    int i;
> +    if (lvl < D_TRACE) {
> +        return;
> +    }
> +    printf("usb-ccid: usb packet(%s/%zd):", dir, len);
> +    for (i = 0; i < len; ++i) {
> +        printf(" %02x", buf[i]);
> +    }
> +    printf("\n");
> +}

There is qemu_hexdump() ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 1/8] usb-ccid: Add support to dump all USB packets

[Stefan Fritsch]

On Fri, 21 Jul 2017, Gerd Hoffmann wrote:

> > index bef1f03c42..624dc2c447 100644
> > --- a/hw/usb/dev-smartcard-reader.c
> > +++ b/hw/usb/dev-smartcard-reader.c
> > @@ -54,9 +54,25 @@ do { \
> >  #define D_INFO 2
> >  #define D_MORE_INFO 3
> >  #define D_VERBOSE 4
> > +#define D_TRACE 5
> > +#define D_REMOTEIO 10
> 
> Considered converting all DPRINTFs into tracepoints instead?
> 
> Then you can turn then on/off individually as needed.

I want to have D_WARN enabled by default, so those cannot be trace points. 
And there are some that use qemu_hexdump which cannot be converted, 
either. I don't think it makes sense to convert the rest.

Cheers,
Stefan

[Qemu-devel] [PATCH v2 2/8] usb-ccid: Fix USB packet generation for 64-Bytes sized packets.

[Stefan Fritsch]

From: Stefan Fritsch <stefan_fritsch@genua.de>

An USB transfer must be terminated by a packet that is shorter than
the maximum USB packet size (usually 64Byte) reported by the device
in its descriptor(s). Thus if the last packet of a transfer is exactly
64-Bytes long,  we must send an additional zero length packet.
Fix this for bulk in transfers.

For bulk out transfers this is already ok, for interrupt transfers
this is not a problem as the pipe has a maximum packet size of 8
and transfers are only two bytes long.

Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
Signed-off-by: Christian Ehrhardt <christian_ehrhardt@genua.de>
---
 hw/usb/dev-smartcard-reader.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 624dc2c447..49791492ea 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1124,7 +1124,13 @@ static void ccid_bulk_in_copy_to_guest(USBCCIDState *s, USBPacket *p)
                         s->current_bulk_in->data + s->current_bulk_in->pos,
                         len);
         s->current_bulk_in->pos += len;
-        if (s->current_bulk_in->pos == s->current_bulk_in->len) {
+        /*
+         * The caller assumes that the packet continues if we fill the
+         * entire iov and the iov length matches the max packet size.
+         * Add an empty USB packet in this case.
+         */
+        if (s->current_bulk_in->pos == s->current_bulk_in->len
+            && (len < p->iov.size || p->iov.size < CCID_MAX_PACKET_SIZE)) {
             ccid_bulk_in_release(s);
         }
     } else {
-- 
2.11.0

[Qemu-devel] [PATCH v2 3/8] usb-ccid: Set protocol parameters based on card ATR

[Stefan Fritsch]

From: Stefan Fritsch <stefan_fritsch@genua.de>

For the ATR interface bytes encoding see ISO/IEC 7816-3. For the
interpretation of the protocol parameter block see the CCID
specification.

- Values that are not present in the ATR default to zero.
- TA_1 is used to fill bmFindexDindex.
- The high nibble of bmTCCKST0 must be the protocol (T=0 or T=1).
- For T=0 the bWaitingInteger can be found in the global byte TC_2,
  for T=1 the waiting integer is in TB_3 and bIFSC is in TA_3.

Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
Signed-off-by: Christian Ehrhardt <christian_ehrhardt@genua.de>
---
 hw/usb/dev-smartcard-reader.c | 66 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 54 insertions(+), 12 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 49791492ea..769949144b 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -801,7 +801,7 @@ static void ccid_write_parameters(USBCCIDState *s, CCID_Header *recv)
         return;
     }
     h->b.hdr.bMessageType = CCID_MESSAGE_TYPE_RDR_to_PC_Parameters;
-    h->b.hdr.dwLength = 0;
+    h->b.hdr.dwLength = (s->bProtocolNum == 1) ? 7 : 5;
     h->b.hdr.bSlot = recv->bSlot;
     h->b.hdr.bSeq = recv->bSeq;
     h->b.bStatus = ccid_calc_status(s);
@@ -871,6 +871,49 @@ static uint8_t atr_get_protocol_num(const uint8_t *atr, uint32_t len)
     return atr[i] & 0x0f;
 }
 
+/*
+ * Get a specific interface byte TX_y (X='A'..'D' y=1..3)
+ * See ISO/IEC 7816-3:2006 Chapter 8 for details.
+ */
+static uint8_t atr_get_TXY(const uint8_t *atr, uint32_t len, char x, int y)
+{
+    int pos;
+    uint8_t t0, ti, i;
+
+    if (x < 'A' || x > 'D') {
+        return 0;
+    }
+    x -= 'A';
+
+    if (len < 2) {
+        return 0;
+    }
+    t0 = atr[1] >> 4;
+    pos = 2;
+    ti = 1;
+    /* Skip TA_i..TD_i if present while not at requested index y */
+    while (ti < y) {
+        pos += !!(t0 & 0x01) + !!(t0 & 0x02) + !!(t0 & 0x04);
+        if (pos >= len || !(t0 & 0x08)) {
+            return 0;
+        }
+        t0 = atr[pos++] >> 4;
+        ti++;
+    }
+    if (!(t0 & 0x01 << x)) {
+        return 0;
+    }
+    for (i = 0; i < x; ++i) {
+        if (t0 & (1 << i)) {
+            pos++;
+        }
+    }
+    if (pos >= len) {
+        return 0;
+    }
+    return atr[pos];
+}
+
 static void ccid_write_data_block_atr(USBCCIDState *s, CCID_Header *recv)
 {
     const uint8_t *atr = NULL;
@@ -890,21 +933,20 @@ static void ccid_write_data_block_atr(USBCCIDState *s, CCID_Header *recv)
                                              : s->bProtocolNum);
     switch (atr_protocol_num) {
     case 0:
-        /* TODO: unimplemented ATR T0 parameters */
-        t0->bmFindexDindex = 0;
-        t0->bmTCCKST0 = 0;
+        t0->bmFindexDindex = atr_get_TXY(atr, len, 'A', 1);
+        t0->bmTCCKST0 = 0;      /* T0 */
         t0->bGuardTimeT0 = 0;
-        t0->bWaitingIntegerT0 = 0;
+        t0->bWaitingIntegerT0 = atr_get_TXY(atr, len, 'C', 2);
         t0->bClockStop = 0;
         break;
     case 1:
-        /* TODO: unimplemented ATR T1 parameters */
-        t1->bmFindexDindex = 0;
-        t1->bmTCCKST1 = 0;
-        t1->bGuardTimeT1 = 0;
-        t1->bWaitingIntegerT1 = 0;
-        t1->bClockStop = 0;
-        t1->bIFSC = 0;
+        /* NOTE: If present TD2 should specify protocl T=1 */
+        t1->bmFindexDindex = atr_get_TXY(atr, len, 'A', 1);
+        t1->bmTCCKST1 = 0x10;   /* T1 */
+        t1->bGuardTimeT1 = atr_get_TXY(atr, len, 'C', 1);
+        t1->bWaitingIntegerT1 = atr_get_TXY(atr, len, 'B', 3);
+        t1->bClockStop = 0x00;
+        t1->bIFSC = atr_get_TXY(atr, len, 'A', 3);
         t1->bNadValue = 0;
         break;
     default:
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 3/8] usb-ccid: Set protocol parameters based on card ATR

[Marc-André Lureau]

On Thu, Jul 20, 2017 at 11:11 AM Stefan Fritsch <sf@sfritsch.de> wrote:

> From: Stefan Fritsch <stefan_fritsch@genua.de>
>
> For the ATR interface bytes encoding see ISO/IEC 7816-3. For the
> interpretation of the protocol parameter block see the CCID
> specification.
>
> - Values that are not present in the ATR default to zero.
> - TA_1 is used to fill bmFindexDindex.
> - The high nibble of bmTCCKST0 must be the protocol (T=0 or T=1).
> - For T=0 the bWaitingInteger can be found in the global byte TC_2,
>   for T=1 the waiting integer is in TB_3 and bIFSC is in TA_3.
>
> Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
> Signed-off-by: Christian Ehrhardt <christian_ehrhardt@genua.de>
> ---
>  hw/usb/dev-smartcard-reader.c | 66
> +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 54 insertions(+), 12 deletions(-)
>
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index 49791492ea..769949144b 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -801,7 +801,7 @@ static void ccid_write_parameters(USBCCIDState *s,
> CCID_Header *recv)
>          return;
>      }
>      h->b.hdr.bMessageType = CCID_MESSAGE_TYPE_RDR_to_PC_Parameters;
> -    h->b.hdr.dwLength = 0;
> +    h->b.hdr.dwLength = (s->bProtocolNum == 1) ? 7 : 5;
>      h->b.hdr.bSlot = recv->bSlot;
>      h->b.hdr.bSeq = recv->bSeq;
>      h->b.bStatus = ccid_calc_status(s);
> @@ -871,6 +871,49 @@ static uint8_t atr_get_protocol_num(const uint8_t
> *atr, uint32_t len)
>      return atr[i] & 0x0f;
>  }
>
> +/*
> + * Get a specific interface byte TX_y (X='A'..'D' y=1..3)
> + * See ISO/IEC 7816-3:2006 Chapter 8 for details.
> + */
> +static uint8_t atr_get_TXY(const uint8_t *atr, uint32_t len, char x, int
> y)
> +{
> +    int pos;
> +    uint8_t t0, ti, i;
> +
> +    if (x < 'A' || x > 'D') {
> +        return 0;
> +    }
>

or you could assert it, since it's compile time constant

(I have vague memories of parsing ATR, but the rest looks ok, it might be
worth to include tests for that function though)


> +    x -= 'A';
> +
> +    if (len < 2) {
> +        return 0;
> +    }
> +    t0 = atr[1] >> 4;
> +    pos = 2;
> +    ti = 1;
> +    /* Skip TA_i..TD_i if present while not at requested index y */
> +    while (ti < y) {
> +        pos += !!(t0 & 0x01) + !!(t0 & 0x02) + !!(t0 & 0x04);
> +        if (pos >= len || !(t0 & 0x08)) {
> +            return 0;
> +        }
> +        t0 = atr[pos++] >> 4;
> +        ti++;
> +    }
> +    if (!(t0 & 0x01 << x)) {
> +        return 0;
> +    }
> +    for (i = 0; i < x; ++i) {
> +        if (t0 & (1 << i)) {
> +            pos++;
> +        }
> +    }
> +    if (pos >= len) {
> +        return 0;
> +    }
> +    return atr[pos];
> +}
> +
>  static void ccid_write_data_block_atr(USBCCIDState *s, CCID_Header *recv)
>  {
>      const uint8_t *atr = NULL;
> @@ -890,21 +933,20 @@ static void ccid_write_data_block_atr(USBCCIDState
> *s, CCID_Header *recv)
>                                               : s->bProtocolNum);
>      switch (atr_protocol_num) {
>      case 0:
> -        /* TODO: unimplemented ATR T0 parameters */
> -        t0->bmFindexDindex = 0;
> -        t0->bmTCCKST0 = 0;
> +        t0->bmFindexDindex = atr_get_TXY(atr, len, 'A', 1);
> +        t0->bmTCCKST0 = 0;      /* T0 */
>          t0->bGuardTimeT0 = 0;
> -        t0->bWaitingIntegerT0 = 0;
> +        t0->bWaitingIntegerT0 = atr_get_TXY(atr, len, 'C', 2);
>          t0->bClockStop = 0;
>          break;
>      case 1:
> -        /* TODO: unimplemented ATR T1 parameters */
> -        t1->bmFindexDindex = 0;
> -        t1->bmTCCKST1 = 0;
> -        t1->bGuardTimeT1 = 0;
> -        t1->bWaitingIntegerT1 = 0;
> -        t1->bClockStop = 0;
> -        t1->bIFSC = 0;
> +        /* NOTE: If present TD2 should specify protocl T=1 */
> +        t1->bmFindexDindex = atr_get_TXY(atr, len, 'A', 1);
> +        t1->bmTCCKST1 = 0x10;   /* T1 */
> +        t1->bGuardTimeT1 = atr_get_TXY(atr, len, 'C', 1);
> +        t1->bWaitingIntegerT1 = atr_get_TXY(atr, len, 'B', 3);
> +        t1->bClockStop = 0x00;
> +        t1->bIFSC = atr_get_TXY(atr, len, 'A', 3);
>          t1->bNadValue = 0;
>          break;
>      default:
> --
> 2.11.0
>
>
> --
Marc-André Lureau

[Qemu-devel] [PATCH v2 4/8] usb-ccid: Fix ATR parsing

[Stefan Fritsch]

From: Stefan Fritsch <stefan_fritsch@genua.de>

- The number of parameter set TA_i...TD_i is unlimited. The list ends
  if TD_i is not present or the high nibble is zero.
- If at least one protocol in any of the TD bytes is non-zero, the
  ATR must have a checksum.
- Add a missing length check before accessing TD.
- Fixup a wrong checksum but accept the ATR.

Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
Signed-off-by: Christian Ehrhardt <christian_ehrhardt@genua.de>
---
 hw/usb/ccid-card-passthru.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 45d96b03c6..e2e94ac1ba 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -146,8 +146,7 @@ static void ccid_card_vscard_handle_init(
 
 static int check_atr(PassthruState *card, uint8_t *data, int len)
 {
-    int historical_length, opt_bytes;
-    int td_count = 0;
+    int historical_length, opt_bytes, tck = 0;
     int td;
 
     if (len < 2) {
@@ -160,10 +159,8 @@ static int check_atr(PassthruState *card, uint8_t *data, int len)
                 data[0]);
         return 0;
     }
-    td_count = 0;
     td = data[1] >> 4;
-    while (td && td_count < 2 && opt_bytes + historical_length + 2 < len) {
-        td_count++;
+    while (td && opt_bytes + historical_length + 2 < len) {
         if (td & 0x1) {
             opt_bytes++;
         }
@@ -175,16 +172,37 @@ static int check_atr(PassthruState *card, uint8_t *data, int len)
         }
         if (td & 0x8) {
             opt_bytes++;
-            td = data[opt_bytes + 2] >> 4;
+            if (opt_bytes + 1 >= len) {
+                break;
+            }
+            /* Checksum byte must be present if T!=0 */
+            if (data[opt_bytes + 1] & 0xf) {
+                tck = 1;
+            }
+            td = data[opt_bytes + 1] >> 4;
+        } else {
+            td = 0;
         }
     }
-    if (len < 2 + historical_length + opt_bytes) {
+    if (len < 2 + historical_length + opt_bytes + tck) {
         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) {
+    if (tck) {
+        int i;
+        uint8_t cksum = 0;
+        for (i = 1; i < 2 + historical_length + opt_bytes; ++i) {
+            cksum ^= data[i];
+        }
+        if (cksum != data[i]) {
+            DPRINTF(card, D_WARN, "atr has bad checksum: "
+                "real=0x%x should be 0x%x (FIXED)\n", cksum, data[i]);
+            data[i] = cksum;
+        }
+    }
+    if (len > 2 + historical_length + opt_bytes + tck) {
         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]);
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 4/8] usb-ccid: Fix ATR parsing

[Marc-André Lureau]

On Thu, Jul 20, 2017 at 11:05 AM Stefan Fritsch <sf@sfritsch.de> wrote:

> From: Stefan Fritsch <stefan_fritsch@genua.de>
>
> - The number of parameter set TA_i...TD_i is unlimited. The list ends
>   if TD_i is not present or the high nibble is zero.
> - If at least one protocol in any of the TD bytes is non-zero, the
>   ATR must have a checksum.
> - Add a missing length check before accessing TD.
> - Fixup a wrong checksum but accept the ATR.
>
>
Could this patch be splitted to help review / bisect? Ideally, I would also
try to write tests for it. Thanks


> Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
> Signed-off-by: Christian Ehrhardt <christian_ehrhardt@genua.de>
> ---
>  hw/usb/ccid-card-passthru.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
> index 45d96b03c6..e2e94ac1ba 100644
> --- a/hw/usb/ccid-card-passthru.c
> +++ b/hw/usb/ccid-card-passthru.c
> @@ -146,8 +146,7 @@ static void ccid_card_vscard_handle_init(
>
>  static int check_atr(PassthruState *card, uint8_t *data, int len)
>  {
> -    int historical_length, opt_bytes;
> -    int td_count = 0;
> +    int historical_length, opt_bytes, tck = 0;
>      int td;
>
>      if (len < 2) {
> @@ -160,10 +159,8 @@ static int check_atr(PassthruState *card, uint8_t
> *data, int len)
>                  data[0]);
>          return 0;
>      }
> -    td_count = 0;
>      td = data[1] >> 4;
> -    while (td && td_count < 2 && opt_bytes + historical_length + 2 < len)
> {
> -        td_count++;
> +    while (td && opt_bytes + historical_length + 2 < len) {
>          if (td & 0x1) {
>              opt_bytes++;
>          }
> @@ -175,16 +172,37 @@ static int check_atr(PassthruState *card, uint8_t
> *data, int len)
>          }
>          if (td & 0x8) {
>              opt_bytes++;
> -            td = data[opt_bytes + 2] >> 4;
> +            if (opt_bytes + 1 >= len) {
> +                break;
> +            }
> +            /* Checksum byte must be present if T!=0 */
> +            if (data[opt_bytes + 1] & 0xf) {
> +                tck = 1;
> +            }
> +            td = data[opt_bytes + 1] >> 4;
> +        } else {
> +            td = 0;
>          }
>      }
> -    if (len < 2 + historical_length + opt_bytes) {
> +    if (len < 2 + historical_length + opt_bytes + tck) {
>          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) {
> +    if (tck) {
> +        int i;
> +        uint8_t cksum = 0;
> +        for (i = 1; i < 2 + historical_length + opt_bytes; ++i) {
> +            cksum ^= data[i];
> +        }
> +        if (cksum != data[i]) {
> +            DPRINTF(card, D_WARN, "atr has bad checksum: "
> +                "real=0x%x should be 0x%x (FIXED)\n", cksum, data[i]);
> +            data[i] = cksum;
> +        }
> +    }
> +    if (len > 2 + historical_length + opt_bytes + tck) {
>          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]);
> --
> 2.11.0
>
>
> --
Marc-André Lureau

[Qemu-devel] [PATCH v2 5/8] usb-ccid: Fix USB descriptor

[Stefan Fritsch]

From: Stefan Fritsch <stefan_fritsch@genua.de>

- We want to support both T=0 and T=1. Additionally, note that all
  fields in USB descriptors are little endian and the supported
  protocols are in the lowest byte the of the 32-Bit dwProtocols field.
- We want APDU level exchanges. This saves us from PPS exchanges and from
  APDU reassembly from T=1 packets.

Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
Signed-off-by: Christian Ehrhardt <christian_ehrhardt@genua.de>
---
 hw/usb/dev-smartcard-reader.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 769949144b..bc4dc35d3f 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -353,8 +353,8 @@ static const uint8_t qemu_ccid_descriptor[] = {
                      */
         0x07,       /* u8  bVoltageSupport; 01h - 5.0v, 02h - 3.0, 03 - 1.8 */
 
-        0x00, 0x00, /* u32 dwProtocols; RRRR PPPP. RRRR = 0000h.*/
-        0x01, 0x00, /* PPPP: 0001h = Protocol T=0, 0002h = Protocol T=1 */
+        0x03, 0x00, /* u32 dwProtocols; RRRR PPPP. RRRR = 0000h.*/
+        0x00, 0x00, /* PPPP: 0001h = Protocol T=0, 0002h = Protocol T=1 */
                     /* u32 dwDefaultClock; in kHZ (0x0fa0 is 4 MHz) */
         0xa0, 0x0f, 0x00, 0x00,
                     /* u32 dwMaximumClock; */
@@ -397,7 +397,7 @@ static const uint8_t qemu_ccid_descriptor[] = {
                      * insertion and removal. Must set bit 5 in bmAttributes
                      * in Configuration descriptor if 100000 is set.
                      */
-        0xfe, 0x04, 0x01, 0x00,
+        0xfe, 0x04, 0x04, 0x00,
                     /*
                      * u32 dwMaxCCIDMessageLength; For extended APDU in
                      * [261 + 10 , 65544 + 10]. Otherwise the minimum is
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 5/8] usb-ccid: Fix USB descriptor

[Marc-André Lureau]

On Thu, Jul 20, 2017 at 11:09 AM Stefan Fritsch <sf@sfritsch.de> wrote:

> From: Stefan Fritsch <stefan_fritsch@genua.de>
>
> - We want to support both T=0 and T=1. Additionally, note that all
>   fields in USB descriptors are little endian and the supported
>   protocols are in the lowest byte the of the 32-Bit dwProtocols field.
> - We want APDU level exchanges. This saves us from PPS exchanges and from
>   APDU reassembly from T=1 packets.
>
>
Did you test this with various versions of Windows?


> Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
> Signed-off-by: Christian Ehrhardt <christian_ehrhardt@genua.de>
> ---
>  hw/usb/dev-smartcard-reader.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index 769949144b..bc4dc35d3f 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -353,8 +353,8 @@ static const uint8_t qemu_ccid_descriptor[] = {
>                       */
>          0x07,       /* u8  bVoltageSupport; 01h - 5.0v, 02h - 3.0, 03 -
> 1.8 */
>
> -        0x00, 0x00, /* u32 dwProtocols; RRRR PPPP. RRRR = 0000h.*/
> -        0x01, 0x00, /* PPPP: 0001h = Protocol T=0, 0002h = Protocol T=1 */
> +        0x03, 0x00, /* u32 dwProtocols; RRRR PPPP. RRRR = 0000h.*/
> +        0x00, 0x00, /* PPPP: 0001h = Protocol T=0, 0002h = Protocol T=1 */
>                      /* u32 dwDefaultClock; in kHZ (0x0fa0 is 4 MHz) */
>          0xa0, 0x0f, 0x00, 0x00,
>                      /* u32 dwMaximumClock; */
> @@ -397,7 +397,7 @@ static const uint8_t qemu_ccid_descriptor[] = {
>                       * insertion and removal. Must set bit 5 in
> bmAttributes
>                       * in Configuration descriptor if 100000 is set.
>                       */
> -        0xfe, 0x04, 0x01, 0x00,
> +        0xfe, 0x04, 0x04, 0x00,
>                      /*
>                       * u32 dwMaxCCIDMessageLength; For extended APDU in
>                       * [261 + 10 , 65544 + 10]. Otherwise the minimum is
> --
> 2.11.0
>
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 5/8] usb-ccid: Fix USB descriptor

[Stefan Fritsch]

On Thu, 20 Jul 2017, Marc-André Lureau wrote:

> On Thu, Jul 20, 2017 at 11:09 AM Stefan Fritsch <sf@sfritsch.de> wrote:
> 
> > From: Stefan Fritsch <stefan_fritsch@genua.de>
> >
> > - We want to support both T=0 and T=1. Additionally, note that all
> >   fields in USB descriptors are little endian and the supported
> >   protocols are in the lowest byte the of the 32-Bit dwProtocols field.
> > - We want APDU level exchanges. This saves us from PPS exchanges and from
> >   APDU reassembly from T=1 packets.
> >
> >
> Did you test this with various versions of Windows?

Yes, we have tested that with windows 7, windows 10 and ubuntu 16.04 as 
guests.

> 
> 
> > Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
> > Signed-off-by: Christian Ehrhardt <christian_ehrhardt@genua.de>
> > ---
> >  hw/usb/dev-smartcard-reader.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> > index 769949144b..bc4dc35d3f 100644
> > --- a/hw/usb/dev-smartcard-reader.c
> > +++ b/hw/usb/dev-smartcard-reader.c
> > @@ -353,8 +353,8 @@ static const uint8_t qemu_ccid_descriptor[] = {
> >                       */
> >          0x07,       /* u8  bVoltageSupport; 01h - 5.0v, 02h - 3.0, 03 -
> > 1.8 */
> >
> > -        0x00, 0x00, /* u32 dwProtocols; RRRR PPPP. RRRR = 0000h.*/
> > -        0x01, 0x00, /* PPPP: 0001h = Protocol T=0, 0002h = Protocol T=1 */
> > +        0x03, 0x00, /* u32 dwProtocols; RRRR PPPP. RRRR = 0000h.*/
> > +        0x00, 0x00, /* PPPP: 0001h = Protocol T=0, 0002h = Protocol T=1 */
> >                      /* u32 dwDefaultClock; in kHZ (0x0fa0 is 4 MHz) */
> >          0xa0, 0x0f, 0x00, 0x00,
> >                      /* u32 dwMaximumClock; */
> > @@ -397,7 +397,7 @@ static const uint8_t qemu_ccid_descriptor[] = {
> >                       * insertion and removal. Must set bit 5 in
> > bmAttributes
> >                       * in Configuration descriptor if 100000 is set.
> >                       */
> > -        0xfe, 0x04, 0x01, 0x00,
> > +        0xfe, 0x04, 0x04, 0x00,
> >                      /*
> >                       * u32 dwMaxCCIDMessageLength; For extended APDU in
> >                       * [261 + 10 , 65544 + 10]. Otherwise the minimum is
> > --
> > 2.11.0
> >
> >
> > --
> Marc-André Lureau
> 

[Qemu-devel] [PATCH v2 6/8] usb-ccid: Fix chaining fields in CCID USB messages

[Stefan Fritsch]

From: Stefan Fritsch <stefan_fritsch@genua.de>

Incoming packets should never need chaining (bChainParamter => 0).
If they ever do we will have to fix this later. Zero is still better
than an uninitialized value.

Warn if outgoing packets try to use chaining (wLevelParameter != 0).
As we are doing APDU level exchanges, the driver should not need to
set these bits.

Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
Signed-off-by: Christian Ehrhardt <christian_ehrhardt@genua.de>
---
 hw/usb/dev-smartcard-reader.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index bc4dc35d3f..709c65c384 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -826,6 +826,7 @@ static void ccid_write_data_block(USBCCIDState *s, uint8_t slot, uint8_t seq,
     p->b.hdr.bSeq = seq;
     p->b.bStatus = ccid_calc_status(s);
     p->b.bError = s->bError;
+    p->bChainParameter = 0;
     if (p->b.bError) {
         DPRINTF(s, D_VERBOSE, "error %d\n", p->b.bError);
     }
@@ -1027,6 +1028,10 @@ static void ccid_on_apdu_from_guest(USBCCIDState *s, CCID_XferBlock *recv)
     len = le32_to_cpu(recv->hdr.dwLength);
     DPRINTF(s, 1, "%s: seq %d, len %d\n", __func__,
                 recv->hdr.bSeq, len);
+    if (le16_to_cpu(recv->wLevelParameter)) {
+        DPRINTF(s, D_WARN, "Unsupported non-zero level Parameter %x\n",
+                le16_to_cpu(recv->wLevelParameter));
+    }
     ccid_add_pending_answer(s, (CCID_Header *)recv);
     if (s->card && len <= BULK_OUT_DATA_SIZE) {
         ccid_card_apdu_from_guest(s->card, recv->abData, len);
-- 
2.11.0

[Qemu-devel] [PATCH v2 7/8] usb-ccid: Increase ccid max APDU size

[Stefan Fritsch]

From: Stefan Fritsch <stefan_fritsch@genua.de>

The emulated ccid smartcard reader has a fixed limit for APDUs from the
card. Increase this to 1024. We have seen software that tries to use 768
byte APDUs.

Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
---
 hw/usb/dev-smartcard-reader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 709c65c384..72324e8313 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -82,7 +82,7 @@ static void usb_packet_dump(int lvl, const char *dir, uint8_t *buf, size_t len)
 #define BULK_OUT_DATA_SIZE 65536
 #define PENDING_ANSWERS_NUM 128
 
-#define BULK_IN_BUF_SIZE 384
+#define BULK_IN_BUF_SIZE 1024
 #define BULK_IN_PENDING_NUM 8
 
 #define CCID_MAX_PACKET_SIZE                64
-- 
2.11.0

[Qemu-devel] [PATCH v2 8/8] usb-ccid: Reduce logging at level WARN

[Stefan Fritsch]

From: Stefan Fritsch <stefan_fritsch@genua.de>

Change all DPRINTF()s using (1 == WARN) to use symbolic
constants. Most of these DPRINTFs are now only logging at higher log
levels.

This allows to use ccid's debug level 1 == WARN in normal operation.

Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
---
 hw/usb/dev-smartcard-reader.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 72324e8313..fcc3ac1338 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -669,7 +669,7 @@ static void ccid_handle_reset(USBDevice *dev)
 {
     USBCCIDState *s = USB_CCID_DEV(dev);
 
-    DPRINTF(s, 1, "Reset\n");
+    DPRINTF(s, D_VERBOSE, "Reset\n");
 
     ccid_reset(s);
 }
@@ -713,7 +713,7 @@ static void ccid_handle_control(USBDevice *dev, USBPacket *p, int request,
     USBCCIDState *s = USB_CCID_DEV(dev);
     int ret;
 
-    DPRINTF(s, 1, "%s: got control %s (%x), value %x\n", __func__,
+    DPRINTF(s, D_TRACE, "%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) {
@@ -723,19 +723,20 @@ static void ccid_handle_control(USBDevice *dev, USBPacket *p, int request,
     switch (request) {
         /* Class specific requests.  */
     case ClassInterfaceOutRequest | CCID_CONTROL_ABORT:
-        DPRINTF(s, 1, "ccid_control abort UNIMPLEMENTED\n");
+        DPRINTF(s, D_INFO, "ccid_control abort UNIMPLEMENTED\n");
         p->status = USB_RET_STALL;
         break;
     case ClassInterfaceRequest | CCID_CONTROL_GET_CLOCK_FREQUENCIES:
-        DPRINTF(s, 1, "ccid_control get clock frequencies UNIMPLEMENTED\n");
+        DPRINTF(s, D_INFO,
+                "ccid_control get clock frequencies UNIMPLEMENTED\n");
         p->status = USB_RET_STALL;
         break;
     case ClassInterfaceRequest | CCID_CONTROL_GET_DATA_RATES:
-        DPRINTF(s, 1, "ccid_control get data rates UNIMPLEMENTED\n");
+        DPRINTF(s, D_INFO, "ccid_control get data rates UNIMPLEMENTED\n");
         p->status = USB_RET_STALL;
         break;
     default:
-        DPRINTF(s, 1, "got unsupported/bogus control %x, value %x\n",
+        DPRINTF(s, D_INFO, "got unsupported/bogus control %x, value %x\n",
                 request, value);
         p->status = USB_RET_STALL;
         break;
@@ -1020,13 +1021,13 @@ static void ccid_on_apdu_from_guest(USBCCIDState *s, CCID_XferBlock *recv)
     uint32_t len;
 
     if (ccid_card_status(s) != ICC_STATUS_PRESENT_ACTIVE) {
-        DPRINTF(s, 1,
+        DPRINTF(s, D_INFO,
                 "usb-ccid: not sending apdu to client, no card connected\n");
         ccid_write_data_block_error(s, recv->hdr.bSlot, recv->hdr.bSeq);
         return;
     }
     len = le32_to_cpu(recv->hdr.dwLength);
-    DPRINTF(s, 1, "%s: seq %d, len %d\n", __func__,
+    DPRINTF(s, D_TRACE, "%s: seq %d, len %d\n", __func__,
                 recv->hdr.bSeq, len);
     if (le16_to_cpu(recv->wLevelParameter)) {
         DPRINTF(s, D_WARN, "Unsupported non-zero level Parameter %x\n",
@@ -1087,7 +1088,7 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p)
         return;
     }
     if (s->bulk_out_pos - 10 != ccid_header->dwLength) {
-        DPRINTF(s, 1,
+        DPRINTF(s, D_WARN,
                 "usb-ccid: bulk_in: message size mismatch (got %d, expected %d)\n",
                 s->bulk_out_pos - 10, ccid_header->dwLength);
         goto err;
@@ -1101,7 +1102,7 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p)
         ccid_write_slot_status(s, ccid_header);
         break;
     case CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOn:
-        DPRINTF(s, 1, "%s: PowerOn: %d\n", __func__,
+        DPRINTF(s, D_VERBOSE, "%s: PowerOn: %d\n", __func__,
                 ((CCID_IccPowerOn *)(ccid_header))->bPowerSelect);
         s->powered = true;
         if (!ccid_card_inserted(s)) {
@@ -1137,7 +1138,7 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p)
         ccid_write_slot_status(s, ccid_header);
         break;
     default:
-        DPRINTF(s, 1,
+        DPRINTF(s, D_WARN,
                 "handle_data: ERROR: unhandled message type %Xh\n",
                 ccid_header->bMessageType);
         /*
@@ -1229,13 +1230,13 @@ static void ccid_handle_data(USBDevice *dev, USBPacket *p)
             }
             break;
         default:
-            DPRINTF(s, 1, "Bad endpoint\n");
+            DPRINTF(s, D_INFO, "Bad endpoint\n");
             p->status = USB_RET_STALL;
             break;
         }
         break;
     default:
-        DPRINTF(s, 1, "Bad token\n");
+        DPRINTF(s, D_INFO, "Bad token\n");
         p->status = USB_RET_STALL;
         break;
     }
@@ -1285,7 +1286,7 @@ void ccid_card_send_apdu_to_guest(CCIDCardState *card,
     Answer *answer;
 
     if (!ccid_has_pending_answers(s)) {
-        DPRINTF(s, 1, "CCID ERROR: got an APDU without pending answers\n");
+        DPRINTF(s, D_WARN, "CCID ERROR: got an APDU without pending answers\n");
         return;
     }
     s->bmCommandStatus = COMMAND_STATUS_NO_ERROR;
@@ -1295,7 +1296,7 @@ void ccid_card_send_apdu_to_guest(CCIDCardState *card,
         ccid_report_error_failed(s, ERROR_HW_ERROR);
         return;
     }
-    DPRINTF(s, 1, "APDU returned to guest %d (answer seq %d, slot %d)\n",
+    DPRINTF(s, D_TRACE, "APDU returned to guest %d (answer seq %d, slot %d)\n",
         len, answer->seq, answer->slot);
     ccid_write_data_block_answer(s, apdu, len);
 }
@@ -1317,7 +1318,7 @@ int ccid_card_ccid_attach(CCIDCardState *card)
     USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
     USBCCIDState *s = USB_CCID_DEV(dev);
 
-    DPRINTF(s, 1, "CCID Attach\n");
+    DPRINTF(s, D_VERBOSE, "CCID Attach\n");
     if (s->migration_state == MIGRATION_MIGRATED) {
         s->migration_state = MIGRATION_NONE;
     }
@@ -1330,7 +1331,7 @@ void ccid_card_ccid_detach(CCIDCardState *card)
     USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
     USBCCIDState *s = USB_CCID_DEV(dev);
 
-    DPRINTF(s, 1, "CCID Detach\n");
+    DPRINTF(s, D_VERBOSE, "CCID Detach\n");
     if (ccid_card_inserted(s)) {
         ccid_on_slot_change(s, false);
     }
@@ -1345,7 +1346,7 @@ void ccid_card_card_error(CCIDCardState *card, uint64_t error)
 
     s->bmCommandStatus = COMMAND_STATUS_FAILED;
     s->last_answer_error = error;
-    DPRINTF(s, 1, "VSC_Error: %" PRIX64 "\n", s->last_answer_error);
+    DPRINTF(s, D_INFO, "VSC_Error: %" PRIX64 "\n", s->last_answer_error);
     /* TODO: these errors should be more verbose and propagated to the guest.*/
     /*
      * We flush all pending answers on CardRemove message in ccid-card-passthru,
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 8/8] usb-ccid: Reduce logging at level WARN

[Marc-André Lureau]

On Thu, Jul 20, 2017 at 11:04 AM Stefan Fritsch <sf@sfritsch.de> wrote:

> From: Stefan Fritsch <stefan_fritsch@genua.de>
>
> Change all DPRINTF()s using (1 == WARN) to use symbolic
> constants. Most of these DPRINTFs are now only logging at higher log
> levels.
>
> This allows to use ccid's debug level 1 == WARN in normal operation.
>
> Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
> ---
>  hw/usb/dev-smartcard-reader.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index 72324e8313..fcc3ac1338 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -669,7 +669,7 @@ static void ccid_handle_reset(USBDevice *dev)
>  {
>      USBCCIDState *s = USB_CCID_DEV(dev);
>
> -    DPRINTF(s, 1, "Reset\n");
> +    DPRINTF(s, D_VERBOSE, "Reset\n");
>
>      ccid_reset(s);
>  }
> @@ -713,7 +713,7 @@ static void ccid_handle_control(USBDevice *dev,
> USBPacket *p, int request,
>      USBCCIDState *s = USB_CCID_DEV(dev);
>      int ret;
>
> -    DPRINTF(s, 1, "%s: got control %s (%x), value %x\n", __func__,
> +    DPRINTF(s, D_TRACE, "%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) {
> @@ -723,19 +723,20 @@ static void ccid_handle_control(USBDevice *dev,
> USBPacket *p, int request,
>      switch (request) {
>          /* Class specific requests.  */
>      case ClassInterfaceOutRequest | CCID_CONTROL_ABORT:
> -        DPRINTF(s, 1, "ccid_control abort UNIMPLEMENTED\n");
> +        DPRINTF(s, D_INFO, "ccid_control abort UNIMPLEMENTED\n");
>          p->status = USB_RET_STALL;
>          break;
>      case ClassInterfaceRequest | CCID_CONTROL_GET_CLOCK_FREQUENCIES:
> -        DPRINTF(s, 1, "ccid_control get clock frequencies
> UNIMPLEMENTED\n");
> +        DPRINTF(s, D_INFO,
> +                "ccid_control get clock frequencies UNIMPLEMENTED\n");
>          p->status = USB_RET_STALL;
>          break;
>      case ClassInterfaceRequest | CCID_CONTROL_GET_DATA_RATES:
> -        DPRINTF(s, 1, "ccid_control get data rates UNIMPLEMENTED\n");
> +        DPRINTF(s, D_INFO, "ccid_control get data rates UNIMPLEMENTED\n");
>          p->status = USB_RET_STALL;
>          break;
>

WARN was quite appropriate for unimplemented code reached, no?


>      default:
> -        DPRINTF(s, 1, "got unsupported/bogus control %x, value %x\n",
> +        DPRINTF(s, D_INFO, "got unsupported/bogus control %x, value %x\n",
>                  request, value);
>

I would also keep it at WARN here,  (as the unhandled case in
ccid_handle_bulk_out)

         p->status = USB_RET_STALL;
>          break;
> @@ -1020,13 +1021,13 @@ static void ccid_on_apdu_from_guest(USBCCIDState
> *s, CCID_XferBlock *recv)
>      uint32_t len;
>
>      if (ccid_card_status(s) != ICC_STATUS_PRESENT_ACTIVE) {
> -        DPRINTF(s, 1,
> +        DPRINTF(s, D_INFO,
>                  "usb-ccid: not sending apdu to client, no card
> connected\n");
>          ccid_write_data_block_error(s, recv->hdr.bSlot, recv->hdr.bSeq);
>          return;
>      }
>      len = le32_to_cpu(recv->hdr.dwLength);
> -    DPRINTF(s, 1, "%s: seq %d, len %d\n", __func__,
> +    DPRINTF(s, D_TRACE, "%s: seq %d, len %d\n", __func__,
>                  recv->hdr.bSeq, len);
>      if (le16_to_cpu(recv->wLevelParameter)) {
>          DPRINTF(s, D_WARN, "Unsupported non-zero level Parameter %x\n",
> @@ -1087,7 +1088,7 @@ static void ccid_handle_bulk_out(USBCCIDState *s,
> USBPacket *p)
>          return;
>      }
>      if (s->bulk_out_pos - 10 != ccid_header->dwLength) {
> -        DPRINTF(s, 1,
> +        DPRINTF(s, D_WARN,
>                  "usb-ccid: bulk_in: message size mismatch (got %d,
> expected %d)\n",
>                  s->bulk_out_pos - 10, ccid_header->dwLength);
>          goto err;
> @@ -1101,7 +1102,7 @@ static void ccid_handle_bulk_out(USBCCIDState *s,
> USBPacket *p)
>          ccid_write_slot_status(s, ccid_header);
>          break;
>      case CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOn:
> -        DPRINTF(s, 1, "%s: PowerOn: %d\n", __func__,
> +        DPRINTF(s, D_VERBOSE, "%s: PowerOn: %d\n", __func__,
>                  ((CCID_IccPowerOn *)(ccid_header))->bPowerSelect);
>          s->powered = true;
>          if (!ccid_card_inserted(s)) {
> @@ -1137,7 +1138,7 @@ static void ccid_handle_bulk_out(USBCCIDState *s,
> USBPacket *p)
>          ccid_write_slot_status(s, ccid_header);
>          break;
>      default:
> -        DPRINTF(s, 1,
> +        DPRINTF(s, D_WARN,
>                  "handle_data: ERROR: unhandled message type %Xh\n",
>                  ccid_header->bMessageType);
>
         /*
> @@ -1229,13 +1230,13 @@ static void ccid_handle_data(USBDevice *dev,
> USBPacket *p)
>              }
>              break;
>          default:
> -            DPRINTF(s, 1, "Bad endpoint\n");
> +            DPRINTF(s, D_INFO, "Bad endpoint\n");
>              p->status = USB_RET_STALL;
>              break;
>          }
>          break;
>      default:
> -        DPRINTF(s, 1, "Bad token\n");
> +        DPRINTF(s, D_INFO, "Bad token\n");
>          p->status = USB_RET_STALL;
>          break;
>      }
> @@ -1285,7 +1286,7 @@ void ccid_card_send_apdu_to_guest(CCIDCardState
> *card,
>      Answer *answer;
>
>      if (!ccid_has_pending_answers(s)) {
> -        DPRINTF(s, 1, "CCID ERROR: got an APDU without pending
> answers\n");
> +        DPRINTF(s, D_WARN, "CCID ERROR: got an APDU without pending
> answers\n");
>          return;
>      }
>      s->bmCommandStatus = COMMAND_STATUS_NO_ERROR;
> @@ -1295,7 +1296,7 @@ void ccid_card_send_apdu_to_guest(CCIDCardState
> *card,
>          ccid_report_error_failed(s, ERROR_HW_ERROR);
>          return;
>      }
> -    DPRINTF(s, 1, "APDU returned to guest %d (answer seq %d, slot %d)\n",
> +    DPRINTF(s, D_TRACE, "APDU returned to guest %d (answer seq %d, slot
> %d)\n",
>          len, answer->seq, answer->slot);
>      ccid_write_data_block_answer(s, apdu, len);
>  }
> @@ -1317,7 +1318,7 @@ int ccid_card_ccid_attach(CCIDCardState *card)
>      USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
>      USBCCIDState *s = USB_CCID_DEV(dev);
>
> -    DPRINTF(s, 1, "CCID Attach\n");
> +    DPRINTF(s, D_VERBOSE, "CCID Attach\n");
>      if (s->migration_state == MIGRATION_MIGRATED) {
>          s->migration_state = MIGRATION_NONE;
>      }
> @@ -1330,7 +1331,7 @@ void ccid_card_ccid_detach(CCIDCardState *card)
>      USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
>      USBCCIDState *s = USB_CCID_DEV(dev);
>
> -    DPRINTF(s, 1, "CCID Detach\n");
> +    DPRINTF(s, D_VERBOSE, "CCID Detach\n");
>      if (ccid_card_inserted(s)) {
>          ccid_on_slot_change(s, false);
>      }
> @@ -1345,7 +1346,7 @@ void ccid_card_card_error(CCIDCardState *card,
> uint64_t error)
>
>      s->bmCommandStatus = COMMAND_STATUS_FAILED;
>      s->last_answer_error = error;
> -    DPRINTF(s, 1, "VSC_Error: %" PRIX64 "\n", s->last_answer_error);
> +    DPRINTF(s, D_INFO, "VSC_Error: %" PRIX64 "\n", s->last_answer_error);
>

I'd keep WARN here too


>      /* TODO: these errors should be more verbose and propagated to the
> guest.*/
>      /*
>       * We flush all pending answers on CardRemove message in
> ccid-card-passthru,
> --
>
-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 8/8] usb-ccid: Reduce logging at level WARN

[Stefan Fritsch]

Thanks for the review.

On Thu, 20 Jul 2017, Marc-André Lureau wrote:

> On Thu, Jul 20, 2017 at 11:04 AM Stefan Fritsch <sf@sfritsch.de> wrote:
> 
> > From: Stefan Fritsch <stefan_fritsch@genua.de>
> >
> > Change all DPRINTF()s using (1 == WARN) to use symbolic
> > constants. Most of these DPRINTFs are now only logging at higher log
> > levels.
> >
> > This allows to use ccid's debug level 1 == WARN in normal operation.
> >
> > Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
> > ---
> >  hw/usb/dev-smartcard-reader.c | 37 +++++++++++++++++++------------------
> >  1 file changed, 19 insertions(+), 18 deletions(-)
> >
> > diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> > index 72324e8313..fcc3ac1338 100644
> > --- a/hw/usb/dev-smartcard-reader.c
> > +++ b/hw/usb/dev-smartcard-reader.c
> > @@ -669,7 +669,7 @@ static void ccid_handle_reset(USBDevice *dev)
> >  {
> >      USBCCIDState *s = USB_CCID_DEV(dev);
> >
> > -    DPRINTF(s, 1, "Reset\n");
> > +    DPRINTF(s, D_VERBOSE, "Reset\n");
> >
> >      ccid_reset(s);
> >  }
> > @@ -713,7 +713,7 @@ static void ccid_handle_control(USBDevice *dev,
> > USBPacket *p, int request,
> >      USBCCIDState *s = USB_CCID_DEV(dev);
> >      int ret;
> >
> > -    DPRINTF(s, 1, "%s: got control %s (%x), value %x\n", __func__,
> > +    DPRINTF(s, D_TRACE, "%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) {
> > @@ -723,19 +723,20 @@ static void ccid_handle_control(USBDevice *dev,
> > USBPacket *p, int request,
> >      switch (request) {
> >          /* Class specific requests.  */
> >      case ClassInterfaceOutRequest | CCID_CONTROL_ABORT:
> > -        DPRINTF(s, 1, "ccid_control abort UNIMPLEMENTED\n");
> > +        DPRINTF(s, D_INFO, "ccid_control abort UNIMPLEMENTED\n");
> >          p->status = USB_RET_STALL;
> >          break;
> >      case ClassInterfaceRequest | CCID_CONTROL_GET_CLOCK_FREQUENCIES:
> > -        DPRINTF(s, 1, "ccid_control get clock frequencies
> > UNIMPLEMENTED\n");
> > +        DPRINTF(s, D_INFO,
> > +                "ccid_control get clock frequencies UNIMPLEMENTED\n");
> >          p->status = USB_RET_STALL;
> >          break;
> >      case ClassInterfaceRequest | CCID_CONTROL_GET_DATA_RATES:
> > -        DPRINTF(s, 1, "ccid_control get data rates UNIMPLEMENTED\n");
> > +        DPRINTF(s, D_INFO, "ccid_control get data rates UNIMPLEMENTED\n");
> >          p->status = USB_RET_STALL;
> >          break;
> >
> 
> WARN was quite appropriate for unimplemented code reached, no?

True. I will re-check if these messages are triggered during normal 
operation for us.

> 
> 
> >      default:
> > -        DPRINTF(s, 1, "got unsupported/bogus control %x, value %x\n",
> > +        DPRINTF(s, D_INFO, "got unsupported/bogus control %x, value %x\n",
> >                  request, value);
> >
> 
> I would also keep it at WARN here,  (as the unhandled case in
> ccid_handle_bulk_out)
> 
>          p->status = USB_RET_STALL;
> >          break;
> > @@ -1020,13 +1021,13 @@ static void ccid_on_apdu_from_guest(USBCCIDState
> > *s, CCID_XferBlock *recv)
> >      uint32_t len;
> >
> >      if (ccid_card_status(s) != ICC_STATUS_PRESENT_ACTIVE) {
> > -        DPRINTF(s, 1,
> > +        DPRINTF(s, D_INFO,
> >                  "usb-ccid: not sending apdu to client, no card
> > connected\n");
> >          ccid_write_data_block_error(s, recv->hdr.bSlot, recv->hdr.bSeq);
> >          return;
> >      }
> >      len = le32_to_cpu(recv->hdr.dwLength);
> > -    DPRINTF(s, 1, "%s: seq %d, len %d\n", __func__,
> > +    DPRINTF(s, D_TRACE, "%s: seq %d, len %d\n", __func__,
> >                  recv->hdr.bSeq, len);
> >      if (le16_to_cpu(recv->wLevelParameter)) {
> >          DPRINTF(s, D_WARN, "Unsupported non-zero level Parameter %x\n",
> > @@ -1087,7 +1088,7 @@ static void ccid_handle_bulk_out(USBCCIDState *s,
> > USBPacket *p)
> >          return;
> >      }
> >      if (s->bulk_out_pos - 10 != ccid_header->dwLength) {
> > -        DPRINTF(s, 1,
> > +        DPRINTF(s, D_WARN,
> >                  "usb-ccid: bulk_in: message size mismatch (got %d,
> > expected %d)\n",
> >                  s->bulk_out_pos - 10, ccid_header->dwLength);
> >          goto err;
> > @@ -1101,7 +1102,7 @@ static void ccid_handle_bulk_out(USBCCIDState *s,
> > USBPacket *p)
> >          ccid_write_slot_status(s, ccid_header);
> >          break;
> >      case CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOn:
> > -        DPRINTF(s, 1, "%s: PowerOn: %d\n", __func__,
> > +        DPRINTF(s, D_VERBOSE, "%s: PowerOn: %d\n", __func__,
> >                  ((CCID_IccPowerOn *)(ccid_header))->bPowerSelect);
> >          s->powered = true;
> >          if (!ccid_card_inserted(s)) {
> > @@ -1137,7 +1138,7 @@ static void ccid_handle_bulk_out(USBCCIDState *s,
> > USBPacket *p)
> >          ccid_write_slot_status(s, ccid_header);
> >          break;
> >      default:
> > -        DPRINTF(s, 1,
> > +        DPRINTF(s, D_WARN,
> >                  "handle_data: ERROR: unhandled message type %Xh\n",
> >                  ccid_header->bMessageType);
> >
>          /*
> > @@ -1229,13 +1230,13 @@ static void ccid_handle_data(USBDevice *dev,
> > USBPacket *p)
> >              }
> >              break;
> >          default:
> > -            DPRINTF(s, 1, "Bad endpoint\n");
> > +            DPRINTF(s, D_INFO, "Bad endpoint\n");
> >              p->status = USB_RET_STALL;
> >              break;
> >          }
> >          break;
> >      default:
> > -        DPRINTF(s, 1, "Bad token\n");
> > +        DPRINTF(s, D_INFO, "Bad token\n");
> >          p->status = USB_RET_STALL;
> >          break;
> >      }
> > @@ -1285,7 +1286,7 @@ void ccid_card_send_apdu_to_guest(CCIDCardState
> > *card,
> >      Answer *answer;
> >
> >      if (!ccid_has_pending_answers(s)) {
> > -        DPRINTF(s, 1, "CCID ERROR: got an APDU without pending
> > answers\n");
> > +        DPRINTF(s, D_WARN, "CCID ERROR: got an APDU without pending
> > answers\n");
> >          return;
> >      }
> >      s->bmCommandStatus = COMMAND_STATUS_NO_ERROR;
> > @@ -1295,7 +1296,7 @@ void ccid_card_send_apdu_to_guest(CCIDCardState
> > *card,
> >          ccid_report_error_failed(s, ERROR_HW_ERROR);
> >          return;
> >      }
> > -    DPRINTF(s, 1, "APDU returned to guest %d (answer seq %d, slot %d)\n",
> > +    DPRINTF(s, D_TRACE, "APDU returned to guest %d (answer seq %d, slot
> > %d)\n",
> >          len, answer->seq, answer->slot);
> >      ccid_write_data_block_answer(s, apdu, len);
> >  }
> > @@ -1317,7 +1318,7 @@ int ccid_card_ccid_attach(CCIDCardState *card)
> >      USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> >      USBCCIDState *s = USB_CCID_DEV(dev);
> >
> > -    DPRINTF(s, 1, "CCID Attach\n");
> > +    DPRINTF(s, D_VERBOSE, "CCID Attach\n");
> >      if (s->migration_state == MIGRATION_MIGRATED) {
> >          s->migration_state = MIGRATION_NONE;
> >      }
> > @@ -1330,7 +1331,7 @@ void ccid_card_ccid_detach(CCIDCardState *card)
> >      USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> >      USBCCIDState *s = USB_CCID_DEV(dev);
> >
> > -    DPRINTF(s, 1, "CCID Detach\n");
> > +    DPRINTF(s, D_VERBOSE, "CCID Detach\n");
> >      if (ccid_card_inserted(s)) {
> >          ccid_on_slot_change(s, false);
> >      }
> > @@ -1345,7 +1346,7 @@ void ccid_card_card_error(CCIDCardState *card,
> > uint64_t error)
> >
> >      s->bmCommandStatus = COMMAND_STATUS_FAILED;
> >      s->last_answer_error = error;
> > -    DPRINTF(s, 1, "VSC_Error: %" PRIX64 "\n", s->last_answer_error);
> > +    DPRINTF(s, D_INFO, "VSC_Error: %" PRIX64 "\n", s->last_answer_error);
> >
> 
> I'd keep WARN here too
> 
> 
> >      /* TODO: these errors should be more verbose and propagated to the
> > guest.*/
> >      /*
> >       * We flush all pending answers on CardRemove message in
> > ccid-card-passthru,
> > --
> >
> 

Re: [Qemu-devel] [PATCH v2 8/8] usb-ccid: Reduce logging at level WARN

[Stefan Fritsch]

On Thu, 20 Jul 2017, Stefan Fritsch wrote:

> Thanks for the review.
> 
> On Thu, 20 Jul 2017, Marc-André Lureau wrote:
> 
> > On Thu, Jul 20, 2017 at 11:04 AM Stefan Fritsch <sf@sfritsch.de> wrote:
> > 
> > > From: Stefan Fritsch <stefan_fritsch@genua.de>
> > >
> > > Change all DPRINTF()s using (1 == WARN) to use symbolic
> > > constants. Most of these DPRINTFs are now only logging at higher log
> > > levels.
> > >
> > > This allows to use ccid's debug level 1 == WARN in normal operation.


> > >      case ClassInterfaceRequest | CCID_CONTROL_GET_DATA_RATES:
> > > -        DPRINTF(s, 1, "ccid_control get data rates UNIMPLEMENTED\n");
> > > +        DPRINTF(s, D_INFO, "ccid_control get data rates UNIMPLEMENTED\n");
> > >          p->status = USB_RET_STALL;
> > >          break;
> > >
> > 
> > WARN was quite appropriate for unimplemented code reached, no?
> 
> True. I will re-check if these messages are triggered during normal 
> operation for us.

I don't see them even with WARN. I will make all your suggested adjustions 
in the next version of the patchset.

Cheers,
Stefan