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é
> <address@hidden> 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 <address@hidden>
>> Suggested-by: Paolo Bonzini <address@hidden>
>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>> ---
>>  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
>>