Re: [Qemu-devel] [PATCH] ccid-card-passthru: check buffer size parameter

[Paolo Bonzini]

On 11/10/2018 13:58, Philippe Mathieu-Daudé wrote:
> Cc'ing Paolo & Marc-André.
> 
> On 11/10/2018 13:24, P J P wrote:
>> From: Prasad J Pandit <address@hidden>
>>
>> While reading virtual smart card data, if buffer 'size' is negative
>> it would lead to memory corruption errors. Add check to avoid it.
> 
> The IOReadHandler does not have documentation.
> 
>  typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size);
> 
> Why is the 'size' argument signed? Does it makes sens to call it with a
> negative value?

Yeah, it probably should be changed to size_t (same for the return value
of can_read callbacks); the size cannot be negative, in fact it can also
never be zero.

Also, the "if" should never trigger because the size is bound to what
can_read returns, which is VSCARD_IN_SIZE - card->vscard_in_pos.

So it is probably better to:

1) move the

    assert(card->vscard_in_pos < VSCARD_IN_SIZE);

to can_read, which becomes

    assert(card->vscard_in_pos <= VSCARD_IN_SIZE);
    return VSCARD_IN_SIZE - card->vscard_in_pos;

2) here, replace the if with an

    assert(size <= VSCARD_IN_SIZE - card->vscard_in_pos);

Note that the right side of the comparison is the return value of
can_read.  Also, this assert will always fail if card->vscard_in_pos >=
VSCARD_IN_SIZE), since size > 0.

3) also here, replace the

    assert(card->vscard_in_hdr < VSCARD_IN_SIZE);

with the stricter and more correct

    assert(card->vscard_in_hdr < card->vscard_in_pos);


Related, I don't understand why the read function ends with

    if (card->vscard_in_hdr == card->vscard_in_pos) {
        card->vscard_in_pos = card->vscard_in_hdr = 0;
    }

I would have expected something more generic, like:

    /* 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;
    }

Marc-André, do you know something about libcacard that justifies the
latter?  If the error_report is changed to an assert it's probably a
good idea anyway to make the code more liberal in what it accepts.
Prasad, can you prepare a v2 that does these changes?

Thanks,

Paolo

> 
> Thanks,
> 
> Phil.
> 
>>
>> Reported-by: Arash TC <address@hidden>
>> Signed-off-by: Prasad J Pandit <address@hidden>
>> ---
>>  hw/usb/ccid-card-passthru.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
>> index 0a6c657228..63ed78f4c6 100644
>> --- a/hw/usb/ccid-card-passthru.c
>> +++ b/hw/usb/ccid-card-passthru.c
>> @@ -275,6 +275,7 @@ static void ccid_card_vscard_read(void *opaque, const 
>> uint8_t *buf, int size)
>>      PassthruState *card = opaque;
>>      VSCMsgHeader *hdr;
>>  
>> +    assert(0 <= size && size < VSCARD_IN_SIZE);
>>      if (card->vscard_in_pos + size > VSCARD_IN_SIZE) {
>>          error_report("no room for data: pos %u +  size %d > %" PRId64 "."
>>                       " dropping connection.",
>>
>