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 <address@hidden> wrote:

> From: Stefan Fritsch <address@hidden>
>
> - 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 <address@hidden>
> Signed-off-by: Christian Ehrhardt <address@hidden>
> ---
>  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