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

[Christophe Fergeau]

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.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180809/0931d406/attachment.sig>