All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/1] USB CCID packet handling issue
@ 2018-05-16 11:55 jjelen
  2018-05-16 11:55 ` [Qemu-devel] [PATCH 1/1] hw/usb/dev-smartcard-reader: Handle 64 B USB packets jjelen
  0 siblings, 1 reply; 3+ messages in thread
From: jjelen @ 2018-05-16 11:55 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann, Jakub Jelen

From: Jakub Jelen <jjelen@redhat.com>

while working on libcacard, I encountered an issue with USB "packet
fragmentation" (for the lack of better knowledge how to call it) in the
CCID code.

It happens if I try to send exactly 34 B from the virtualized Smart
Card to the guest. These 34 B are packed into 64 B payload on USB
layer, which is the boundary where the "fragmentation" of USB 1.1
packets should happen.

In the guest, the packet is delivered, but not "committed", I see it in
wireshark with 3 seconds delay and with -ENOENT URB status, while pcscd
times out while waiting for this packet.

If I send one byte less or more, the packet(s) go through as expected.

I debugged up to the function, that is handling the data passing to the
USB emulation layer [1].

>From the debug logs, I can see that there is something clearly wrong.
For 63 B message, I can see the following:

usb-ccid: ccid_bulk_in_copy_to_guest: 64/63 req/act to guest (BULK_IN)
usb-ccid: ccid_bulk_in_copy_to_guest: returning short (EREMOTEIO) 63 <
64

But if I bump the length to 64 B, I see one correct message sending the
data and then hundreds of messages like this (assuming until the caller
timer times out):

usb-ccid: ccid_bulk_in_copy_to_guest: 64/64 req/act to guest (BULK_IN)
usb-ccid: ccid_bulk_in_copy_to_guest: returning short (EREMOTEIO) 0 <
64
usb-ccid: ccid_bulk_in_copy_to_guest: returning short (EREMOTEIO) 0 <
64
...

>From the USB specification (8.5.3.2 Variable-length Data Stage), the
following section seems to explain the issue:

>  When all of the data structure is returned to the host, the function
> should indicate that the Data stage is ended by returning a packet
> that is shorter than the MaxPacketSize for thepipe.  If the data
> structure is an exact multiple of wMaxPacketSize for the pipe, the
> function will return a zero-length packet to indicate the end of the
> Data stage.

The code does not have any check for the full packet size and it is not
sending zero-length packet in that case, only NAKs, which means we can
not send data now, the host retries and then comes the timeout.

The attached patch address the issue for me and was already reviewed by
Gerd Hoffmann.

Jakub Jelen (1):
  hw/usb/dev-smartcard-reader: Handle 64 B USB packets

 hw/usb/dev-smartcard-reader.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

-- 
2.14.3

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [Qemu-devel] [PATCH 1/1] hw/usb/dev-smartcard-reader: Handle 64 B USB packets
  2018-05-16 11:55 [Qemu-devel] [PATCH 0/1] USB CCID packet handling issue jjelen
@ 2018-05-16 11:55 ` jjelen
  2018-05-18  7:44   ` Gerd Hoffmann
  0 siblings, 1 reply; 3+ messages in thread
From: jjelen @ 2018-05-16 11:55 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann, Jakub Jelen

From: Jakub Jelen <jjelen@redhat.com>

The current code was not correctly handling 64 B (Max USB 1.1 payload size)
packets and therefore preventing some of the messages from smart card to
pass through to the guest.

If the smart card in host responded with 34 B of data in APDU layer, the
CCID headers added up to 64 B. The packet was send, but not correctly
committed per USB specification (8.5.3.2  Variable-length Data Stage):

>   When all of the data structure is returned to the host, the function
> should indicate that the Data stage is ended by returning a packet
> that is shorter than the MaxPacketSize for the pipe.  If the data
> structure is an exact multiple of wMaxPacketSize for the pipe, the
> function will return a zero-length packet to indicate the end of the
> Data stage.

This lead the guest applications to timeout while waiting for the rest
of data (the emulation layer is answering with NAK until the timeout).

This patch is checking the current maximum packet size and if the
payload of this size is detected, the message buffer is not yet released.
With the next call, the empty buffer is sent and the message buffer
is finally released.

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
---
 hw/usb/dev-smartcard-reader.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index cabb564788..f7451923f4 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1064,7 +1064,8 @@ err:
     return;
 }
 
-static void ccid_bulk_in_copy_to_guest(USBCCIDState *s, USBPacket *p)
+static void ccid_bulk_in_copy_to_guest(USBCCIDState *s, USBPacket *p,
+    unsigned int max_packet_size)
 {
     int len = 0;
 
@@ -1072,10 +1073,13 @@ static void ccid_bulk_in_copy_to_guest(USBCCIDState *s, USBPacket *p)
     if (s->current_bulk_in != NULL) {
         len = MIN(s->current_bulk_in->len - s->current_bulk_in->pos,
                   p->iov.size);
-        usb_packet_copy(p, s->current_bulk_in->data +
-                        s->current_bulk_in->pos, len);
+        if (len) {
+            usb_packet_copy(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) {
+        if (s->current_bulk_in->pos == s->current_bulk_in->len
+            && len != max_packet_size) {
             ccid_bulk_in_release(s);
         }
     } else {
@@ -1107,7 +1111,7 @@ static void ccid_handle_data(USBDevice *dev, USBPacket *p)
     case USB_TOKEN_IN:
         switch (p->ep->nr) {
         case CCID_BULK_IN_EP:
-            ccid_bulk_in_copy_to_guest(s, p);
+            ccid_bulk_in_copy_to_guest(s, p, dev->ep_ctl.max_packet_size);
             break;
         case CCID_INT_IN_EP:
             if (s->notify_slot_change) {
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] hw/usb/dev-smartcard-reader: Handle 64 B USB packets
  2018-05-16 11:55 ` [Qemu-devel] [PATCH 1/1] hw/usb/dev-smartcard-reader: Handle 64 B USB packets jjelen
@ 2018-05-18  7:44   ` Gerd Hoffmann
  0 siblings, 0 replies; 3+ messages in thread
From: Gerd Hoffmann @ 2018-05-18  7:44 UTC (permalink / raw)
  To: jjelen; +Cc: qemu-devel

On Wed, May 16, 2018 at 01:55:44PM +0200, jjelen@redhat.com wrote:
> From: Jakub Jelen <jjelen@redhat.com>
> 
> The current code was not correctly handling 64 B (Max USB 1.1 payload size)
> packets and therefore preventing some of the messages from smart card to
> pass through to the guest.

Added to usb queue.

thanks,
  Gerd

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-05-18  7:44 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-16 11:55 [Qemu-devel] [PATCH 0/1] USB CCID packet handling issue jjelen
2018-05-16 11:55 ` [Qemu-devel] [PATCH 1/1] hw/usb/dev-smartcard-reader: Handle 64 B USB packets jjelen
2018-05-18  7:44   ` Gerd Hoffmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.