From 0aeebc73b7976bae5cb7e9768e3d9a0fd9d634e8 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Thu, 16 Feb 2017 14:13:37 +0100 Subject: usb-ccid: better bulk_out error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add err goto label where we can jump to from all error conditions. STALL request on all errors. Reset position on all errors. Normal request processing is not in a else branch any more, so this code is reintended, there are no code changes in that part of the code though. Signed-off-by: Gerd Hoffmann Reviewed-by: Marc-André Lureau Message-id: 1487250819-23764-2-git-send-email-kraxel@redhat.com --- hw/usb/dev-smartcard-reader.c | 116 ++++++++++++++++++++++-------------------- 1 file changed, 61 insertions(+), 55 deletions(-) (limited to 'hw/usb/dev-smartcard-reader.c') diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c index 1325ea1..badcfcb 100644 --- a/hw/usb/dev-smartcard-reader.c +++ b/hw/usb/dev-smartcard-reader.c @@ -1001,8 +1001,7 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p) CCID_Header *ccid_header; if (p->iov.size + s->bulk_out_pos > BULK_OUT_DATA_SIZE) { - p->status = USB_RET_STALL; - return; + goto err; } ccid_header = (CCID_Header *)s->bulk_out_data; usb_packet_copy(p, s->bulk_out_data + s->bulk_out_pos, p->iov.size); @@ -1017,64 +1016,71 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p) DPRINTF(s, 1, "%s: bad USB_TOKEN_OUT length, should be at least 10 bytes\n", __func__); - } else { - 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, "%s: PowerOn: %d\n", __func__, + goto err; + } + + 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, "%s: PowerOn: %d\n", __func__, ((CCID_IccPowerOn *)(ccid_header))->bPowerSelect); - s->powered = true; - if (!ccid_card_inserted(s)) { - ccid_report_error_failed(s, ERROR_ICC_MUTE); - } - /* atr is written regardless of error. */ - ccid_write_data_block_atr(s, ccid_header); - break; - case CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOff: - ccid_reset_error_status(s); - s->powered = false; - ccid_write_slot_status(s, ccid_header); - break; - case CCID_MESSAGE_TYPE_PC_to_RDR_XfrBlock: - ccid_on_apdu_from_guest(s, (CCID_XferBlock *)s->bulk_out_data); - break; - case CCID_MESSAGE_TYPE_PC_to_RDR_SetParameters: - ccid_reset_error_status(s); - ccid_set_parameters(s, ccid_header); - ccid_write_parameters(s, ccid_header); - break; - case CCID_MESSAGE_TYPE_PC_to_RDR_ResetParameters: - ccid_reset_error_status(s); - ccid_reset_parameters(s); - ccid_write_parameters(s, ccid_header); - break; - case CCID_MESSAGE_TYPE_PC_to_RDR_GetParameters: - 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, + s->powered = true; + if (!ccid_card_inserted(s)) { + ccid_report_error_failed(s, ERROR_ICC_MUTE); + } + /* atr is written regardless of error. */ + ccid_write_data_block_atr(s, ccid_header); + break; + case CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOff: + ccid_reset_error_status(s); + s->powered = false; + ccid_write_slot_status(s, ccid_header); + break; + case CCID_MESSAGE_TYPE_PC_to_RDR_XfrBlock: + ccid_on_apdu_from_guest(s, (CCID_XferBlock *)s->bulk_out_data); + break; + case CCID_MESSAGE_TYPE_PC_to_RDR_SetParameters: + ccid_reset_error_status(s); + ccid_set_parameters(s, ccid_header); + ccid_write_parameters(s, ccid_header); + break; + case CCID_MESSAGE_TYPE_PC_to_RDR_ResetParameters: + ccid_reset_error_status(s); + ccid_reset_parameters(s); + ccid_write_parameters(s, ccid_header); + break; + case CCID_MESSAGE_TYPE_PC_to_RDR_GetParameters: + 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", ccid_header->bMessageType); - /* - * The caller is expecting the device to respond, tell it we - * don't support the operation. - */ - ccid_report_error_failed(s, ERROR_CMD_NOT_SUPPORTED); - ccid_write_slot_status(s, ccid_header); - break; - } + /* + * The caller is expecting the device to respond, tell it we + * don't support the operation. + */ + ccid_report_error_failed(s, ERROR_CMD_NOT_SUPPORTED); + ccid_write_slot_status(s, ccid_header); + break; } s->bulk_out_pos = 0; + return; + +err: + p->status = USB_RET_STALL; + s->bulk_out_pos = 0; + return; } static void ccid_bulk_in_copy_to_guest(USBCCIDState *s, USBPacket *p) -- cgit v1.1 From 7569c54642e8aa9fa03e250c7c578bd4d3747f00 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Thu, 16 Feb 2017 14:13:38 +0100 Subject: usb-ccid: move header size check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move up header size check, so we can use header fields in sanity checks (in followup patches). Also reword the debug message. Signed-off-by: Gerd Hoffmann Reviewed-by: Marc-André Lureau Message-id: 1487250819-23764-3-git-send-email-kraxel@redhat.com --- hw/usb/dev-smartcard-reader.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) (limited to 'hw/usb/dev-smartcard-reader.c') diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c index badcfcb..1acc1fb 100644 --- a/hw/usb/dev-smartcard-reader.c +++ b/hw/usb/dev-smartcard-reader.c @@ -1003,21 +1003,20 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p) if (p->iov.size + s->bulk_out_pos > BULK_OUT_DATA_SIZE) { goto err; } - ccid_header = (CCID_Header *)s->bulk_out_data; usb_packet_copy(p, 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__); + goto err; + } + + ccid_header = (CCID_Header *)s->bulk_out_data; if (p->iov.size == CCID_MAX_PACKET_SIZE) { DPRINTF(s, D_VERBOSE, "usb-ccid: bulk_in: expecting more packets (%zd/%d)\n", p->iov.size, ccid_header->dwLength); return; } - if (s->bulk_out_pos < 10) { - DPRINTF(s, 1, - "%s: bad USB_TOKEN_OUT length, should be at least 10 bytes\n", - __func__); - goto err; - } DPRINTF(s, D_MORE_INFO, "%s %x %s\n", __func__, ccid_header->bMessageType, -- cgit v1.1 From 31fb4444a485a348f8e2699d7c3dd15e1819ad2c Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Thu, 16 Feb 2017 14:13:39 +0100 Subject: usb-ccid: add check message size checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Check message size too when figuring whenever we should expect more data. Fix debug message to show useful data, p->iov.size is fixed anyway if we land there, print how much we got meanwhile instead. Also check announced message size against actual message size. That is a more general fix for CVE-2017-5898 than commit "c7dfbf3 usb: ccid: check ccid apdu length". Signed-off-by: Gerd Hoffmann Reviewed-by: Marc-André Lureau Message-id: 1487250819-23764-4-git-send-email-kraxel@redhat.com --- hw/usb/dev-smartcard-reader.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) (limited to 'hw/usb/dev-smartcard-reader.c') diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c index 1acc1fb..7cd4ed0 100644 --- a/hw/usb/dev-smartcard-reader.c +++ b/hw/usb/dev-smartcard-reader.c @@ -1011,12 +1011,19 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p) } ccid_header = (CCID_Header *)s->bulk_out_data; - if (p->iov.size == CCID_MAX_PACKET_SIZE) { + if ((s->bulk_out_pos - 10 < ccid_header->dwLength) && + (p->iov.size == CCID_MAX_PACKET_SIZE)) { DPRINTF(s, D_VERBOSE, - "usb-ccid: bulk_in: expecting more packets (%zd/%d)\n", - p->iov.size, ccid_header->dwLength); + "usb-ccid: bulk_in: expecting more packets (%d/%d)\n", + s->bulk_out_pos - 10, ccid_header->dwLength); return; } + if (s->bulk_out_pos - 10 != ccid_header->dwLength) { + DPRINTF(s, 1, + "usb-ccid: bulk_in: message size mismatch (got %d, expected %d)\n", + s->bulk_out_pos - 10, ccid_header->dwLength); + goto err; + } DPRINTF(s, D_MORE_INFO, "%s %x %s\n", __func__, ccid_header->bMessageType, -- cgit v1.1