[Spice-devel] [PATCH libcacard 2/2] vreader: Handle read failure

[Jason Andryuk]

On Thu, Aug 9, 2018 at 5:35 AM Christophe Fergeau <cfergeau at redhat.com> wrote:
>
> On Thu, Aug 09, 2018 at 09:26:35AM +0200, Jakub Jelen wrote:
> > On Wed, 2018-08-08 at 14:08 -0400, Jason Andryuk wrote:
> > > On Wed, Aug 8, 2018 at 11:33 AM Jakub Jelen <jjelen at redhat.com>
> > > wrote:
> > > >
> > > > On Wed, 2018-08-08 at 16:51 +0200, Marc-André Lureau wrote:
> > > > > Hi
> > > > >
> > > > > On Tue, Jul 24, 2018 at 8:34 PM, Jason Andryuk <
> > > > > jandryuk at gmail.com>
> > > > > wrote:
> > > > > > If a command fails, card_status will not match
> > > > > > VCARD_DONE.  That
> > > > > > will
> > > > > > trigger the assert and abort the process.  Instead, handle
> > > > > > VCARD_FAIL and
> > > > > > return an error in that case.  Client software can then deal
> > > > > > with
> > > > > > the
> > > > > > error, and we continue running to handle future commands.
> > > > > >
> > > > > > This can be triggered by removing the physical smartcard mid-
> > > > > > operation.
> > > > >
> > > > > There are other paths, like invalid instruction on
> > > > > cac_common_process_apdu_read()
> > > >
> > > > The invalid instructions should return valid response with error
> > > > indicated in SW (status words). The referenced function has the
> > > > default
> > > > VCARD_FAIL value is in the code somehow bogus in case we would like
> > > > to
> > > > fail early or fail to handle some case (?).
> > > >
> > > > The VCARD_FAIL option is really about more serious issues as Jason
> > > > is
> > > > pointing out.
> > > >
> > > > Handling the error here, rather than segfaulting in assert later
> > > > sounds
> > > > like a good idea. But from reading the code, I still can not find a
> > > > path where we could encounter this value here.
> > > >
> > > > From what I see, all the paths here return either VCARD_DONE. Can
> > > > you
> > > > advice during which operation did you encounter this error?
> > >
> > > My setup is qemu <-> vscclient <-> pcscd with passthru:
> > > vscclient -e 'use_hw=yes hw_type=passthru'
> > >
> > > In a Windows VM, I ran `certutil -scinfo` from a cmd window.  While
> > > it
> > > was running, I pulled out my smart card.  Without my patch, vscclient
> > > terminates.  With it, vscclient continues running.
> > >
> > > The call stack is:
> > > vreader_xfr_bytes
> > >   vcard_process_apdu
> > >     vcard_process_applet_apdu
> > >       apdu_cb
> > >
> > > apdu_cb can return VCARD_FAIL for send_receive or
> > > vcard_response_new_data failure.
> > >
> > > So you think src/capcsc.c:apdu_cb() should return VCARD_DONE but use
> > > vcard_response_set_status_bytes to write an appropriate status word?
> >
> > I was referring mostly to the card emulation code which I know better
> > and which is safe in this way it returns the correct status words.
> >
> > I don't know much about the passthrough mode. But you are right -- it
> > can return VCARD_FAIL in case of the transmit to the real card fails
> > and I believe failing this way is a correct thing to do.
> >
> > Acked-by: Jakub Jelen <jjelen at redhat.com>
>
> Thanks for the patch and review, I've added the reproducer description
> to the commit log and pushed this.

Thank you.

-Jason