[Qemu-devel] [PATCH] io: return 0 for EOF in TLS session read after shutdown

[Qemu-devel] [PATCH] io: return 0 for EOF in TLS session read after shutdown

[Daniel P. Berrangé]

GNUTLS takes a paranoid approach when seeing 0 bytes returned by the
underlying OS read() function. It will consider this an error and
return GNUTLS_E_PREMATURE_TERMINATION instead of propagating the 0
return value. It expects apps to arrange for clean termination at
the protocol level and not rely on seeing EOF from a read call to
detect shutdown. This is to harden apps against a malicious 3rd party
causing termination of the sockets layer.

This is unhelpful for the QEMU NBD code which does have a clean
protocol level shutdown, but still relies on seeing 0 from the I/O
channel read in the coroutine handling incoming replies.

The upshot is that when using a plain NBD connection shutdown is
silent, but when using TLS, the client spams the console with

  Cannot read from TLS channel: Broken pipe

The NBD connection has, however, called qio_channel_shutdown()
at this point to indicate that it is done with I/O. This gives
the opportunity to optimize the code such that when the channel
has been shutdown in the read direction, the error code
GNUTLS_E_PREMATURE_TERMINATION gets turned into a '0' return
instead of an error.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/tlssession.c      | 3 +++
 include/io/channel-tls.h | 1 +
 include/io/channel.h     | 6 +++---
 io/channel-tls.c         | 5 +++++
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 2f28fa7f71..0dedd4af52 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -473,6 +473,9 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
         case GNUTLS_E_INTERRUPTED:
             errno = EINTR;
             break;
+        case GNUTLS_E_PREMATURE_TERMINATION:
+            errno = ECONNABORTED;
+            break;
         default:
             errno = EIO;
             break;
diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h
index 87fcaf9146..fdbdf12feb 100644
--- a/include/io/channel-tls.h
+++ b/include/io/channel-tls.h
@@ -48,6 +48,7 @@ struct QIOChannelTLS {
     QIOChannel parent;
     QIOChannel *master;
     QCryptoTLSSession *session;
+    QIOChannelShutdown shutdown;
 };
 
 /**
diff --git a/include/io/channel.h b/include/io/channel.h
index e8cdadb0b0..da2f138200 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -51,9 +51,9 @@ enum QIOChannelFeature {
 typedef enum QIOChannelShutdown QIOChannelShutdown;
 
 enum QIOChannelShutdown {
-    QIO_CHANNEL_SHUTDOWN_BOTH,
-    QIO_CHANNEL_SHUTDOWN_READ,
-    QIO_CHANNEL_SHUTDOWN_WRITE,
+    QIO_CHANNEL_SHUTDOWN_READ = 1,
+    QIO_CHANNEL_SHUTDOWN_WRITE = 2,
+    QIO_CHANNEL_SHUTDOWN_BOTH = 3,
 };
 
 typedef gboolean (*QIOChannelFunc)(QIOChannel *ioc,
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 9628e6fa47..c98ead21b0 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -275,6 +275,9 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
                 } else {
                     return QIO_CHANNEL_ERR_BLOCK;
                 }
+            } else if (errno == ECONNABORTED &&
+                       (tioc->shutdown & QIO_CHANNEL_SHUTDOWN_READ)) {
+                return 0;
             }
 
             error_setg_errno(errp, errno,
@@ -357,6 +360,8 @@ static int qio_channel_tls_shutdown(QIOChannel *ioc,
 {
     QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
 
+    tioc->shutdown |= how;
+
     return qio_channel_shutdown(tioc->master, how, errp);
 }
 
-- 
2.19.1

Re: [Qemu-devel] [PATCH] io: return 0 for EOF in TLS session read after shutdown

[Eric Blake]

On 11/19/18 7:42 AM, Daniel P. Berrangé wrote:
> GNUTLS takes a paranoid approach when seeing 0 bytes returned by the
> underlying OS read() function. It will consider this an error and
> return GNUTLS_E_PREMATURE_TERMINATION instead of propagating the 0
> return value. It expects apps to arrange for clean termination at
> the protocol level and not rely on seeing EOF from a read call to
> detect shutdown. This is to harden apps against a malicious 3rd party
> causing termination of the sockets layer.
> 
> This is unhelpful for the QEMU NBD code which does have a clean
> protocol level shutdown, but still relies on seeing 0 from the I/O
> channel read in the coroutine handling incoming replies.
> 
> The upshot is that when using a plain NBD connection shutdown is
> silent, but when using TLS, the client spams the console with
> 
>    Cannot read from TLS channel: Broken pipe
> 
> The NBD connection has, however, called qio_channel_shutdown()
> at this point to indicate that it is done with I/O. This gives
> the opportunity to optimize the code such that when the channel
> has been shutdown in the read direction, the error code
> GNUTLS_E_PREMATURE_TERMINATION gets turned into a '0' return
> instead of an error.

Detecting premature termination when the client has NOT requested 
orderly shutdown is still important, and this patch preserves that 
aspect.  You are only changing the case where the client has informed 
the qio code "yes, an early termination is now okay".

> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---

> +++ b/include/io/channel.h
> @@ -51,9 +51,9 @@ enum QIOChannelFeature {
>   typedef enum QIOChannelShutdown QIOChannelShutdown;
>   
>   enum QIOChannelShutdown {
> -    QIO_CHANNEL_SHUTDOWN_BOTH,
> -    QIO_CHANNEL_SHUTDOWN_READ,
> -    QIO_CHANNEL_SHUTDOWN_WRITE,
> +    QIO_CHANNEL_SHUTDOWN_READ = 1,
> +    QIO_CHANNEL_SHUTDOWN_WRITE = 2,
> +    QIO_CHANNEL_SHUTDOWN_BOTH = 3,

Nice use of bit operations :)

> +++ b/io/channel-tls.c
> @@ -275,6 +275,9 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
>                   } else {
>                       return QIO_CHANNEL_ERR_BLOCK;
>                   }
> +            } else if (errno == ECONNABORTED &&
> +                       (tioc->shutdown & QIO_CHANNEL_SHUTDOWN_READ)) {
> +                return 0;
>               }

Reviewed-by: Eric Blake <eblake@redhat.com>

I like this patch better than my proposed hack to ignore read errors 
after requesting quit; testing it now.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org