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 <p...@fedoraproject.org> >> >> 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 <tohidi.ar...@gmail.com> >> Signed-off-by: Prasad J Pandit <p...@fedoraproject.org> >> --- >> 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.", >> >