On 2/15/19 11:59 AM, Marc-André Lureau wrote:
> Hi
>
> On Thu, Feb 14, 2019 at 9:20 PM Philippe Mathieu-Daudé
> <phi...@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.ar...@gmail.com>
>> Suggested-by: Paolo Bonzini <pbonz...@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <phi...@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
>>
