[Qemu-devel] [PATCH v2 0/9] ccid-card-passthru: check buffer size parameter

[Qemu-devel] [PATCH v2 0/9] ccid-card-passthru: check buffer size parameter

[Philippe Mathieu-Daudé]

Hi,

This is the v2 of Prasad J Pandit first version [*], with Paolo's
review comment addressed.
This is a quick fix for CVE-2018-18438: "Integer overflow in
ccid_card_vscard_read() allows memory corruption".

Please review,

Phil.

[*] https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02200.html

Philippe Mathieu-Daudé (9):
  ccid-card-passthru: Move assertion in read() to can_read()
  ccid-card-passthru: Replace never trigger if statement by an assertion
  ccid-card-passthru: Assert on a stricter expression
  ccid-card-passthru: Let the chardev::read() be more generic
  ccid-card-passthru: Replace assert() by QEMU_BUILD_BUG_ON()
  ccid-card-passthru: Simplify the if() condition
  ccid-card-passthru: Use QERR_MISSING_PARAMETER
  ccid-card-passthru: Use size_t to hold size argument
  ccid-card-passthru: Use size_t for index

 hw/usb/ccid-card-passthru.c | 73 +++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 39 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/9] ccid-card-passthru: Move assertion in read() to can_read()

[Philippe Mathieu-Daudé]

chardev::read() depends of what chardev::can_read() returns, move the
assertion to can_read().

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/usb/ccid-card-passthru.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 0a6c657228..8bb1314f49 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -116,8 +116,8 @@ static int ccid_card_vscard_can_read(void *opaque)
 {
     PassthruState *card = opaque;
 
-    return VSCARD_IN_SIZE >= card->vscard_in_pos ?
-           VSCARD_IN_SIZE - card->vscard_in_pos : 0;
+    assert(card->vscard_in_pos <= VSCARD_IN_SIZE);
+    return VSCARD_IN_SIZE - card->vscard_in_pos;
 }
 
 static void ccid_card_vscard_handle_init(
@@ -282,7 +282,6 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int 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;
-- 
2.20.1

[Qemu-devel] [PATCH v2 2/9] ccid-card-passthru: Replace never trigger if statement by an assertion

[Philippe Mathieu-Daudé]

The right side of the comparison is the return value of can_read():
VSCARD_IN_SIZE - card->vscard_in_pos.
Since the 'size' argument of chardev::read() is bound to
what chardev::can_read() returns, this condition can never happen.

Add an assertion, which will always fail if card->vscard_in_pos >=
VSCARD_IN_SIZE), since size > 0.

This is a quick fix for CVE-2018-18438 "Integer overflow in
ccid_card_vscard_read() allows memory corruption".

Fixes: CVE-2018-18438
Reported-by: Arash Tohidi Chafi <tohidi.arash@gmail.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/usb/ccid-card-passthru.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 8bb1314f49..1676b5fc05 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -264,24 +264,12 @@ static void ccid_card_vscard_handle_message(PassthruState *card,
     }
 }
 
-static void ccid_card_vscard_drop_connection(PassthruState *card)
-{
-    qemu_chr_fe_deinit(&card->cs, true);
-    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;
 
-    if (card->vscard_in_pos + size > VSCARD_IN_SIZE) {
-        error_report("no room for data: pos %u +  size %d > %" PRId64 "."
-                     " dropping connection.",
-                     card->vscard_in_pos, size, VSCARD_IN_SIZE);
-        ccid_card_vscard_drop_connection(card);
-        return;
-    }
+    assert(size <= VSCARD_IN_SIZE - card->vscard_in_pos);
     assert(card->vscard_in_hdr < VSCARD_IN_SIZE);
     memcpy(card->vscard_in_data + card->vscard_in_pos, buf, size);
     card->vscard_in_pos += size;
-- 
2.20.1

[Qemu-devel] [PATCH v2 3/9] ccid-card-passthru: Assert on a stricter expression

[Philippe Mathieu-Daudé]

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/usb/ccid-card-passthru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 1676b5fc05..0c44b38fc2 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -270,7 +270,7 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size)
     VSCMsgHeader *hdr;
 
     assert(size <= VSCARD_IN_SIZE - card->vscard_in_pos);
-    assert(card->vscard_in_hdr < VSCARD_IN_SIZE);
+    assert(card->vscard_in_hdr < card->vscard_in_pos);
     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);
-- 
2.20.1

[Qemu-devel] [PATCH v2 4/9] ccid-card-passthru: Let the chardev::read() be more generic

[Philippe Mathieu-Daudé]

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/usb/ccid-card-passthru.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 0c44b38fc2..ba7c285ded 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -285,8 +285,14 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size)
         card->vscard_in_hdr += hdr->length + sizeof(VSCMsgHeader);
         hdr = (VSCMsgHeader *)(card->vscard_in_data + card->vscard_in_hdr);
     }
-    if (card->vscard_in_hdr == card->vscard_in_pos) {
-        card->vscard_in_pos = card->vscard_in_hdr = 0;
+
+    /* Drop any messages that were fully processed.  */
+    if (card->vscard_in_hdr > 0) {
+        memmove(card->vscard_in_data,
+                card->vscard_in_data + card->vscard_in_hdr,
+                card->vscard_in_pos - card->vscard_in_hdr);
+        card->vscard_in_pos -= card->vscard_in_hdr;
+        card->vscard_in_hdr = 0;
     }
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 5/9] ccid-card-passthru: Replace assert() by QEMU_BUILD_BUG_ON()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/usb/ccid-card-passthru.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index ba7c285ded..ccc3ffa7fa 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -29,6 +29,9 @@ do {                                                    \
 #define D_MORE_INFO 3
 #define D_VERBOSE 4
 
+/* maximum size of ATR - from 7816-3 */
+#define MAX_ATR_SIZE        40
+
 /* TODO: do we still need this? */
 static const uint8_t DEFAULT_ATR[] = {
 /*
@@ -41,10 +44,9 @@ static const uint8_t DEFAULT_ATR[] = {
  0x13, 0x08
 };
 
-#define VSCARD_IN_SIZE      (64 * KiB)
+QEMU_BUILD_BUG_ON(sizeof(DEFAULT_ATR) > MAX_ATR_SIZE);
 
-/* maximum size of ATR - from 7816-3 */
-#define MAX_ATR_SIZE        40
+#define VSCARD_IN_SIZE      (64 * KiB)
 
 typedef struct PassthruState PassthruState;
 
@@ -351,7 +353,6 @@ static void passthru_realize(CCIDCardState *base, Error **errp)
     }
     card->debug = parse_debug_env("QEMU_CCID_PASSTHRU_DEBUG", D_VERBOSE,
                                   card->debug);
-    assert(sizeof(DEFAULT_ATR) <= MAX_ATR_SIZE);
     memcpy(card->atr, DEFAULT_ATR, sizeof(DEFAULT_ATR));
     card->atr_length = sizeof(DEFAULT_ATR);
 }
-- 
2.20.1

[Qemu-devel] [PATCH v2 6/9] ccid-card-passthru: Simplify the if() condition

[Philippe Mathieu-Daudé]

Simplify the if() condition so we can remove an indent layer
and the code is easier to review.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/usb/ccid-card-passthru.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index ccc3ffa7fa..6cb8b2d26b 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -338,19 +338,17 @@ static void passthru_realize(CCIDCardState *base, Error **errp)
 {
     PassthruState *card = PASSTHRU_CCID_CARD(base);
 
-    card->vscard_in_pos = 0;
-    card->vscard_in_hdr = 0;
-    if (qemu_chr_fe_backend_connected(&card->cs)) {
-        DPRINTF(card, D_INFO, "ccid-card-passthru: initing chardev");
-        qemu_chr_fe_set_handlers(&card->cs,
-            ccid_card_vscard_can_read,
-            ccid_card_vscard_read,
-            ccid_card_vscard_event, NULL, card, NULL, true);
-        ccid_card_vscard_send_init(card);
-    } else {
+    if (!qemu_chr_fe_backend_connected(&card->cs)) {
         error_setg(errp, "missing chardev");
         return;
     }
+    card->vscard_in_pos = 0;
+    card->vscard_in_hdr = 0;
+    DPRINTF(card, D_INFO, "ccid-card-passthru: initing chardev");
+    qemu_chr_fe_set_handlers(&card->cs, ccid_card_vscard_can_read,
+                             ccid_card_vscard_read, ccid_card_vscard_event,
+                             NULL, card, NULL, true);
+    ccid_card_vscard_send_init(card);
     card->debug = parse_debug_env("QEMU_CCID_PASSTHRU_DEBUG", D_VERBOSE,
                                   card->debug);
     memcpy(card->atr, DEFAULT_ATR, sizeof(DEFAULT_ATR));
-- 
2.20.1

[Qemu-devel] [PATCH v2 7/9] ccid-card-passthru: Use QERR_MISSING_PARAMETER

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/usb/ccid-card-passthru.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 6cb8b2d26b..d63aa28584 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -16,6 +16,7 @@
 #include "qemu/sockets.h"
 #include "ccid.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
 
 #define DPRINTF(card, lvl, fmt, ...)                    \
 do {                                                    \
@@ -339,7 +340,7 @@ static void passthru_realize(CCIDCardState *base, Error **errp)
     PassthruState *card = PASSTHRU_CCID_CARD(base);
 
     if (!qemu_chr_fe_backend_connected(&card->cs)) {
-        error_setg(errp, "missing chardev");
+        error_setg(errp, QERR_MISSING_PARAMETER, "chardev");
         return;
     }
     card->vscard_in_pos = 0;
-- 
2.20.1

[Qemu-devel] [PATCH v2 8/9] ccid-card-passthru: Use size_t to hold size argument

[Philippe Mathieu-Daudé]

check_atr() is called once with a unsigned argument.
Since there is no need to use a signed type, use a size_t.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/usb/ccid-card-passthru.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index d63aa28584..083eb5ca08 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -149,9 +149,10 @@ static void ccid_card_vscard_handle_init(
     ccid_card_vscard_send_init(card);
 }
 
-static int check_atr(PassthruState *card, uint8_t *data, int len)
+static int check_atr(PassthruState *card, uint8_t *data, size_t len)
 {
-    int historical_length, opt_bytes;
+    size_t historical_length;
+    int opt_bytes;
     int td_count = 0;
     int td;
 
@@ -185,18 +186,18 @@ static int check_atr(PassthruState *card, uint8_t *data, int len)
     }
     if (len < 2 + historical_length + opt_bytes) {
         DPRINTF(card, D_WARN,
-            "atr too short: len %d, but historical_len %d, T1 0x%X\n",
+            "atr too short: len %zu, but historical_len %zu, 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",
+            "atr too long: len %zu, but hist/opt %zu/%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",
+            "atr passes check: %zu total length, %zu historical, %d optional\n",
             len, historical_length, opt_bytes);
 
     return 1;
-- 
2.20.1

[Qemu-devel] [PATCH v2 9/9] ccid-card-passthru: Use size_t for index

[Philippe Mathieu-Daudé]

The variable 'opt_bytes' is an index to the data[] array.
Use size_t for indexes.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/usb/ccid-card-passthru.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 083eb5ca08..664e8a1b54 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -152,7 +152,7 @@ static void ccid_card_vscard_handle_init(
 static int check_atr(PassthruState *card, uint8_t *data, size_t len)
 {
     size_t historical_length;
-    int opt_bytes;
+    size_t opt_bytes;
     int td_count = 0;
     int td;
 
@@ -192,12 +192,13 @@ static int check_atr(PassthruState *card, uint8_t *data, size_t len)
     }
     if (len > 2 + historical_length + opt_bytes) {
         DPRINTF(card, D_WARN,
-            "atr too long: len %zu, but hist/opt %zu/%d, T1 0x%X\n",
+            "atr too long: len %zu, but hist/opt %zu/%zu, T1 0x%X\n",
             len, historical_length, opt_bytes, data[1]);
         /* let it through */
     }
     DPRINTF(card, D_VERBOSE,
-            "atr passes check: %zu total length, %zu historical, %d optional\n",
+            "atr passes check: %zu total length,"
+            " %zu historical, %zu optional\n",
             len, historical_length, opt_bytes);
 
     return 1;
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 1/9] ccid-card-passthru: Move assertion in read() to can_read()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

On 2/14/19 2:19 PM, Philippe Mathieu-Daudé wrote:
> chardev::read() depends of what chardev::can_read() returns, move the

s/of/on/

> assertion to can_read().
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/usb/ccid-card-passthru.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 7/9] ccid-card-passthru: Use QERR_MISSING_PARAMETER

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

On 2/14/19 2:19 PM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/usb/ccid-card-passthru.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
> index 6cb8b2d26b..d63aa28584 100644
> --- a/hw/usb/ccid-card-passthru.c
> +++ b/hw/usb/ccid-card-passthru.c
> @@ -16,6 +16,7 @@
>  #include "qemu/sockets.h"
>  #include "ccid.h"
>  #include "qapi/error.h"
> +#include "qapi/qmp/qerror.h"
>  
>  #define DPRINTF(card, lvl, fmt, ...)                    \
>  do {                                                    \
> @@ -339,7 +340,7 @@ static void passthru_realize(CCIDCardState *base, Error **errp)
>      PassthruState *card = PASSTHRU_CCID_CARD(base);
>  
>      if (!qemu_chr_fe_backend_connected(&card->cs)) {
> -        error_setg(errp, "missing chardev");
> +        error_setg(errp, QERR_MISSING_PARAMETER, "chardev");

We're trying to get rid of QERR_ usage, not add more. But this
particular one seems to be a wash:

$ git grep '"missing ' | wc -l
24
$ git grep QERR_MISSING_P | wc -l
25

so I'd be interested if Markus has an opinion on this one.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 1/9] ccid-card-passthru: Move assertion in read() to can_read()

[Wei Yang]

On Thu, Feb 14, 2019 at 09:19:31PM +0100, Philippe Mathieu-Daudé wrote:
>chardev::read() depends of what chardev::can_read() returns, move the
>assertion to can_read().
>
>Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>---
> hw/usb/ccid-card-passthru.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
>diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
>index 0a6c657228..8bb1314f49 100644
>--- a/hw/usb/ccid-card-passthru.c
>+++ b/hw/usb/ccid-card-passthru.c
>@@ -116,8 +116,8 @@ static int ccid_card_vscard_can_read(void *opaque)
> {
>     PassthruState *card = opaque;
> 
>-    return VSCARD_IN_SIZE >= card->vscard_in_pos ?
>-           VSCARD_IN_SIZE - card->vscard_in_pos : 0;
>+    assert(card->vscard_in_pos <= VSCARD_IN_SIZE);

Do you have special reason change "<" to "<="?

>+    return VSCARD_IN_SIZE - card->vscard_in_pos;
> }
> 
> static void ccid_card_vscard_handle_init(
>@@ -282,7 +282,6 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int 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;
>-- 
>2.20.1
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 3/9] ccid-card-passthru: Assert on a stricter expression

[Wei Yang]

Would it be better to have some description?

On Thu, Feb 14, 2019 at 09:19:33PM +0100, Philippe Mathieu-Daudé wrote:
>Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>---
> hw/usb/ccid-card-passthru.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
>index 1676b5fc05..0c44b38fc2 100644
>--- a/hw/usb/ccid-card-passthru.c
>+++ b/hw/usb/ccid-card-passthru.c
>@@ -270,7 +270,7 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size)
>     VSCMsgHeader *hdr;
> 
>     assert(size <= VSCARD_IN_SIZE - card->vscard_in_pos);
>-    assert(card->vscard_in_hdr < VSCARD_IN_SIZE);
>+    assert(card->vscard_in_hdr < card->vscard_in_pos);
>     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);
>-- 
>2.20.1
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 2/9] ccid-card-passthru: Replace never trigger if statement by an assertion

[Marc-André Lureau]

Hi

On Thu, Feb 14, 2019 at 9:20 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> The right side of the comparison is the return value of can_read():
> VSCARD_IN_SIZE - card->vscard_in_pos.
> Since the 'size' argument of chardev::read() is bound to
> what chardev::can_read() returns, this condition can never happen.

I think so too, because vscard_in_pos is unchanged between the 2
callbacks (or set to 0 in break event).

>
> Add an assertion, which will always fail if card->vscard_in_pos >=
> VSCARD_IN_SIZE), since size > 0.

If "size > VSCARD_IN_SIZE - card->vscard_in_pos" this is a chardev
bug. But which backend does that?

Iow, did we ever reach the "no room for data" error?

>
> This is a quick fix for CVE-2018-18438 "Integer overflow in
> ccid_card_vscard_read() allows memory corruption".

I have a hard time to find how that memory corruption can happen. It
would be a broken chardev (one calling qemu_chr_be_write() with a size
bigger than qemu_chr_be_can_write()). It would need to be fixed. But
which one does that?

>
> Fixes: CVE-2018-18438
> Reported-by: Arash Tohidi Chafi <tohidi.arash@gmail.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/usb/ccid-card-passthru.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
> index 8bb1314f49..1676b5fc05 100644
> --- a/hw/usb/ccid-card-passthru.c
> +++ b/hw/usb/ccid-card-passthru.c
> @@ -264,24 +264,12 @@ static void ccid_card_vscard_handle_message(PassthruState *card,
>      }
>  }
>
> -static void ccid_card_vscard_drop_connection(PassthruState *card)
> -{
> -    qemu_chr_fe_deinit(&card->cs, true);
> -    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;
>
> -    if (card->vscard_in_pos + size > VSCARD_IN_SIZE) {
> -        error_report("no room for data: pos %u +  size %d > %" PRId64 "."
> -                     " dropping connection.",
> -                     card->vscard_in_pos, size, VSCARD_IN_SIZE);
> -        ccid_card_vscard_drop_connection(card);
> -        return;
> -    }
> +    assert(size <= VSCARD_IN_SIZE - card->vscard_in_pos);
>      assert(card->vscard_in_hdr < VSCARD_IN_SIZE);
>      memcpy(card->vscard_in_data + card->vscard_in_pos, buf, size);
>      card->vscard_in_pos += size;
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH v2 1/9] ccid-card-passthru: Move assertion in read() to can_read()

[Marc-André Lureau]

Hi

On Thu, Feb 14, 2019 at 9:19 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> chardev::read() depends of what chardev::can_read() returns, move the
> assertion to can_read().
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Moving to can_read() makes sense, because nothing (except the chardev
BREAK event) should update vscard_in_pos between the can_read() and
read() callback.

Changing the condition from < to <= could use some explanation. The
can_read() callback should handle the case where the vscard_in buffer
is full (adding = is necessary). And the read() callback should not be
called with size == 0, when card->vscard_in_pos == VSCARD_IN_SIZE (no
data to read).

But it wouldn't harm to leave the existing assert().

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





> ---
>  hw/usb/ccid-card-passthru.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
> index 0a6c657228..8bb1314f49 100644
> --- a/hw/usb/ccid-card-passthru.c
> +++ b/hw/usb/ccid-card-passthru.c
> @@ -116,8 +116,8 @@ static int ccid_card_vscard_can_read(void *opaque)
>  {
>      PassthruState *card = opaque;
>
> -    return VSCARD_IN_SIZE >= card->vscard_in_pos ?
> -           VSCARD_IN_SIZE - card->vscard_in_pos : 0;
> +    assert(card->vscard_in_pos <= VSCARD_IN_SIZE);
> +    return VSCARD_IN_SIZE - card->vscard_in_pos;
>  }
>
>  static void ccid_card_vscard_handle_init(
> @@ -282,7 +282,6 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int 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;
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH v2 3/9] ccid-card-passthru: Assert on a stricter expression

[Marc-André Lureau]

Hi

On Thu, Feb 14, 2019 at 9:20 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/usb/ccid-card-passthru.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
> index 1676b5fc05..0c44b38fc2 100644
> --- a/hw/usb/ccid-card-passthru.c
> +++ b/hw/usb/ccid-card-passthru.c
> @@ -270,7 +270,7 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size)
>      VSCMsgHeader *hdr;
>
>      assert(size <= VSCARD_IN_SIZE - card->vscard_in_pos);
> -    assert(card->vscard_in_hdr < VSCARD_IN_SIZE);
> +    assert(card->vscard_in_hdr < card->vscard_in_pos);


I think you need "<=".

card->vscard_in_hdr is updated in the loop and may reach card->vscard_in_pos:

    while ((card->vscard_in_pos - card->vscard_in_hdr >= sizeof(VSCMsgHeader))
         &&(card->vscard_in_pos - card->vscard_in_hdr >=
                                  sizeof(VSCMsgHeader) + ntohl(hdr->length))) {
...
        card->vscard_in_hdr += hdr->length + sizeof(VSCMsgHeader);
...
    }

if "hdr->length + sizeof(VSCMsgHeader)" == "card->vscard_in_pos -
card->vscard_in_hdr", card->vscard_in_hdr == card->vscard_in_pos after
the loop.


I think the existing assert() could also stay.

>      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);
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH v2 4/9] ccid-card-passthru: Let the chardev::read() be more generic

[Marc-André Lureau]

Hi

On Thu, Feb 14, 2019 at 9:20 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/usb/ccid-card-passthru.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
> index 0c44b38fc2..ba7c285ded 100644
> --- a/hw/usb/ccid-card-passthru.c
> +++ b/hw/usb/ccid-card-passthru.c
> @@ -285,8 +285,14 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size)
>          card->vscard_in_hdr += hdr->length + sizeof(VSCMsgHeader);
>          hdr = (VSCMsgHeader *)(card->vscard_in_data + card->vscard_in_hdr);
>      }
> -    if (card->vscard_in_hdr == card->vscard_in_pos) {
> -        card->vscard_in_pos = card->vscard_in_hdr = 0;

Interesting, it looks like we could end in a blocking condition today.

card->vscard_in_pos - card->vscard_in_hdr could not have enough room
to process an incoming message. After filling the buffer, it would
stop reading.

> +
> +    /* Drop any messages that were fully processed.  */
> +    if (card->vscard_in_hdr > 0) {
> +        memmove(card->vscard_in_data,
> +                card->vscard_in_data + card->vscard_in_hdr,
> +                card->vscard_in_pos - card->vscard_in_hdr);
> +        card->vscard_in_pos -= card->vscard_in_hdr;
> +        card->vscard_in_hdr = 0;
>      }
>  }

At least, by moving data around, this would leave always enough space
for the header to be fully read.

But I think we should add a condition like
card->vscard_in_hdr + hdr->length + sizeof(VSCMsgHeader) <=
VSCARD_IN_SIZE, to make sure the incoming message fits in the
vscard_in_data buffer, else disconnect?

>
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH v2 5/9] ccid-card-passthru: Replace assert() by QEMU_BUILD_BUG_ON()

[Marc-André Lureau]

On Thu, Feb 14, 2019 at 9:20 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

> ---
>  hw/usb/ccid-card-passthru.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
> index ba7c285ded..ccc3ffa7fa 100644
> --- a/hw/usb/ccid-card-passthru.c
> +++ b/hw/usb/ccid-card-passthru.c
> @@ -29,6 +29,9 @@ do {                                                    \
>  #define D_MORE_INFO 3
>  #define D_VERBOSE 4
>
> +/* maximum size of ATR - from 7816-3 */
> +#define MAX_ATR_SIZE        40
> +
>  /* TODO: do we still need this? */
>  static const uint8_t DEFAULT_ATR[] = {
>  /*
> @@ -41,10 +44,9 @@ static const uint8_t DEFAULT_ATR[] = {
>   0x13, 0x08
>  };
>
> -#define VSCARD_IN_SIZE      (64 * KiB)
> +QEMU_BUILD_BUG_ON(sizeof(DEFAULT_ATR) > MAX_ATR_SIZE);
>
> -/* maximum size of ATR - from 7816-3 */
> -#define MAX_ATR_SIZE        40
> +#define VSCARD_IN_SIZE      (64 * KiB)
>
>  typedef struct PassthruState PassthruState;
>
> @@ -351,7 +353,6 @@ static void passthru_realize(CCIDCardState *base, Error **errp)
>      }
>      card->debug = parse_debug_env("QEMU_CCID_PASSTHRU_DEBUG", D_VERBOSE,
>                                    card->debug);
> -    assert(sizeof(DEFAULT_ATR) <= MAX_ATR_SIZE);
>      memcpy(card->atr, DEFAULT_ATR, sizeof(DEFAULT_ATR));
>      card->atr_length = sizeof(DEFAULT_ATR);
>  }
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH v2 6/9] ccid-card-passthru: Simplify the if() condition

[Marc-André Lureau]

Hi
On Thu, Feb 14, 2019 at 9:20 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Simplify the if() condition so we can remove an indent layer
> and the code is easier to review.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

> ---
>  hw/usb/ccid-card-passthru.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
> index ccc3ffa7fa..6cb8b2d26b 100644
> --- a/hw/usb/ccid-card-passthru.c
> +++ b/hw/usb/ccid-card-passthru.c
> @@ -338,19 +338,17 @@ static void passthru_realize(CCIDCardState *base, Error **errp)
>  {
>      PassthruState *card = PASSTHRU_CCID_CARD(base);
>
> -    card->vscard_in_pos = 0;
> -    card->vscard_in_hdr = 0;

The 0 assignment are needless too (objects are memset 0), but not a
problem either.

> -    if (qemu_chr_fe_backend_connected(&card->cs)) {
> -        DPRINTF(card, D_INFO, "ccid-card-passthru: initing chardev");
> -        qemu_chr_fe_set_handlers(&card->cs,
> -            ccid_card_vscard_can_read,
> -            ccid_card_vscard_read,
> -            ccid_card_vscard_event, NULL, card, NULL, true);
> -        ccid_card_vscard_send_init(card);
> -    } else {
> +    if (!qemu_chr_fe_backend_connected(&card->cs)) {
>          error_setg(errp, "missing chardev");
>          return;
>      }
> +    card->vscard_in_pos = 0;
> +    card->vscard_in_hdr = 0;
> +    DPRINTF(card, D_INFO, "ccid-card-passthru: initing chardev");
> +    qemu_chr_fe_set_handlers(&card->cs, ccid_card_vscard_can_read,
> +                             ccid_card_vscard_read, ccid_card_vscard_event,
> +                             NULL, card, NULL, true);
> +    ccid_card_vscard_send_init(card);
>      card->debug = parse_debug_env("QEMU_CCID_PASSTHRU_DEBUG", D_VERBOSE,
>                                    card->debug);
>      memcpy(card->atr, DEFAULT_ATR, sizeof(DEFAULT_ATR));
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH v2 8/9] ccid-card-passthru: Use size_t to hold size argument

[Marc-André Lureau]

Hi

On Thu, Feb 14, 2019 at 9:28 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> check_atr() is called once with a unsigned argument.
> Since there is no need to use a signed type, use a size_t.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

and make data const?

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

> ---
>  hw/usb/ccid-card-passthru.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
> index d63aa28584..083eb5ca08 100644
> --- a/hw/usb/ccid-card-passthru.c
> +++ b/hw/usb/ccid-card-passthru.c
> @@ -149,9 +149,10 @@ static void ccid_card_vscard_handle_init(
>      ccid_card_vscard_send_init(card);
>  }
>
> -static int check_atr(PassthruState *card, uint8_t *data, int len)
> +static int check_atr(PassthruState *card, uint8_t *data, size_t len)
>  {
> -    int historical_length, opt_bytes;
> +    size_t historical_length;
> +    int opt_bytes;
>      int td_count = 0;
>      int td;
>
> @@ -185,18 +186,18 @@ static int check_atr(PassthruState *card, uint8_t *data, int len)
>      }
>      if (len < 2 + historical_length + opt_bytes) {
>          DPRINTF(card, D_WARN,
> -            "atr too short: len %d, but historical_len %d, T1 0x%X\n",
> +            "atr too short: len %zu, but historical_len %zu, 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",
> +            "atr too long: len %zu, but hist/opt %zu/%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",
> +            "atr passes check: %zu total length, %zu historical, %d optional\n",
>              len, historical_length, opt_bytes);
>
>      return 1;
> --
> 2.20.1
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 9/9] ccid-card-passthru: Use size_t for index

[Marc-André Lureau]

On Thu, Feb 14, 2019 at 9:27 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> The variable 'opt_bytes' is an index to the data[] array.
> Use size_t for indexes.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

> ---
>  hw/usb/ccid-card-passthru.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
> index 083eb5ca08..664e8a1b54 100644
> --- a/hw/usb/ccid-card-passthru.c
> +++ b/hw/usb/ccid-card-passthru.c
> @@ -152,7 +152,7 @@ static void ccid_card_vscard_handle_init(
>  static int check_atr(PassthruState *card, uint8_t *data, size_t len)
>  {
>      size_t historical_length;
> -    int opt_bytes;
> +    size_t opt_bytes;
>      int td_count = 0;
>      int td;
>
> @@ -192,12 +192,13 @@ static int check_atr(PassthruState *card, uint8_t *data, size_t len)
>      }
>      if (len > 2 + historical_length + opt_bytes) {
>          DPRINTF(card, D_WARN,
> -            "atr too long: len %zu, but hist/opt %zu/%d, T1 0x%X\n",
> +            "atr too long: len %zu, but hist/opt %zu/%zu, T1 0x%X\n",
>              len, historical_length, opt_bytes, data[1]);
>          /* let it through */
>      }
>      DPRINTF(card, D_VERBOSE,
> -            "atr passes check: %zu total length, %zu historical, %d optional\n",
> +            "atr passes check: %zu total length,"
> +            " %zu historical, %zu optional\n",
>              len, historical_length, opt_bytes);
>
>      return 1;
> --
> 2.20.1
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 2/9] ccid-card-passthru: Replace never trigger if statement by an assertion

[Philippe Mathieu-Daudé]

On 2/15/19 11:59 AM, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Feb 14, 2019 at 9:20 PM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>>
>> The right side of the comparison is the return value of can_read():
>> VSCARD_IN_SIZE - card->vscard_in_pos.
>> Since the 'size' argument of chardev::read() is bound to
>> what chardev::can_read() returns, this condition can never happen.
> 
> I think so too, because vscard_in_pos is unchanged between the 2
> callbacks (or set to 0 in break event).
> 
>>
>> Add an assertion, which will always fail if card->vscard_in_pos >=
>> VSCARD_IN_SIZE), since size > 0.
> 
> If "size > VSCARD_IN_SIZE - card->vscard_in_pos" this is a chardev
> bug. But which backend does that?
> 
> Iow, did we ever reach the "no room for data" error?
> 
>>
>> This is a quick fix for CVE-2018-18438 "Integer overflow in
>> ccid_card_vscard_read() allows memory corruption".
> 
> I have a hard time to find how that memory corruption can happen. It
> would be a broken chardev (one calling qemu_chr_be_write() with a size
> bigger than qemu_chr_be_can_write()). It would need to be fixed. But

It will :)

> which one does that?

Arash or Prasad can you help us here? Do you have a reproducer?

>>
>> Fixes: CVE-2018-18438
>> Reported-by: Arash Tohidi Chafi <tohidi.arash@gmail.com>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/usb/ccid-card-passthru.c | 14 +-------------
>>  1 file changed, 1 insertion(+), 13 deletions(-)
>>
>> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
>> index 8bb1314f49..1676b5fc05 100644
>> --- a/hw/usb/ccid-card-passthru.c
>> +++ b/hw/usb/ccid-card-passthru.c
>> @@ -264,24 +264,12 @@ static void ccid_card_vscard_handle_message(PassthruState *card,
>>      }
>>  }
>>
>> -static void ccid_card_vscard_drop_connection(PassthruState *card)
>> -{
>> -    qemu_chr_fe_deinit(&card->cs, true);
>> -    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;
>>
>> -    if (card->vscard_in_pos + size > VSCARD_IN_SIZE) {
>> -        error_report("no room for data: pos %u +  size %d > %" PRId64 "."
>> -                     " dropping connection.",
>> -                     card->vscard_in_pos, size, VSCARD_IN_SIZE);
>> -        ccid_card_vscard_drop_connection(card);
>> -        return;
>> -    }
>> +    assert(size <= VSCARD_IN_SIZE - card->vscard_in_pos);
>>      assert(card->vscard_in_hdr < VSCARD_IN_SIZE);
>>      memcpy(card->vscard_in_data + card->vscard_in_pos, buf, size);
>>      card->vscard_in_pos += size;
>> --
>> 2.20.1
>>

Re: [Qemu-devel] [PATCH v2 2/9] ccid-card-passthru: Replace never trigger if statement by an assertion

[P J P]

+-- On Mon, 18 Feb 2019, Philippe Mathieu-Daudé wrote --+
| On 2/15/19 11:59 AM, Marc-André Lureau wrote:
| Arash or Prasad can you help us here? Do you have a reproducer?

No, we don't have a reproducer handy I'm afraid.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v2 2/9] ccid-card-passthru: Replace never trigger if statement by an assertion

[Marc-André Lureau]

Hi

On Thu, Feb 21, 2019 at 12:06 PM P J P <ppandit@redhat.com> wrote:
>
> +-- On Mon, 18 Feb 2019, Philippe Mathieu-Daudé wrote --+
> | On 2/15/19 11:59 AM, Marc-André Lureau wrote:
> | Arash or Prasad can you help us here? Do you have a reproducer?
>
> No, we don't have a reproducer handy I'm afraid.

Without a reproducer or convincing arguments, do we have a CVE?


-- 
Marc-André Lureau