| Message ID | 20190214201939.494-5-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ccid-card-passthru: check buffer size parameter | expand |
Hi On Thu, Feb 14, 2019 at 9:20 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/usb/ccid-card-passthru.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c > index 0c44b38fc2..ba7c285ded 100644 > --- a/hw/usb/ccid-card-passthru.c > +++ b/hw/usb/ccid-card-passthru.c > @@ -285,8 +285,14 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size) > card->vscard_in_hdr += hdr->length + sizeof(VSCMsgHeader); > hdr = (VSCMsgHeader *)(card->vscard_in_data + card->vscard_in_hdr); > } > - if (card->vscard_in_hdr == card->vscard_in_pos) { > - card->vscard_in_pos = card->vscard_in_hdr = 0; Interesting, it looks like we could end in a blocking condition today. card->vscard_in_pos - card->vscard_in_hdr could not have enough room to process an incoming message. After filling the buffer, it would stop reading. > + > + /* 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; > } > } At least, by moving data around, this would leave always enough space for the header to be fully read. But I think we should add a condition like card->vscard_in_hdr + hdr->length + sizeof(VSCMsgHeader) <= VSCARD_IN_SIZE, to make sure the incoming message fits in the vscard_in_data buffer, else disconnect? > > -- > 2.20.1 >
diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c index 0c44b38fc2..ba7c285ded 100644 --- a/hw/usb/ccid-card-passthru.c +++ b/hw/usb/ccid-card-passthru.c @@ -285,8 +285,14 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size) card->vscard_in_hdr += hdr->length + sizeof(VSCMsgHeader); hdr = (VSCMsgHeader *)(card->vscard_in_data + card->vscard_in_hdr); } - if (card->vscard_in_hdr == card->vscard_in_pos) { - card->vscard_in_pos = card->vscard_in_hdr = 0; + + /* 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; } }
Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/usb/ccid-card-passthru.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)