| Message ID | 20181011112436.9305-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ccid-card-passthru: check buffer size parameter | expand |
Cc'ing Paolo & Marc-André. On 11/10/2018 13:24, P J P wrote: > From: Prasad J Pandit <pjp@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? Thanks, Phil. > > Reported-by: Arash TC <tohidi.arash@gmail.com> > Signed-off-by: Prasad J Pandit <pjp@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.", >
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 <pjp@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.arash@gmail.com> >> Signed-off-by: Prasad J Pandit <pjp@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.", >> >
+-- On Thu, 11 Oct 2018, Philippe Mathieu-Daudé wrote --+ | 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? No, it doesn't IMO. I had first changed argument type 'int' to uint32_t'. as typedef void IOReadHandler(void *opaque, const uint8_t *buf, uint32_t size); But 'IOReadHandler' is registered and called from multiple char devices, which lead to compile time errors. As the function prototype changed. I'll update them all, if the above change is okay. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
On 11/10/2018 14:29, P J P wrote: > +-- On Thu, 11 Oct 2018, Philippe Mathieu-Daudé wrote --+ > | 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? > > No, it doesn't IMO. I had first changed argument type 'int' to uint32_t'. > as > > typedef void IOReadHandler(void *opaque, const uint8_t *buf, uint32_t size); > > But 'IOReadHandler' is registered and called from multiple char devices, > which lead to compile time errors. As the function prototype changed. > > I'll update them all, if the above change is okay. Yes, please do so. Paolo > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F >
On 11/10/2018 14:29, P J P wrote: > +-- On Thu, 11 Oct 2018, Philippe Mathieu-Daudé wrote --+ > | 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? > > No, it doesn't IMO. I had first changed argument type 'int' to uint32_t'. > as > > typedef void IOReadHandler(void *opaque, const uint8_t *buf, uint32_t size); > > But 'IOReadHandler' is registered and called from multiple char devices, > which lead to compile time errors. As the function prototype changed. > > I'll update them all, if the above change is okay. I started this change and already converted 40 files.
+-- On Thu, 11 Oct 2018, Philippe Mathieu-Daudé wrote --+ | I started this change and already converted 40 files. Wow, that's super swift! :) Will wait for the patch V2 from you then. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
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.",