Re: [PATCH] crypto: propagate Error object on premature termination

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Oct 06, 2025 at 04:00:41PM -0400, Peter Xu wrote:
> On Mon, Oct 06, 2025 at 08:01:26PM +0100, Daniel P. Berrangé wrote:
> > The way that premature termination was handled in TLS connections was
> > changed to handle an ordering problem during graceful shutdown in the
> > migration code.
> > 
> > Unfortunately one of the codepaths returned -1 to indicate an error
> > condition, but failed to set the 'errp' parameter.
> > 
> > This broke error handling in the qio_channel_tls_handshake function,
> > as the QTask callback would no longer see that an error was raised.
> > As a result, the client will go on to try to use the already closed
> > TLS connection, resulting in misleading errors.
> > 
> > This was evidenced in the I/O test 233 which showed changes such as
> 
> Oops..
> 
> > 
> > -qemu-nbd: Certificate does not match the hostname localhost
> > +qemu-nbd: Failed to read initial magic: Unable to read from socket: Connection reset by peer
> > 
> > Fixes: 7e0c22d585581b8083ffdeb332ea497218665daf
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  crypto/tlssession.c |  8 +++++---
> >  io/channel-tls.c    | 13 +++++++------
> >  2 files changed, 12 insertions(+), 9 deletions(-)
> > 
> > diff --git a/crypto/tlssession.c b/crypto/tlssession.c
> > index ac38c2121d..8c0bf457ad 100644
> > --- a/crypto/tlssession.c
> > +++ b/crypto/tlssession.c
> > @@ -569,8 +569,6 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
> >      if (ret < 0) {
> >          if (ret == GNUTLS_E_AGAIN) {
> >              return QCRYPTO_TLS_SESSION_ERR_BLOCK;
> > -        } else if (ret == GNUTLS_E_PREMATURE_TERMINATION) {
> > -            return QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION;
> >          } else {
> >              if (session->rerr) {
> >                  error_propagate(errp, session->rerr);
> > @@ -580,7 +578,11 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
> >                             "Cannot read from TLS channel: %s",
> >                             gnutls_strerror(ret));
> >              }
> > -            return -1;
> > +            if (ret == GNUTLS_E_PREMATURE_TERMINATION) {
> > +                return QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION;
> > +            } else {
> > +                return -1;
> > +            }
> >          }
> >      }
> >  
> > diff --git a/io/channel-tls.c b/io/channel-tls.c
> > index 1fbed4be0c..70fad38d18 100644
> > --- a/io/channel-tls.c
> > +++ b/io/channel-tls.c
> > @@ -368,6 +368,7 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
> >                                       int flags,
> >                                       Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
> >      size_t i;
> >      ssize_t got = 0;
> > @@ -384,13 +385,13 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
> >              } else {
> >                  return QIO_CHANNEL_ERR_BLOCK;
> >              }
> > -        } else if (ret == QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION) {
> > -            if (qio_channel_tls_allow_premature_termination(tioc, flags)) {
> > -                ret = 0;
> > -            } else {
> 
> Would it be simpler to set error only here (and also we can have an
> explicit error message for the premature termination)?

I wanted to retain the original GNUTLS provided error message, which
is not available here, so must set it in the tlssession.c file.

> No strong opinions.. feel free to use whatever version:
> 
> Acked-by: Peter Xu <peterx@redhat.com>
> 
> Thanks!
> 
> > -                return -1;
> > -            }
> >          } else if (ret < 0) {
> > +            if (ret == QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION &&
> > +                qio_channel_tls_allow_premature_termination(tioc, flags)) {
> > +                error_free(*errp);
> > +                *errp = NULL;
> > +                return got;
> > +            }
> >              return -1;
> >          }
> >          got += ret;
> > -- 
> > 2.50.1
> > 
> 
> -- 
> Peter Xu
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|