public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] staging: usbip: Extend crypto support
@ 2013-09-19 14:11 Dominik Paulus
  2013-09-19 14:11 ` [PATCH 1/7] staging: usbip: TLS for all userspace communication Dominik Paulus
                   ` (6 more replies)
  0 siblings, 7 replies; 13+ messages in thread
From: Dominik Paulus @ 2013-09-19 14:11 UTC (permalink / raw)
  To: usbip-devel
  Cc: Greg Kroah-Hartman, Masanari Iida, Dominik Paulus, Tobias Polzer,
	Kurt Kanzenbach, Stefan Reif, Joe Perches, Bart Westgeest,
	Jake Champlin, Ilija Hadzic, Anthony Foiani, Bernard Blackham,
	Harvey Yang, linux-usb, devel, linux-kernel, linux-kernel

Hi,

this patch series extends our previous set of patches (see [1]). We extended
the crypto support so all of the usbip network traffic can now be completely
encrypted and authenticated.

We now use GnuTLS not only for password verification, but extend the lifetime
of the TLS connection to cover all of the userland communications.  Before
handing over the connection to the kernel, two randomly generated 128 bit
session keys are exchanged between client and server and stored in sysfs
together with the sockfd. The kernel uses these keys to encrypt and
authenticate all of the traffic using AES-GCM and the linux crypto API.
Separate keys are used for both directions of the data channel.

To the best of our knowledge, the implemented encryption should provide decent
security. However, it still lacks complete review; we also note that in the
documentation.

As mentioned in the project README, the network protocol needs more discussion.
This series increments the protocol version, because the improved crypto
support breaks compatibility with the previous patch series[1]. In the long
term, the protocol should be extended to support proper feature negotiation. If
both patch series are merged as one, the protocol version increment can be
omitted - both patch series are compatible with unauthenticated transport, but
are incompatible with each other.

Regards,
	Tobias Polzer and Dominik Paulus

[1] <1379066161-8278-1-git-send-email-dominik.paulus@fau.de>,
    https://lkml.org/lkml/2013/9/13/104


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/7] staging: usbip: TLS for all userspace communication
  2013-09-19 14:11 [PATCH 0/7] staging: usbip: Extend crypto support Dominik Paulus
@ 2013-09-19 14:11 ` Dominik Paulus
  2013-09-19 14:11 ` [PATCH 2/7] staging: usbip: Exchange session keys in userspace Dominik Paulus
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Dominik Paulus @ 2013-09-19 14:11 UTC (permalink / raw)
  To: usbip-devel
  Cc: Dominik Paulus, Tobias Polzer, Greg Kroah-Hartman, Masanari Iida,
	Kurt Kanzenbach, Stefan Reif, Joe Perches, Bart Westgeest,
	Jake Champlin, Ilija Hadzic, Anthony Foiani, Bernard Blackham,
	Harvey Yang, linux-usb, devel, linux-kernel, linux-kernel

This patch extends the TLS support to cover all communication in
userspace. The TLS connection is released shortly before the socket is
passed to the kernel.

This requires for additional connection state to be passed between
functions. We thus replaced the sockfd by a struct containing the TLS
context as well as the fd.

Signed-off-by: Dominik Paulus <dominik.paulus@fau.de>
Signed-off-by: Tobias Polzer <tobias.polzer@fau.de>
---
 drivers/staging/usbip/userspace/src/usbip_attach.c |  24 +-
 drivers/staging/usbip/userspace/src/usbip_list.c   |  24 +-
 .../staging/usbip/userspace/src/usbip_network.c    | 279 +++++++++++++++++----
 .../staging/usbip/userspace/src/usbip_network.h    |  49 +++-
 drivers/staging/usbip/userspace/src/usbipd.c       | 244 ++++--------------
 5 files changed, 358 insertions(+), 262 deletions(-)

diff --git a/drivers/staging/usbip/userspace/src/usbip_attach.c b/drivers/staging/usbip/userspace/src/usbip_attach.c
index 651e93a..9f4a064 100644
--- a/drivers/staging/usbip/userspace/src/usbip_attach.c
+++ b/drivers/staging/usbip/userspace/src/usbip_attach.c
@@ -86,7 +86,8 @@ static int record_connection(char *host, char *port, char *busid, int rhport)
 	return 0;
 }
 
-static int import_device(int sockfd, struct usbip_usb_device *udev)
+static int import_device(struct usbip_connection *conn, struct usbip_usb_device
+		*udev)
 {
 	int rc;
 	int port;
@@ -104,8 +105,10 @@ static int import_device(int sockfd, struct usbip_usb_device *udev)
 		return -1;
 	}
 
-	rc = usbip_vhci_attach_device(port, sockfd, udev->busnum,
+	usbip_net_bye(conn);
+	rc = usbip_vhci_attach_device(port, conn->sockfd, udev->busnum,
 				      udev->devnum, udev->speed);
+
 	if (rc < 0) {
 		err("import device");
 		usbip_vhci_driver_close();
@@ -117,7 +120,7 @@ static int import_device(int sockfd, struct usbip_usb_device *udev)
 	return port;
 }
 
-static int query_import_device(int sockfd, char *busid)
+static int query_import_device(struct usbip_connection *conn, char *busid)
 {
 	int rc;
 	struct op_import_request request;
@@ -128,7 +131,7 @@ static int query_import_device(int sockfd, char *busid)
 	memset(&reply, 0, sizeof(reply));
 
 	/* send a request */
-	rc = usbip_net_send_op_common(sockfd, OP_REQ_IMPORT, 0);
+	rc = usbip_net_send_op_common(conn, OP_REQ_IMPORT, 0);
 	if (rc < 0) {
 		err("send op_common");
 		return -1;
@@ -138,20 +141,20 @@ static int query_import_device(int sockfd, char *busid)
 
 	PACK_OP_IMPORT_REQUEST(0, &request);
 
-	rc = usbip_net_send(sockfd, (void *) &request, sizeof(request));
+	rc = usbip_net_send(conn, (void *) &request, sizeof(request));
 	if (rc < 0) {
 		err("send op_import_request");
 		return -1;
 	}
 
 	/* receive a reply */
-	rc = usbip_net_recv_op_common(sockfd, &code);
+	rc = usbip_net_recv_op_common(conn, &code);
 	if (rc < 0) {
 		err("recv op_common: %s", usbip_net_strerror(rc));
 		return -1;
 	}
 
-	rc = usbip_net_recv(sockfd, (void *) &reply, sizeof(reply));
+	rc = usbip_net_recv(conn, (void *) &reply, sizeof(reply));
 	if (rc < 0) {
 		err("recv op_import_reply");
 		return -1;
@@ -166,7 +169,7 @@ static int query_import_device(int sockfd, char *busid)
 	}
 
 	/* import a device */
-	return import_device(sockfd, &reply.udev);
+	return import_device(conn, &reply.udev);
 }
 
 static int attach_device(char *host, char *busid)
@@ -174,14 +177,15 @@ static int attach_device(char *host, char *busid)
 	int sockfd;
 	int rc;
 	int rhport;
+	struct usbip_connection conn;
 
-	sockfd = usbip_net_connect(host);
+	sockfd = usbip_net_connect(host, &conn);
 	if (sockfd < 0) {
 		err("connection attempt failed");
 		return -1;
 	}
 
-	rhport = query_import_device(sockfd, busid);
+	rhport = query_import_device(&conn, busid);
 	if (rhport < 0) {
 		err("query");
 		return -1;
diff --git a/drivers/staging/usbip/userspace/src/usbip_list.c b/drivers/staging/usbip/userspace/src/usbip_list.c
index ff7acf8..187eb7d 100644
--- a/drivers/staging/usbip/userspace/src/usbip_list.c
+++ b/drivers/staging/usbip/userspace/src/usbip_list.c
@@ -45,7 +45,7 @@ void usbip_list_usage(void)
 	printf("usage: %s", usbip_list_usage_string);
 }
 
-static int get_exported_devices(char *host, int sockfd)
+static int get_exported_devices(char *host, struct usbip_connection *conn)
 {
 	char product_name[100];
 	char class_name[100];
@@ -56,13 +56,13 @@ static int get_exported_devices(char *host, int sockfd)
 	unsigned int i;
 	int j, rc;
 
-	rc = usbip_net_send_op_common(sockfd, OP_REQ_DEVLIST, 0);
+	rc = usbip_net_send_op_common(conn, OP_REQ_DEVLIST, 0);
 	if (rc < 0) {
 		dbg("usbip_net_send_op_common failed");
 		return -1;
 	}
 
-	rc = usbip_net_recv_op_common(sockfd, &code);
+	rc = usbip_net_recv_op_common(conn, &code);
 	if (rc < 0) {
 		err("usbip_net_recv_op_common failed: %s",
 			usbip_net_strerror(rc));
@@ -70,7 +70,7 @@ static int get_exported_devices(char *host, int sockfd)
 	}
 
 	memset(&reply, 0, sizeof(reply));
-	rc = usbip_net_recv(sockfd, &reply, sizeof(reply));
+	rc = usbip_net_recv(conn, &reply, sizeof(reply));
 	if (rc < 0) {
 		dbg("usbip_net_recv_op_devlist failed");
 		return -1;
@@ -89,7 +89,7 @@ static int get_exported_devices(char *host, int sockfd)
 
 	for (i = 0; i < reply.ndev; i++) {
 		memset(&udev, 0, sizeof(udev));
-		rc = usbip_net_recv(sockfd, &udev, sizeof(udev));
+		rc = usbip_net_recv(conn, &udev, sizeof(udev));
 		if (rc < 0) {
 			dbg("usbip_net_recv failed: usbip_usb_device[%d]", i);
 			return -1;
@@ -106,7 +106,7 @@ static int get_exported_devices(char *host, int sockfd)
 		printf("%11s: %s\n", "", class_name);
 
 		for (j = 0; j < udev.bNumInterfaces; j++) {
-			rc = usbip_net_recv(sockfd, &uintf, sizeof(uintf));
+			rc = usbip_net_recv(conn, &uintf, sizeof(uintf));
 			if (rc < 0) {
 				dbg("usbip_net_recv failed: usbip_usb_intf[%d]",
 				    j);
@@ -130,23 +130,23 @@ static int get_exported_devices(char *host, int sockfd)
 static int list_exported_devices(char *host)
 {
 	int rc;
-	int sockfd;
+	struct usbip_connection conn;
 
-	sockfd = usbip_net_connect(host);
-	if (sockfd < 0) {
+	rc = usbip_net_connect(host, &conn);
+	if (rc < 0) {
 		err("could not connect to %s:%s: %s", host,
-		    usbip_port_string, gai_strerror(sockfd));
+		    usbip_port_string, usbip_net_strerror(rc));
 		return -1;
 	}
 	dbg("connected to %s:%s", host, usbip_port_string);
 
-	rc = get_exported_devices(host, sockfd);
+	rc = get_exported_devices(host, &conn);
 	if (rc < 0) {
 		err("failed to get device list from %s", host);
 		return -1;
 	}
 
-	close(sockfd);
+	usbip_net_bye(&conn);
 
 	return 0;
 }
diff --git a/drivers/staging/usbip/userspace/src/usbip_network.c b/drivers/staging/usbip/userspace/src/usbip_network.c
index 799a68f..0b87552 100644
--- a/drivers/staging/usbip/userspace/src/usbip_network.c
+++ b/drivers/staging/usbip/userspace/src/usbip_network.c
@@ -24,11 +24,13 @@
 #include <netdb.h>
 #include <netinet/tcp.h>
 #include <unistd.h>
+#include <assert.h>
 
 #include "../config.h"
 
 #ifdef HAVE_GNUTLS
 #include <gnutls/gnutls.h>
+#include <gnutls/crypto.h>
 #endif
 
 #ifdef HAVE_LIBWRAP
@@ -113,8 +115,8 @@ void usbip_net_pack_usb_interface(int pack __attribute__((unused)),
 	/* uint8_t members need nothing */
 }
 
-static ssize_t usbip_net_xmit(int sockfd, void *buff, size_t bufflen,
-			      int sending)
+static ssize_t usbip_net_xmit(struct usbip_connection *conn, void *buff, size_t
+	bufflen, int sending)
 {
 	ssize_t nbytes;
 	ssize_t total = 0;
@@ -123,10 +125,22 @@ static ssize_t usbip_net_xmit(int sockfd, void *buff, size_t bufflen,
 		return 0;
 
 	do {
-		if (sending)
-			nbytes = send(sockfd, buff, bufflen, 0);
+		if (!conn->have_crypto && sending)
+			nbytes = send(conn->sockfd, buff, bufflen, 0);
+		else if (!conn->have_crypto && !sending)
+			nbytes = recv(conn->sockfd, buff, bufflen, MSG_WAITALL);
+#ifdef HAVE_GNUTLS
+		else if (sending)
+			nbytes = gnutls_record_send(conn->session, buff, bufflen);
 		else
-			nbytes = recv(sockfd, buff, bufflen, MSG_WAITALL);
+			nbytes = gnutls_record_recv(conn->session, buff, bufflen);
+#else
+		/*
+		 * Assertion to let gcc be able to infer proper initialization
+		 * of nbytes.
+		 */
+		assert(!conn->have_crypto);
+#endif
 
 		if (nbytes <= 0)
 			return -1;
@@ -140,17 +154,20 @@ static ssize_t usbip_net_xmit(int sockfd, void *buff, size_t bufflen,
 	return total;
 }
 
-ssize_t usbip_net_recv(int sockfd, void *buff, size_t bufflen)
+ssize_t usbip_net_recv(struct usbip_connection *conn, void *buff, size_t
+		bufflen)
 {
-	return usbip_net_xmit(sockfd, buff, bufflen, 0);
+	return usbip_net_xmit(conn, buff, bufflen, 0);
 }
 
-ssize_t usbip_net_send(int sockfd, void *buff, size_t bufflen)
+ssize_t usbip_net_send(struct usbip_connection *conn, void *buff, size_t
+		bufflen)
 {
-	return usbip_net_xmit(sockfd, buff, bufflen, 1);
+	return usbip_net_xmit(conn, buff, bufflen, 1);
 }
 
-int usbip_net_send_op_common(int sockfd, uint32_t code, uint32_t status)
+int usbip_net_send_op_common(struct usbip_connection *conn, uint32_t code,
+		uint32_t status)
 {
 	struct op_common op_common;
 	int rc;
@@ -163,7 +180,7 @@ int usbip_net_send_op_common(int sockfd, uint32_t code, uint32_t status)
 
 	PACK_OP_COMMON(1, &op_common);
 
-	rc = usbip_net_send(sockfd, &op_common, sizeof(op_common));
+	rc = usbip_net_send(conn, &op_common, sizeof(op_common));
 	if (rc < 0) {
 		dbg("usbip_net_send failed: %d", rc);
 		return -1;
@@ -172,14 +189,15 @@ int usbip_net_send_op_common(int sockfd, uint32_t code, uint32_t status)
 	return 0;
 }
 
-int usbip_net_recv_op_common(int sockfd, uint16_t *code)
+
+int usbip_net_recv_op_common(struct usbip_connection *conn, uint16_t *code)
 {
 	struct op_common op_common;
 	int rc;
 
 	memset(&op_common, 0, sizeof(op_common));
 
-	rc = usbip_net_recv(sockfd, &op_common, sizeof(op_common));
+	rc = usbip_net_recv(conn, &op_common, sizeof(op_common));
 	if (rc < 0) {
 		dbg("usbip_net_recv failed: %d", rc);
 		return -ERR_SYSERR;
@@ -225,7 +243,8 @@ const char *usbip_net_strerror(int status)
 		/* ERR_AUTHREQ */ "Server requires authentication",
 		/* ERR_PERM */ "Permission denied",
 		/* ERR_NOTFOUND */ "Requested device not found",
-		/* ERR_NOAUTH */ "Server doesn't support authentication"
+		/* ERR_NOAUTH */ "Server doesn't support authentication",
+		/* ERR_INUSE */ "Requested device is already in use"
 	};
 	if (status < 0)
 		status = -status;
@@ -251,7 +270,8 @@ int usbip_net_set_nodelay(int sockfd)
 	const int val = 1;
 	int ret;
 
-	ret = setsockopt(sockfd, IPPROTO_TCP, TCP_NODELAY, &val, sizeof(val));
+	ret = setsockopt(sockfd, IPPROTO_TCP, TCP_NODELAY, &val,
+		sizeof(val));
 	if (ret < 0)
 		dbg("setsockopt: TCP_NODELAY");
 
@@ -330,82 +350,249 @@ int usbip_net_tcp_connect(char *hostname, char *service)
 }
 
 #ifdef HAVE_GNUTLS
-int usbip_net_srp_handshake(int sockfd)
+static gnutls_datum_t usbip_net_srp_salt, usbip_net_srp_verifier;
+static gnutls_srp_server_credentials_t usbip_net_srp_cred;
+
+#define SRP_GROUP gnutls_srp_2048_group_generator
+#define SRP_PRIME gnutls_srp_2048_group_prime
+
+int usbip_net_srp_client_handshake(struct usbip_connection *conn)
 {
 	int ret;
-	gnutls_session_t session;
-	gnutls_srp_client_credentials_t srp_cred;
 
-	ret = gnutls_srp_allocate_client_credentials(&srp_cred);
+	ret = gnutls_srp_allocate_client_credentials(&conn->srp_client_cred);
 	if (ret < 0)
 		return ret;
 
-	gnutls_srp_set_client_credentials(srp_cred, "dummyuser",
+	gnutls_srp_set_client_credentials(conn->srp_client_cred, "dummyuser",
 		usbip_srp_password);
 
-	ret = gnutls_init(&session, GNUTLS_CLIENT);
+	ret = gnutls_init(&conn->session, GNUTLS_CLIENT);
 	if (ret < 0) {
-		gnutls_srp_free_client_credentials(srp_cred);
+		gnutls_srp_free_client_credentials(conn->srp_client_cred);
 		return ret;
 	}
 
-	gnutls_priority_set_direct(session, "NORMAL:+SRP", NULL);
+	gnutls_priority_set_direct(conn->session, "NORMAL:+SRP", NULL);
 
-	gnutls_credentials_set(session, GNUTLS_CRD_SRP, srp_cred);
-	gnutls_transport_set_int (session, sockfd);
+	gnutls_credentials_set(conn->session, GNUTLS_CRD_SRP,
+			conn->srp_client_cred);
+	gnutls_transport_set_int (conn->session, conn->sockfd);
 
 	do {
-		ret = gnutls_handshake(session);
+		ret = gnutls_handshake(conn->session);
 	} while (ret < 0 && !gnutls_error_is_fatal(ret));
 
-	gnutls_bye(session, GNUTLS_SHUT_RDWR);
+	return ret;
+}
 
-	gnutls_deinit(session);
-	gnutls_srp_free_client_credentials(srp_cred);
+int usbip_net_srp_server_handshake(struct usbip_connection *conn)
+{
+	int ret;
+
+	if (gnutls_init(&conn->session, GNUTLS_SERVER) != 0)
+		return -1;
+	gnutls_priority_set_direct(conn->session, "NORMAL:-KX-ALL:+SRP", NULL);
+	if (gnutls_credentials_set(conn->session, GNUTLS_CRD_SRP,
+		usbip_net_srp_cred) != 0)
+		return -1;
+
+	gnutls_transport_set_int(conn->session, conn->sockfd);
+
+	do {
+		ret = gnutls_handshake(conn->session);
+	} while (ret < 0 && gnutls_error_is_fatal(ret) == 0);
+
+	if (ret < 0)
+		err("GnuTLS handshake failed (%s)", gnutls_strerror(ret));
+	else
+		info("GnuTLS handshake completed");
+
+	conn->have_crypto = 1;
 
 	return ret;
 }
+
+static int net_srp_callback(gnutls_session_t sess, const char *username,
+	gnutls_datum_t *nsalt, gnutls_datum_t *nverifier, gnutls_datum_t *g,
+	gnutls_datum_t *n)
+{
+	/*
+	 * GnuTLS expects us to allocate all data returned from callbacks
+	 * using gnutls_malloc(), thus, we have to create a fresh copy of
+	 * our static credentials for every connection.
+	 */
+	nsalt->data = gnutls_malloc(usbip_net_srp_salt.size);
+	nverifier->data = gnutls_malloc(usbip_net_srp_verifier.size);
+	if (nsalt->data == NULL || nverifier->data == NULL) {
+		gnutls_free(nsalt->data);
+		gnutls_free(nverifier->data);
+		return -1;
+	}
+	nsalt->size = usbip_net_srp_salt.size;
+	nverifier->size = usbip_net_srp_verifier.size;
+	memcpy(nverifier->data, usbip_net_srp_verifier.data,
+			usbip_net_srp_verifier.size);
+	memcpy(nsalt->data, usbip_net_srp_salt.data, usbip_net_srp_salt.size);
+
+	*g = SRP_GROUP;
+	*n = SRP_PRIME;
+
+	/* We only have a single session, thus, ignore it */
+	(void) sess;
+
+	if (strcmp(username, "dummyuser"))
+		/* User invalid, stored dummy data in g and n. */
+		return 1;
+
+	return 0;
+}
+
+int usbip_net_init_gnutls(void)
+{
+	int ret;
+
+	gnutls_global_init();
+
+	usbip_net_srp_salt.data = gnutls_malloc(16);
+	if (!usbip_net_srp_salt.data)
+		return GNUTLS_E_MEMORY_ERROR;
+
+	ret = gnutls_rnd(GNUTLS_RND_NONCE, usbip_net_srp_salt.data, 16);
+	if (ret < 0)
+		return ret;
+	usbip_net_srp_salt.size = 16;
+
+	ret = gnutls_srp_allocate_server_credentials(&usbip_net_srp_cred);
+	if (ret < 0)
+		return ret;
+
+	ret = gnutls_srp_verifier("dummyuser", optarg, &usbip_net_srp_salt,
+		&SRP_GROUP, &SRP_PRIME, &usbip_net_srp_verifier);
+	if (ret < 0)
+		return ret;
+
+	gnutls_srp_set_server_credentials_function(usbip_net_srp_cred,
+		net_srp_callback);
+
+	return GNUTLS_E_SUCCESS;
+}
 #endif
 
-/*
- * Connect to the server. Performs the TCP connection attempt
- * and - if necessary - the TLS handshake used for authentication.
- */
-int usbip_net_connect(char *hostname)
+#ifdef HAVE_LIBWRAP
+static int tcpd_auth(int connfd)
 {
-	int sockfd;
+	struct request_info request;
+	int rc;
+
+	request_init(&request, RQ_DAEMON, PROGNAME, RQ_FILE, connfd, 0);
+	fromhost(&request);
+	rc = hosts_access(&request);
+	if (rc == 0)
+		return -1;
+
+	return 0;
+}
+#endif
+
+int usbip_net_accept(int listenfd, struct usbip_connection *conn)
+{
+	int connfd;
+	struct sockaddr_storage ss;
+	socklen_t len = sizeof(ss);
+	char host[NI_MAXHOST], port[NI_MAXSERV];
+	int rc;
+
+	memset(&ss, 0, sizeof(ss));
+
+	connfd = accept(listenfd, (struct sockaddr *)&ss, &len);
+	if (connfd < 0) {
+		err("failed to accept connection");
+		return -1;
+	}
+
+	rc = getnameinfo((struct sockaddr *)&ss, len, host, sizeof(host),
+			 port, sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV);
+	if (rc)
+		err("getnameinfo: %s", gai_strerror(rc));
+
+#ifdef HAVE_LIBWRAP
+	rc = tcpd_auth(connfd);
+	if (rc < 0) {
+		info("denied access from %s", host);
+		close(connfd);
+		return -1;
+	}
+#endif
+	info("connection from %s, port %s", host, port);
+
+	conn->sockfd = connfd;
+	conn->have_crypto = 0;
+	conn->server = 1;
+
+	return 0;
+}
+
+int usbip_net_connect(char *hostname, struct usbip_connection *conn)
+{
+	conn->sockfd = usbip_net_tcp_connect(hostname, usbip_port_string);
+	if (conn->sockfd < 0) {
+		err("TCP connection attempt failed: %s",
+				gai_strerror(conn->sockfd));
+		return ERR_SYSERR;
+	}
 
-	sockfd = usbip_net_tcp_connect(hostname, usbip_port_string);
-	if (sockfd < 0)
-		return sockfd;
+	conn->have_crypto = 0;
+	conn->server = 0;
 
 #ifdef HAVE_GNUTLS
 	if (usbip_srp_password) {
 		int rc;
 		uint16_t code = OP_REP_STARTTLS;
 
-		rc = usbip_net_send_op_common(sockfd, OP_REQ_STARTTLS, 0);
+		rc = usbip_net_send_op_common(conn, OP_REQ_STARTTLS, 0);
 		if (rc < 0) {
+			close(conn->sockfd);
 			err("usbip_net_send_op_common failed");
-			return EAI_SYSTEM;
+			return ERR_SYSERR;
 		}
 
-		rc = usbip_net_recv_op_common(sockfd, &code);
+		rc = usbip_net_recv_op_common(conn, &code);
 		if (rc < 0) {
 			err("STARTTLS attempt failed: %s",
 				usbip_net_strerror(rc));
-			return -1;
+			close(conn->sockfd);
+			return ERR_SYSERR;
 		}
 
-		rc = usbip_net_srp_handshake(sockfd);
+		rc = usbip_net_srp_client_handshake(conn);
 		if (rc < 0) {
 			err("Unable to perform TLS handshake (wrong password?): %s",
 				gnutls_strerror(rc));
-			close(sockfd);
-			return EAI_SYSTEM;
+			close(conn->sockfd);
+			return ERR_SYSERR;
 		}
+
+		conn->have_crypto = 1;
 	}
 #endif
 
-	return sockfd;
+	return 0;
+}
+
+void usbip_net_bye(struct usbip_connection *conn)
+{
+#ifdef HAVE_GNUTLS
+	if (conn->have_crypto) {
+		gnutls_bye(conn->session, GNUTLS_SHUT_RDWR);
+
+		gnutls_deinit(conn->session);
+		if (!conn->server)
+			gnutls_srp_free_client_credentials(conn->srp_client_cred);
+
+		conn->have_crypto = 0;
+	}
+#else
+	(void)conn;
+#endif
 }
diff --git a/drivers/staging/usbip/userspace/src/usbip_network.h b/drivers/staging/usbip/userspace/src/usbip_network.h
index 6a41fd8..0001c46 100644
--- a/drivers/staging/usbip/userspace/src/usbip_network.h
+++ b/drivers/staging/usbip/userspace/src/usbip_network.h
@@ -9,6 +9,10 @@
 #include "../config.h"
 #endif
 
+#ifdef HAVE_GNUTLS
+#include <gnutls/gnutls.h>
+#endif
+
 #include <sys/types.h>
 #include <sysfs/libsysfs.h>
 
@@ -25,6 +29,19 @@ extern char *usbip_port_string;
 extern char *usbip_srp_password;
 void usbip_setup_port_number(char *arg);
 
+/*
+ * Connection handle
+ */
+struct usbip_connection {
+#ifdef HAVE_GNUTLS
+	gnutls_session_t session;
+	gnutls_srp_client_credentials_t srp_client_cred;
+#endif
+	int have_crypto;
+	int sockfd;
+	int server;
+};
+
 /* ---------------------------------------------------------------------- */
 /* Common header for all the kinds of PDUs. */
 struct op_common {
@@ -44,6 +61,7 @@ struct op_common {
 #define ERR_PERM       0x06
 #define ERR_NOTFOUND   0x07
 #define ERR_NOAUTH     0x08
+#define ERR_INUSE      0x09
 	uint32_t status; /* op_code status (for reply) */
 
 } __attribute__((packed));
@@ -194,19 +212,40 @@ void usbip_net_pack_usb_device(int pack, struct usbip_usb_device *udev);
 void usbip_net_pack_usb_interface(int pack, struct usbip_usb_interface *uinf);
 const char *usbip_net_strerror(int status);
 
-ssize_t usbip_net_recv(int sockfd, void *buff, size_t bufflen);
-ssize_t usbip_net_send(int sockfd, void *buff, size_t bufflen);
-int usbip_net_send_op_common(int sockfd, uint32_t code, uint32_t status);
+ssize_t usbip_net_recv(struct usbip_connection *conn, void *buff, size_t
+	bufflen);
+ssize_t usbip_net_send(struct usbip_connection *conn, void *buff, size_t
+	bufflen);
+int usbip_net_send_op_common(struct usbip_connection *conn, uint32_t code,
+	uint32_t status);
 /*
  * Receive opcode.
  * Returns: 0 on success, negative error code (that may be passed to
  * usbip_net_strerror) on failure.
  */
-int usbip_net_recv_op_common(int sockfd, uint16_t *code);
+int usbip_net_recv_op_common(struct usbip_connection *conn, uint16_t *code);
 int usbip_net_set_reuseaddr(int sockfd);
 int usbip_net_set_nodelay(int sockfd);
 int usbip_net_set_keepalive(int sockfd);
 int usbip_net_set_v6only(int sockfd);
-int usbip_net_connect(char *hostname);
+/*
+ * Connect to the server. Performs the TCP connection attempt
+ * and - if necessary - the TLS handshake used for authentication.
+ *
+ * Newly generated connection parameters are stored in the - caller
+ * allocated - usbip_connection struct conn.
+ *
+ * Returns:
+ *	0 on success
+ *	negative error code on failure
+ */
+int usbip_net_connect(char *hostname, struct usbip_connection *conn);
+int usbip_net_accept(int listenfd, struct usbip_connection *conn);
+int usbip_net_srp_server_handshake(struct usbip_connection *conn);
+/*
+ * Shuts down the TLS connection, but leaves the socket intact
+ */
+void usbip_net_bye(struct usbip_connection *conn);
+int usbip_net_init_gnutls(void);
 
 #endif /* __USBIP_NETWORK_H */
diff --git a/drivers/staging/usbip/userspace/src/usbipd.c b/drivers/staging/usbip/userspace/src/usbipd.c
index 6550460..6bd97a0 100644
--- a/drivers/staging/usbip/userspace/src/usbipd.c
+++ b/drivers/staging/usbip/userspace/src/usbipd.c
@@ -59,6 +59,7 @@
 #define DEFAULT_PID_FILE "/var/run/" PROGNAME ".pid"
 
 static const char usbip_version_string[] = PACKAGE_STRING;
+static int need_auth;
 
 static const char usbipd_help_string[] =
 	"usage: usbipd [options]\n"
@@ -93,78 +94,6 @@ static const char usbipd_help_string[] =
 	"	-v, --version\n"
 	"		Show version.\n";
 
-static int need_auth;
-#ifdef HAVE_GNUTLS
-static gnutls_datum_t srp_salt, srp_verifier;
-static gnutls_srp_server_credentials_t srp_cred;
-
-#define SRP_GROUP gnutls_srp_2048_group_generator
-#define SRP_PRIME gnutls_srp_2048_group_prime
-
-static int net_srp_callback(gnutls_session_t sess, const char *username,
-	gnutls_datum_t *nsalt, gnutls_datum_t *nverifier, gnutls_datum_t *g,
-	gnutls_datum_t *n)
-{
-	/*
-	 * GnuTLS expects us to allocate all data returned from callbacks
-	 * using gnutls_malloc(), thus, we have to create a fresh copy of
-	 * our static credentials for every connection.
-	 */
-	nsalt->data = gnutls_malloc(srp_salt.size);
-	nverifier->data = gnutls_malloc(srp_verifier.size);
-	if (nsalt->data == NULL || nverifier->data == NULL) {
-		gnutls_free(nsalt->data);
-		gnutls_free(nverifier->data);
-		return -1;
-	}
-	nsalt->size = srp_salt.size;
-	nverifier->size = srp_verifier.size;
-	memcpy(nverifier->data, srp_verifier.data, srp_verifier.size);
-	memcpy(nsalt->data, srp_salt.data, srp_salt.size);
-
-	*g = SRP_GROUP;
-	*n = SRP_PRIME;
-
-	/* We only have a single session, thus, ignore it */
-	(void) sess;
-
-	if (strcmp(username, "dummyuser"))
-		/* User invalid, stored dummy data in g and n. */
-		return 1;
-
-	return 0;
-}
-
-static int net_srp_server_handshake(int connfd)
-{
-	int ret;
-	gnutls_session_t session;
-
-	if (gnutls_init(&session, GNUTLS_SERVER) != 0)
-		return -1;
-	gnutls_priority_set_direct(session, "NORMAL:-KX-ALL:+SRP", NULL);
-	if (gnutls_credentials_set(session, GNUTLS_CRD_SRP, srp_cred) != 0)
-		return -1;
-
-	gnutls_transport_set_int(session, connfd);
-
-	do {
-		ret = gnutls_handshake(session);
-	} while (ret < 0 && gnutls_error_is_fatal(ret) == 0);
-
-	if (ret < 0)
-		err("GnuTLS handshake failed (%s)", gnutls_strerror(ret));
-	else
-		info("GnuTLS handshake completed");
-
-	if (gnutls_bye(session, GNUTLS_SHUT_RDWR) != 0)
-		err("Unable to shutdown TLS connection.");
-	gnutls_deinit(session);
-
-	return ret;
-}
-#endif
-
 static void usbipd_help(void)
 {
 	printf("%s\n", usbipd_help_string);
@@ -225,7 +154,7 @@ static int check_allowed(char *acls, int sockfd)
 	return match;
 }
 
-static int recv_request_import(int sockfd)
+static int recv_request_import(struct usbip_connection *conn)
 {
 	struct op_import_request req;
 	struct op_common reply;
@@ -240,7 +169,7 @@ static int recv_request_import(int sockfd)
 	memset(&req, 0, sizeof(req));
 	memset(&reply, 0, sizeof(reply));
 
-	rc = usbip_net_recv(sockfd, &req, sizeof(req));
+	rc = usbip_net_recv(conn, &req, sizeof(req));
 	if (rc < 0) {
 		dbg("usbip_net_recv failed: import request");
 		return -1;
@@ -258,12 +187,7 @@ static int recv_request_import(int sockfd)
 
 	if (found) {
 		/* should set TCP_NODELAY for usbip */
-		usbip_net_set_nodelay(sockfd);
-
-		/* export device needs a TCP/IP socket descriptor */
-		rc = usbip_host_export_device(edev, sockfd);
-		if (rc < 0)
-			error = ERR_SYSERR;
+		usbip_net_set_nodelay(conn->sockfd);
 
 		/* check for allowed IPs */
 		snprintf(ip_attr_path, sizeof(ip_attr_path), "%s/%s:%d.%d/%s",
@@ -276,7 +200,8 @@ static int recv_request_import(int sockfd)
 			if (rc < 0) {
 				err("Unable to open sysfs");
 				error = ERR_SYSERR;
-			} else if (check_allowed(usbip_acl->value, sockfd) != 1) {
+			} else if (check_allowed(usbip_acl->value,
+						conn->sockfd) != 1) {
 				info("Access denied to device %s",
 					edev->udev.busid);
 				error = ERR_PERM;
@@ -285,12 +210,22 @@ static int recv_request_import(int sockfd)
 		} else {
 			err("failed to get ip list");
 		}
+
+		/*
+		 * There is a race condition here: Other clients might
+		 * take it, as this check doesn't lock the device
+		 * However, this seems hardly avoidable here.
+		 */
+		if (edev->status != SDEV_ST_AVAILABLE) {
+			error = ERR_INUSE;
+			found = 0;
+		}
 	} else {
 		info("requested device not found: %s", req.busid);
 		error = ERR_NOTFOUND;
 	}
 
-	rc = usbip_net_send_op_common(sockfd, OP_REP_IMPORT, error);
+	rc = usbip_net_send_op_common(conn, OP_REP_IMPORT, error);
 	if (rc < 0) {
 		dbg("usbip_net_send_op_common failed: %#0x", OP_REP_IMPORT);
 		return -1;
@@ -304,18 +239,27 @@ static int recv_request_import(int sockfd)
 	memcpy(&pdu_udev, &edev->udev, sizeof(pdu_udev));
 	usbip_net_pack_usb_device(1, &pdu_udev);
 
-	rc = usbip_net_send(sockfd, &pdu_udev, sizeof(pdu_udev));
+	rc = usbip_net_send(conn, &pdu_udev, sizeof(pdu_udev));
 	if (rc < 0) {
 		dbg("usbip_net_send failed: devinfo");
 		return -1;
 	}
 
+	usbip_net_bye(conn);
+
+	/* export device needs a TCP/IP socket descriptor */
+	rc = usbip_host_export_device(edev, conn->sockfd);
+	if (rc < 0) {
+		err("usbip_host_export_device");
+		return -1;
+	}
+
 	dbg("import request busid %s: complete", req.busid);
 
 	return 0;
 }
 
-static int send_reply_devlist(int connfd)
+static int send_reply_devlist(struct usbip_connection *conn)
 {
 	struct usbip_exported_device *edev;
 	struct usbip_usb_device pdu_udev;
@@ -332,14 +276,14 @@ static int send_reply_devlist(int connfd)
 	}
 	info("exportable devices: %d", reply.ndev);
 
-	rc = usbip_net_send_op_common(connfd, OP_REP_DEVLIST, ERR_OK);
+	rc = usbip_net_send_op_common(conn, OP_REP_DEVLIST, ERR_OK);
 	if (rc < 0) {
 		dbg("usbip_net_send_op_common failed: %#0x", OP_REP_DEVLIST);
 		return -1;
 	}
 	PACK_OP_DEVLIST_REPLY(1, &reply);
 
-	rc = usbip_net_send(connfd, &reply, sizeof(reply));
+	rc = usbip_net_send(conn, &reply, sizeof(reply));
 	if (rc < 0) {
 		dbg("usbip_net_send failed: %#0x", OP_REP_DEVLIST);
 		return -1;
@@ -351,7 +295,7 @@ static int send_reply_devlist(int connfd)
 		memcpy(&pdu_udev, &edev->udev, sizeof(pdu_udev));
 		usbip_net_pack_usb_device(1, &pdu_udev);
 
-		rc = usbip_net_send(connfd, &pdu_udev, sizeof(pdu_udev));
+		rc = usbip_net_send(conn, &pdu_udev, sizeof(pdu_udev));
 		if (rc < 0) {
 			dbg("usbip_net_send failed: pdu_udev");
 			return -1;
@@ -362,7 +306,7 @@ static int send_reply_devlist(int connfd)
 			memcpy(&pdu_uinf, &edev->uinf[i], sizeof(pdu_uinf));
 			usbip_net_pack_usb_interface(1, &pdu_uinf);
 
-			rc = usbip_net_send(connfd, &pdu_uinf,
+			rc = usbip_net_send(conn, &pdu_uinf,
 					    sizeof(pdu_uinf));
 			if (rc < 0) {
 				dbg("usbip_net_send failed: pdu_uinf");
@@ -374,20 +318,20 @@ static int send_reply_devlist(int connfd)
 	return 0;
 }
 
-static int recv_request_devlist(int connfd)
+static int recv_request_devlist(struct usbip_connection *conn)
 {
 	struct op_devlist_request req;
 	int rc;
 
 	memset(&req, 0, sizeof(req));
 
-	rc = usbip_net_recv(connfd, &req, sizeof(req));
+	rc = usbip_net_recv(conn, &req, sizeof(req));
 	if (rc < 0) {
 		dbg("usbip_net_recv failed: devlist request");
 		return -1;
 	}
 
-	rc = send_reply_devlist(connfd);
+	rc = send_reply_devlist(conn);
 	if (rc < 0) {
 		dbg("send_reply_devlist failed");
 		return -1;
@@ -396,7 +340,7 @@ static int recv_request_devlist(int connfd)
 	return 0;
 }
 
-static int recv_pdu(int connfd)
+static int recv_pdu(struct usbip_connection *conn)
 {
 	int auth = !need_auth, cont = 1, ret;
 
@@ -413,19 +357,19 @@ static int recv_pdu(int connfd)
 	while (cont) {
 		uint16_t code = OP_UNSPEC;
 
-		ret = usbip_net_recv_op_common(connfd, &code);
+		ret = usbip_net_recv_op_common(conn, &code);
 		if (ret < 0) {
 			dbg("could not receive opcode: %#0x: %s", code,
 				usbip_net_strerror(ret));
 			return -1;
 		}
 
-		info("received request: %#0x(%d)", code, connfd);
+		info("received request: %#0x", code);
 
 		/* We require an authenticated encryption */
 		if (!auth && code != OP_REQ_STARTTLS) {
 			info("Unauthenticated connection attempt");
-			usbip_net_send_op_common(connfd, OP_REPLY, ERR_AUTHREQ);
+			usbip_net_send_op_common(conn, OP_REPLY, ERR_AUTHREQ);
 			return -1;
 		}
 
@@ -433,15 +377,16 @@ static int recv_pdu(int connfd)
 #ifdef HAVE_GNUTLS
 		case OP_REQ_STARTTLS:
 			if (!need_auth) {
-				usbip_net_send_op_common(connfd, OP_REPLY,
+				usbip_net_send_op_common(conn, OP_REPLY,
 					ERR_NOAUTH);
 				err("Unexpected TLS handshake attempt (client "
 					"uses password, server doesn't)");
 				ret = -1;
 			} else {
-				usbip_net_send_op_common(connfd, OP_REPLY,
+				usbip_net_send_op_common(conn, OP_REPLY,
 					ERR_OK);
-				ret = net_srp_server_handshake(connfd);
+				err("Starting handshake");
+				ret = usbip_net_srp_server_handshake(conn);
 				if (ret != 0)
 					err("TLS handshake failed");
 				auth = 1;
@@ -449,11 +394,11 @@ static int recv_pdu(int connfd)
 			break;
 #endif
 		case OP_REQ_DEVLIST:
-			ret = recv_request_devlist(connfd);
+			ret = recv_request_devlist(conn);
 			cont = 0;
 			break;
 		case OP_REQ_IMPORT:
-			ret = recv_request_import(connfd);
+			ret = recv_request_import(conn);
 			cont = 0;
 			break;
 		case OP_REQ_DEVINFO:
@@ -464,9 +409,9 @@ static int recv_pdu(int connfd)
 		}
 
 		if (ret == 0)
-			info("request %#0x(%d): complete", code, connfd);
+			info("request %#0x: complete", code);
 		else {
-			info("request %#0x(%d): failed", code, connfd);
+			info("request %#0x: failed", code);
 			break;
 		}
 	}
@@ -474,71 +419,23 @@ static int recv_pdu(int connfd)
 	return ret;
 }
 
-#ifdef HAVE_LIBWRAP
-static int tcpd_auth(int connfd)
-{
-	struct request_info request;
-	int rc;
-
-	request_init(&request, RQ_DAEMON, PROGNAME, RQ_FILE, connfd, 0);
-	fromhost(&request);
-	rc = hosts_access(&request);
-	if (rc == 0)
-		return -1;
-
-	return 0;
-}
-#endif
-
-static int do_accept(int listenfd)
-{
-	int connfd;
-	struct sockaddr_storage ss;
-	socklen_t len = sizeof(ss);
-	char host[NI_MAXHOST], port[NI_MAXSERV];
-	int rc;
-
-	memset(&ss, 0, sizeof(ss));
-
-	connfd = accept(listenfd, (struct sockaddr *)&ss, &len);
-	if (connfd < 0) {
-		err("failed to accept connection");
-		return -1;
-	}
-
-	rc = getnameinfo((struct sockaddr *)&ss, len, host, sizeof(host),
-			 port, sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV);
-	if (rc)
-		err("getnameinfo: %s", gai_strerror(rc));
-
-#ifdef HAVE_LIBWRAP
-	rc = tcpd_auth(connfd);
-	if (rc < 0) {
-		info("denied access from %s", host);
-		close(connfd);
-		return -1;
-	}
-#endif
-	info("connection from %s, port %s", host, port);
-
-	return connfd;
-}
-
 int process_request(int listenfd)
 {
 	pid_t childpid;
-	int connfd;
+	struct usbip_connection conn;
+	int rc;
 
-	connfd = do_accept(listenfd);
-	if (connfd < 0)
+	rc = usbip_net_accept(listenfd, &conn);
+	if (rc < 0)
 		return -1;
 	childpid = fork();
 	if (childpid == 0) {
 		close(listenfd);
-		recv_pdu(connfd);
+		recv_pdu(&conn);
+		usbip_net_bye(&conn);
 		exit(0);
 	}
-	close(connfd);
+	close(conn.sockfd);
 	return 0;
 }
 
@@ -775,37 +672,6 @@ static int do_standalone_mode(int daemonize, int ipv4, int ipv6)
 	return 0;
 }
 
-#ifdef HAVE_GNUTLS
-static int usbip_init_gnutls(void)
-{
-	int ret;
-
-	gnutls_global_init();
-
-	srp_salt.data = gnutls_malloc(16);
-	if (!srp_salt.data)
-		return GNUTLS_E_MEMORY_ERROR;
-
-	ret = gnutls_rnd(GNUTLS_RND_NONCE, srp_salt.data, 16);
-	if (ret < 0)
-		return ret;
-	srp_salt.size = 16;
-
-	ret = gnutls_srp_allocate_server_credentials(&srp_cred);
-	if (ret < 0)
-		return ret;
-
-	ret = gnutls_srp_verifier("dummyuser", optarg, &srp_salt, &SRP_GROUP,
-		&SRP_PRIME, &srp_verifier);
-	if (ret < 0)
-		return ret;
-
-	gnutls_srp_set_server_credentials_function(srp_cred, net_srp_callback);
-
-	return GNUTLS_E_SUCCESS;
-}
-#endif
-
 int main(int argc, char *argv[])
 {
 	static const struct option longopts[] = {
@@ -859,7 +725,7 @@ int main(int argc, char *argv[])
 		case 's':
 #ifdef HAVE_GNUTLS
 			need_auth = 1;
-			ret = usbip_init_gnutls();
+			ret = usbip_net_init_gnutls();
 			if (ret < 0) {
 				err("Unable to initialize GnuTLS: %s",
 					gnutls_strerror(ret));
-- 
1.8.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/7] staging: usbip: Exchange session keys in userspace
  2013-09-19 14:11 [PATCH 0/7] staging: usbip: Extend crypto support Dominik Paulus
  2013-09-19 14:11 ` [PATCH 1/7] staging: usbip: TLS for all userspace communication Dominik Paulus
@ 2013-09-19 14:11 ` Dominik Paulus
  2013-09-19 14:11 ` [PATCH 3/7] staging: usbip: Pass session keys to the kernel Dominik Paulus
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Dominik Paulus @ 2013-09-19 14:11 UTC (permalink / raw)
  To: usbip-devel
  Cc: Dominik Paulus, Tobias Polzer, Greg Kroah-Hartman, Masanari Iida,
	Kurt Kanzenbach, Stefan Reif, Joe Perches, Bart Westgeest,
	Jake Champlin, Ilija Hadzic, Anthony Foiani, Bernard Blackham,
	Harvey Yang, linux-usb, devel, linux-kernel, linux-kernel

In preparation for the kernel crypto support, we exchange two - randomly
generated - session keys between usbip and usbipd to be used for
encrypting all traffic generated in kernelspace. We use two different
128-bit keys, one for sending and one for receiving. Both are generated
by the client (usbip, probably has more entropy available than the
server) and transferred over the already established TLS connection.

As this breaks compatibility with older clients supporting userspace
encryption, the protocol version is increased.

Signed-off-by: Dominik Paulus <dominik.paulus@fau.de>
Signed-off-by: Tobias Polzer <tobias.polzer@fau.de>
---
 .../staging/usbip/userspace/libsrc/usbip_common.h  | 21 ++++++++++
 .../usbip/userspace/libsrc/usbip_host_driver.c     |  5 ++-
 .../usbip/userspace/libsrc/usbip_host_driver.h     |  3 +-
 .../staging/usbip/userspace/libsrc/vhci_driver.c   | 19 +++------
 .../staging/usbip/userspace/libsrc/vhci_driver.h   |  9 ++---
 drivers/staging/usbip/userspace/src/usbip_attach.c | 47 +++++++++++++++++++++-
 .../staging/usbip/userspace/src/usbip_network.h    |  2 +-
 drivers/staging/usbip/userspace/src/usbipd.c       | 32 ++++++++++++---
 8 files changed, 106 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/usbip/userspace/libsrc/usbip_common.h b/drivers/staging/usbip/userspace/libsrc/usbip_common.h
index 938ad1c..f804c04 100644
--- a/drivers/staging/usbip/userspace/libsrc/usbip_common.h
+++ b/drivers/staging/usbip/userspace/libsrc/usbip_common.h
@@ -127,6 +127,27 @@ struct usbip_usb_device {
 	uint8_t bNumInterfaces;
 } __attribute__((packed));
 
+
+/*
+ * These structs contain the configuration
+ * data to be passed to the kernel
+ */
+struct host_conf {
+	int sockfd;
+	uint8_t use_crypto;
+	uint8_t key1[16];
+	uint8_t key2[16];
+};
+struct vhci_conf {
+	uint8_t port;
+	int sockfd;
+	uint32_t devid;
+	uint32_t speed;
+	uint8_t use_crypto;
+	uint8_t key1[16];
+	uint8_t key2[16];
+};
+
 #define to_string(s)	#s
 
 void dump_usb_interface(struct usbip_usb_interface *);
diff --git a/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c b/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
index 71a449c..60247f2 100644
--- a/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
+++ b/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
@@ -332,7 +332,8 @@ int usbip_host_refresh_device_list(void)
 	return 0;
 }
 
-int usbip_host_export_device(struct usbip_exported_device *edev, int sockfd)
+int usbip_host_export_device(struct usbip_exported_device *edev,
+		struct host_conf *conf)
 {
 	char attr_name[] = "usbip_sockfd";
 	char attr_path[SYSFS_PATH_MAX];
@@ -366,7 +367,7 @@ int usbip_host_export_device(struct usbip_exported_device *edev, int sockfd)
 		return -1;
 	}
 
-	snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", sockfd);
+	snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", conf->sockfd);
 	dbg("write: %s", sockfd_buff);
 
 	ret = sysfs_write_attribute(attr, sockfd_buff, strlen(sockfd_buff));
diff --git a/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.h b/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.h
index 34fd14c..ceaf7cc 100644
--- a/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.h
+++ b/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.h
@@ -42,7 +42,8 @@ int usbip_host_driver_open(void);
 void usbip_host_driver_close(void);
 
 int usbip_host_refresh_device_list(void);
-int usbip_host_export_device(struct usbip_exported_device *edev, int sockfd);
+int usbip_host_export_device(struct usbip_exported_device *edev,
+		struct host_conf *conf);
 struct usbip_exported_device *usbip_host_get_device(int num);
 
 #endif /* __USBIP_HOST_DRIVER_H */
diff --git a/drivers/staging/usbip/userspace/libsrc/vhci_driver.c b/drivers/staging/usbip/userspace/libsrc/vhci_driver.c
index 1091bb2..d1d45bb 100644
--- a/drivers/staging/usbip/userspace/libsrc/vhci_driver.c
+++ b/drivers/staging/usbip/userspace/libsrc/vhci_driver.c
@@ -467,8 +467,8 @@ int usbip_vhci_get_free_port(void)
 	return -1;
 }
 
-int usbip_vhci_attach_device2(uint8_t port, int sockfd, uint32_t devid,
-		uint32_t speed) {
+int usbip_vhci_attach_device(struct vhci_conf *conf)
+{
 	struct sysfs_attribute *attr_attach;
 	char buff[200]; /* what size should be ? */
 	int ret;
@@ -481,7 +481,7 @@ int usbip_vhci_attach_device2(uint8_t port, int sockfd, uint32_t devid,
 	}
 
 	snprintf(buff, sizeof(buff), "%u %u %u %u",
-			port, sockfd, devid, speed);
+			conf->port, conf->sockfd, conf->devid, conf->speed);
 	dbg("writing: %s", buff);
 
 	ret = sysfs_write_attribute(attr_attach, buff, strlen(buff));
@@ -490,25 +490,16 @@ int usbip_vhci_attach_device2(uint8_t port, int sockfd, uint32_t devid,
 		return -1;
 	}
 
-	dbg("attached port: %d", port);
+	dbg("attached port: %d", conf->port);
 
 	return 0;
 }
 
-static unsigned long get_devid(uint8_t busnum, uint8_t devnum)
+unsigned long get_devid(uint8_t busnum, uint8_t devnum)
 {
 	return (busnum << 16) | devnum;
 }
 
-/* will be removed */
-int usbip_vhci_attach_device(uint8_t port, int sockfd, uint8_t busnum,
-		uint8_t devnum, uint32_t speed)
-{
-	int devid = get_devid(busnum, devnum);
-
-	return usbip_vhci_attach_device2(port, sockfd, devid, speed);
-}
-
 int usbip_vhci_detach_device(uint8_t port)
 {
 	struct sysfs_attribute  *attr_detach;
diff --git a/drivers/staging/usbip/userspace/libsrc/vhci_driver.h b/drivers/staging/usbip/userspace/libsrc/vhci_driver.h
index 89949aa..325d0fa 100644
--- a/drivers/staging/usbip/userspace/libsrc/vhci_driver.h
+++ b/drivers/staging/usbip/userspace/libsrc/vhci_driver.h
@@ -55,13 +55,10 @@ int  usbip_vhci_refresh_device_list(void);
 
 
 int usbip_vhci_get_free_port(void);
-int usbip_vhci_attach_device2(uint8_t port, int sockfd, uint32_t devid,
-		uint32_t speed);
-
-/* will be removed */
-int usbip_vhci_attach_device(uint8_t port, int sockfd, uint8_t busnum,
-		uint8_t devnum, uint32_t speed);
+int usbip_vhci_attach_device(struct vhci_conf *conf);
 
 int usbip_vhci_detach_device(uint8_t port);
 
+unsigned long get_devid(uint8_t busnum, uint8_t devnum);
+
 #endif /* __VHCI_DRIVER_H */
diff --git a/drivers/staging/usbip/userspace/src/usbip_attach.c b/drivers/staging/usbip/userspace/src/usbip_attach.c
index 9f4a064..7b8a5db 100644
--- a/drivers/staging/usbip/userspace/src/usbip_attach.c
+++ b/drivers/staging/usbip/userspace/src/usbip_attach.c
@@ -29,6 +29,13 @@
 #include <unistd.h>
 #include <errno.h>
 
+#include "../config.h"
+
+#ifdef HAVE_GNUTLS
+#include <gnutls/gnutls.h>
+#include <gnutls/crypto.h>
+#endif
+
 #include "vhci_driver.h"
 #include "usbip_common.h"
 #include "usbip_network.h"
@@ -91,6 +98,38 @@ static int import_device(struct usbip_connection *conn, struct usbip_usb_device
 {
 	int rc;
 	int port;
+	struct vhci_conf conf;
+
+	conf.use_crypto = 0;
+#ifdef HAVE_GNUTLS
+	if (conn->have_crypto) {
+		dbg("Generating session key and sending it to client");
+
+		rc = gnutls_rnd(GNUTLS_RND_RANDOM, conf.key1,
+				sizeof(conf.key1));
+		if (rc < 0) {
+			err("Session key generation failed: %s",
+					gnutls_strerror(rc));
+			return -1;
+		}
+		rc = gnutls_rnd(GNUTLS_RND_RANDOM, conf.key2,
+				sizeof(conf.key2));
+		if (rc < 0) {
+			err("Session key generation failed: %s",
+					gnutls_strerror(rc));
+			return -1;
+		}
+
+		if (usbip_net_send(conn, (void *) conf.key1, sizeof(conf.key1))
+				< 0 || usbip_net_send(conn, (void *) conf.key2,
+					sizeof(conf.key2)) < 0) {
+			err("Unable to send session key to client");
+			return -1;
+		}
+
+		conf.use_crypto = 1;
+	}
+#endif
 
 	rc = usbip_vhci_driver_open();
 	if (rc < 0) {
@@ -106,9 +145,13 @@ static int import_device(struct usbip_connection *conn, struct usbip_usb_device
 	}
 
 	usbip_net_bye(conn);
-	rc = usbip_vhci_attach_device(port, conn->sockfd, udev->busnum,
-				      udev->devnum, udev->speed);
 
+	conf.port = port;
+	conf.sockfd = conn->sockfd;
+	conf.devid = get_devid(udev->busnum, udev->devnum);
+	conf.speed = udev->speed;
+
+	rc = usbip_vhci_attach_device(&conf);
 	if (rc < 0) {
 		err("import device");
 		usbip_vhci_driver_close();
diff --git a/drivers/staging/usbip/userspace/src/usbip_network.h b/drivers/staging/usbip/userspace/src/usbip_network.h
index 0001c46..5222e4c 100644
--- a/drivers/staging/usbip/userspace/src/usbip_network.h
+++ b/drivers/staging/usbip/userspace/src/usbip_network.h
@@ -22,7 +22,7 @@
  * Protocol version. Incremented only on non-backwards-compatible
  * changes.
  */
-#define PROTOCOL_VERSION 0x111
+#define PROTOCOL_VERSION 0x112
 
 extern int usbip_port;
 extern char *usbip_port_string;
diff --git a/drivers/staging/usbip/userspace/src/usbipd.c b/drivers/staging/usbip/userspace/src/usbipd.c
index 6bd97a0..1a80bb6 100644
--- a/drivers/staging/usbip/userspace/src/usbipd.c
+++ b/drivers/staging/usbip/userspace/src/usbipd.c
@@ -245,13 +245,33 @@ static int recv_request_import(struct usbip_connection *conn)
 		return -1;
 	}
 
-	usbip_net_bye(conn);
+	if (found) {
+		struct host_conf conf = {
+			.sockfd = conn->sockfd,
+			.use_crypto = 0
+		};
 
-	/* export device needs a TCP/IP socket descriptor */
-	rc = usbip_host_export_device(edev, conn->sockfd);
-	if (rc < 0) {
-		err("usbip_host_export_device");
-		return -1;
+#ifdef HAVE_GNUTLS
+		if (conn->have_crypto) {
+			if (usbip_net_recv(conn, (void *) conf.key1,
+						sizeof(conf.key1)) < 0 ||
+					usbip_net_recv(conn, (void *) conf.key2,
+						sizeof(conf.key2)) < 0) {
+				err("Unable to receive session key");
+				return -1;
+			}
+			conf.use_crypto = 1;
+		}
+#endif
+
+		usbip_net_bye(conn);
+		/* export device needs a TCP/IP socket descriptor */
+		conf.sockfd = conn->sockfd;
+		rc = usbip_host_export_device(edev, &conf);
+		if (rc < 0) {
+			err("usbip_host_export_device");
+			return -1;
+		}
 	}
 
 	dbg("import request busid %s: complete", req.busid);
-- 
1.8.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/7] staging: usbip: Pass session keys to the kernel
  2013-09-19 14:11 [PATCH 0/7] staging: usbip: Extend crypto support Dominik Paulus
  2013-09-19 14:11 ` [PATCH 1/7] staging: usbip: TLS for all userspace communication Dominik Paulus
  2013-09-19 14:11 ` [PATCH 2/7] staging: usbip: Exchange session keys in userspace Dominik Paulus
@ 2013-09-19 14:11 ` Dominik Paulus
  2013-09-19 14:11 ` [PATCH 4/7] staging: usbip: Wrap kernel_sendmsg()/recvmsg() Dominik Paulus
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Dominik Paulus @ 2013-09-19 14:11 UTC (permalink / raw)
  To: usbip-devel
  Cc: Dominik Paulus, Tobias Polzer, Greg Kroah-Hartman, Masanari Iida,
	Kurt Kanzenbach, Stefan Reif, Joe Perches, Bart Westgeest,
	Jake Champlin, Ilija Hadzic, Anthony Foiani, Bernard Blackham,
	Harvey Yang, linux-usb, devel, linux-kernel, linux-kernel

This extends the userspace code to write the generated session keys to
sysfs in hexadecimal encoding after establishing the connection.
The kernel code is modified to parse the session keys.

Signed-off-by: Dominik Paulus <dominik.paulus@fau.de>
Signed-off-by: Tobias Polzer <tobias.polzer@fau.de>
---
 drivers/staging/usbip/stub_dev.c                   | 26 +++++++++++++++++++-
 drivers/staging/usbip/usbip_common.h               | 10 ++++++++
 .../staging/usbip/userspace/libsrc/usbip_common.c  | 14 +++++++++++
 .../staging/usbip/userspace/libsrc/usbip_common.h  |  2 ++
 .../usbip/userspace/libsrc/usbip_host_driver.c     | 15 ++++++++----
 .../staging/usbip/userspace/libsrc/vhci_driver.c   |  8 ++++---
 drivers/staging/usbip/vhci_sysfs.c                 | 28 +++++++++++++++++++++-
 7 files changed, 94 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
index c44d5f2..f7bc6e1 100644
--- a/drivers/staging/usbip/stub_dev.c
+++ b/drivers/staging/usbip/stub_dev.c
@@ -87,13 +87,37 @@ static ssize_t store_sockfd(struct device *dev, struct device_attribute *attr,
 	int sockfd = 0;
 	struct socket *socket;
 	ssize_t err = -EINVAL;
+	char sendkey[USBIP_KEYSIZE], recvkey[USBIP_KEYSIZE];
+	char sendkey_hex[2 * USBIP_KEYSIZE + 1];
+	char recvkey_hex[2 * USBIP_KEYSIZE + 1];
 
 	if (!sdev) {
 		dev_err(dev, "sdev is null\n");
 		return -ENODEV;
 	}
 
-	sscanf(buf, "%d", &sockfd);
+	/*
+	 * Read symmetric crypto keys. They are randomly
+	 * generated by userspace and passed to the kernel
+	 * via sysfs (encoded in hexadecimal)
+	 */
+	if (sscanf(buf, "%d %d %32s %32s", &sockfd, &sdev->ud.use_crypto,
+			sendkey_hex, recvkey_hex) < 1) {
+		dev_err(dev, "Invalid write to sysfs: Invalid sockfd\n");
+		return -EINVAL;
+	}
+	if (sdev->ud.use_crypto) {
+		int i;
+		dev_info(dev, "Using encrypted data transport\n");
+		for (i = USBIP_KEYSIZE - 1; i >= 0; --i) {
+			sendkey_hex[2 * (i + 1)] = 0;
+			sscanf(sendkey_hex + (2 * i), "%2hhX", &sendkey[i]);
+		}
+		for (i = USBIP_KEYSIZE - 1; i >= 0; --i) {
+			recvkey_hex[2 * (i + 1)] = 0;
+			sscanf(recvkey_hex + (2 * i), "%2hhX", &recvkey[i]);
+		}
+	}
 
 	if (sockfd != -1) {
 		dev_info(dev, "stub up\n");
diff --git a/drivers/staging/usbip/usbip_common.h b/drivers/staging/usbip/usbip_common.h
index 7e6c543..96c87ee 100644
--- a/drivers/staging/usbip/usbip_common.h
+++ b/drivers/staging/usbip/usbip_common.h
@@ -32,6 +32,13 @@
 
 #define USBIP_VERSION "1.0.0"
 
+/*
+ * Length of symmetric keys. Currently, this should be fixed at 16 bytes.
+ * Will break code if changed, look at userspace and stub_dev.c/vhci_sysfs.c
+ * where this constant is used before changing.
+ */
+#define USBIP_KEYSIZE 16
+
 #undef pr_fmt
 
 #ifdef DEBUG
@@ -290,6 +297,9 @@ struct usbip_device {
 		void (*reset)(struct usbip_device *);
 		void (*unusable)(struct usbip_device *);
 	} eh_ops;
+
+	/* Crypto support */
+	int use_crypto;
 };
 
 #define kthread_get_run(threadfn, data, namefmt, ...)			   \
diff --git a/drivers/staging/usbip/userspace/libsrc/usbip_common.c b/drivers/staging/usbip/userspace/libsrc/usbip_common.c
index 17e08e0..1ec9cc9 100644
--- a/drivers/staging/usbip/userspace/libsrc/usbip_common.c
+++ b/drivers/staging/usbip/userspace/libsrc/usbip_common.c
@@ -290,3 +290,17 @@ void usbip_names_get_class(char *buff, size_t size, uint8_t class,
 
 	snprintf(buff, size, "%s / %s / %s (%02x/%02x/%02x)", c, s, p, class, subclass, protocol);
 }
+
+/*
+ * Converts a 16-byte key to hexadecimal to be pushed to kernelspace
+ *
+ * Output buffer must be at least 33 bytes long
+ */
+char *keytohex(unsigned char *key, char *out)
+{
+	int i;
+	out[32] = 0;
+	for (i = 0; i != 16; ++i)
+		sprintf(out + (2 * i), "%02X", key[i]);
+	return out;
+}
diff --git a/drivers/staging/usbip/userspace/libsrc/usbip_common.h b/drivers/staging/usbip/userspace/libsrc/usbip_common.h
index f804c04..a5d4d21 100644
--- a/drivers/staging/usbip/userspace/libsrc/usbip_common.h
+++ b/drivers/staging/usbip/userspace/libsrc/usbip_common.h
@@ -168,4 +168,6 @@ void usbip_names_get_product(char *buff, size_t size, uint16_t vendor,
 void usbip_names_get_class(char *buff, size_t size, uint8_t class,
 			   uint8_t subclass, uint8_t protocol);
 
+char *keytohex(unsigned char *key, char *out);
+
 #endif /* __USBIP_COMMON_H */
diff --git a/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c b/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
index 60247f2..be08ad7 100644
--- a/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
+++ b/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
@@ -338,7 +338,7 @@ int usbip_host_export_device(struct usbip_exported_device *edev,
 	char attr_name[] = "usbip_sockfd";
 	char attr_path[SYSFS_PATH_MAX];
 	struct sysfs_attribute *attr;
-	char sockfd_buff[30];
+	char sockfd_buff[512];
 	int ret;
 
 	if (edev->status != SDEV_ST_AVAILABLE) {
@@ -367,10 +367,17 @@ int usbip_host_export_device(struct usbip_exported_device *edev,
 		return -1;
 	}
 
-	snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", conf->sockfd);
-	dbg("write: %s", sockfd_buff);
+	{
+		char key1[33], key2[33];
+		snprintf(sockfd_buff, sizeof(sockfd_buff), "%d %d %s %s\n",
+				conf->sockfd, conf->use_crypto,
+				keytohex(conf->key2, key2),
+				keytohex(conf->key1, key1));
+		dbg("write: %s", sockfd_buff);
+	}
 
-	ret = sysfs_write_attribute(attr, sockfd_buff, strlen(sockfd_buff));
+	ret = sysfs_write_attribute(attr, (const char *) sockfd_buff,
+			strlen(sockfd_buff));
 	if (ret < 0) {
 		dbg("sysfs_write_attribute failed: sockfd %s to %s",
 		    sockfd_buff, attr_path);
diff --git a/drivers/staging/usbip/userspace/libsrc/vhci_driver.c b/drivers/staging/usbip/userspace/libsrc/vhci_driver.c
index d1d45bb..66cdfaf 100644
--- a/drivers/staging/usbip/userspace/libsrc/vhci_driver.c
+++ b/drivers/staging/usbip/userspace/libsrc/vhci_driver.c
@@ -470,7 +470,7 @@ int usbip_vhci_get_free_port(void)
 int usbip_vhci_attach_device(struct vhci_conf *conf)
 {
 	struct sysfs_attribute *attr_attach;
-	char buff[200]; /* what size should be ? */
+	char buff[512], key1[33], key2[33];
 	int ret;
 
 	attr_attach = sysfs_get_device_attr(vhci_driver->hc_device, "attach");
@@ -480,8 +480,10 @@ int usbip_vhci_attach_device(struct vhci_conf *conf)
 		return -1;
 	}
 
-	snprintf(buff, sizeof(buff), "%u %u %u %u",
-			conf->port, conf->sockfd, conf->devid, conf->speed);
+	snprintf(buff, sizeof(buff), "%u %u %u %u %d %s %s",
+			conf->port, conf->sockfd, conf->devid, conf->speed,
+			conf->use_crypto, keytohex(conf->key1, key1),
+			keytohex(conf->key2, key2));
 	dbg("writing: %s", buff);
 
 	ret = sysfs_write_attribute(attr_attach, buff, strlen(buff));
diff --git a/drivers/staging/usbip/vhci_sysfs.c b/drivers/staging/usbip/vhci_sysfs.c
index 9b51586..1ef3f25 100644
--- a/drivers/staging/usbip/vhci_sysfs.c
+++ b/drivers/staging/usbip/vhci_sysfs.c
@@ -174,14 +174,40 @@ static ssize_t store_attach(struct device *dev, struct device_attribute *attr,
 	struct socket *socket;
 	int sockfd = 0;
 	__u32 rhport = 0, devid = 0, speed = 0;
+	unsigned char sendkey[USBIP_KEYSIZE], recvkey[USBIP_KEYSIZE];
+	char sendkey_hex[2*USBIP_KEYSIZE+1], recvkey_hex[2*USBIP_KEYSIZE+1];
+	int use_crypto = 0;
 
 	/*
 	 * @rhport: port number of vhci_hcd
 	 * @sockfd: socket descriptor of an established TCP connection
 	 * @devid: unique device identifier in a remote host
 	 * @speed: usb device speed in a remote host
+	 * @use_crypto: Whether to use an encrypted connection
+	 * @recvkey_hex/@sendkey_hex: Hexadecimal encoded 128bit
+	 *              symmetric encryption keys to be used.
+	 *              Generated randomly for each connection
+	 *              by userspace.
 	 */
-	sscanf(buf, "%u %u %u %u", &rhport, &sockfd, &devid, &speed);
+	sendkey_hex[0] = 0;
+	recvkey_hex[0] = 0;
+	sscanf(buf, "%u %u %u %u %d %32s %32s", &rhport, &sockfd,
+			&devid, &speed, &use_crypto, sendkey_hex,
+			recvkey_hex);
+	if (use_crypto && strlen(sendkey_hex) == 32 &&
+			strlen(recvkey_hex) == 32) {
+		int i;
+		dev_info(dev, "Using encrypted data transport\n");
+		/* Decode decimal representation */
+		for (i = USBIP_KEYSIZE - 1; i >= 0; --i) {
+			sendkey_hex[2 * (i + 1)] = 0;
+			sscanf(sendkey_hex + (2 * i), "%2hhX", &sendkey[i]);
+		}
+		for (i = USBIP_KEYSIZE - 1; i >= 0; --i) {
+			recvkey_hex[2 * (i + 1)] = 0;
+			sscanf(recvkey_hex + (2 * i), "%2hhX", &recvkey[i]);
+		}
+	}
 
 	usbip_dbg_vhci_sysfs("rhport(%u) sockfd(%u) devid(%u) speed(%u)\n",
 			     rhport, sockfd, devid, speed);
-- 
1.8.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/7] staging: usbip: Wrap kernel_sendmsg()/recvmsg()
  2013-09-19 14:11 [PATCH 0/7] staging: usbip: Extend crypto support Dominik Paulus
                   ` (2 preceding siblings ...)
  2013-09-19 14:11 ` [PATCH 3/7] staging: usbip: Pass session keys to the kernel Dominik Paulus
@ 2013-09-19 14:11 ` Dominik Paulus
  2013-09-19 14:11 ` [PATCH 5/7] staging: usbip: Add encryption support to kernel Dominik Paulus
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Dominik Paulus @ 2013-09-19 14:11 UTC (permalink / raw)
  To: usbip-devel
  Cc: Dominik Paulus, Tobias Polzer, Greg Kroah-Hartman, Masanari Iida,
	Kurt Kanzenbach, Stefan Reif, Joe Perches, Bart Westgeest,
	Jake Champlin, Ilija Hadzic, Anthony Foiani, Bernard Blackham,
	Harvey Yang, linux-usb, devel, linux-kernel, linux-kernel

This adds two simple wrappers around kernel_sendmsg() and
kernel_recvmsg() that can be extended to perform additional
cryptographic operations on the data before sending it.

Signed-off-by: Dominik Paulus <dominik.paulus@fau.de>
Signed-off-by: Tobias Polzer <tobias.polzer@fau.de>
---
 drivers/staging/usbip/stub_rx.c      |  2 +-
 drivers/staging/usbip/stub_tx.c      |  6 ++--
 drivers/staging/usbip/usbip_common.c | 66 ++++++++++++++++++++----------------
 drivers/staging/usbip/usbip_common.h |  7 +++-
 drivers/staging/usbip/vhci_rx.c      |  2 +-
 drivers/staging/usbip/vhci_tx.c      |  4 +--
 6 files changed, 48 insertions(+), 39 deletions(-)

diff --git a/drivers/staging/usbip/stub_rx.c b/drivers/staging/usbip/stub_rx.c
index db48a78..6ba9969 100644
--- a/drivers/staging/usbip/stub_rx.c
+++ b/drivers/staging/usbip/stub_rx.c
@@ -553,7 +553,7 @@ static void stub_rx_pdu(struct usbip_device *ud)
 	memset(&pdu, 0, sizeof(pdu));
 
 	/* receive a pdu header */
-	ret = usbip_recv(ud->tcp_socket, &pdu, sizeof(pdu));
+	ret = usbip_recv(ud, &pdu, sizeof(pdu));
 	if (ret != sizeof(pdu)) {
 		dev_err(dev, "recv a header, %d\n", ret);
 		usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
diff --git a/drivers/staging/usbip/stub_tx.c b/drivers/staging/usbip/stub_tx.c
index cd5326a..9a9f9e6 100644
--- a/drivers/staging/usbip/stub_tx.c
+++ b/drivers/staging/usbip/stub_tx.c
@@ -257,8 +257,7 @@ static int stub_send_ret_submit(struct stub_device *sdev)
 			iovnum++;
 		}
 
-		ret = kernel_sendmsg(sdev->ud.tcp_socket, &msg,
-						iov,  iovnum, txsize);
+		ret = usbip_sendmsg(&sdev->ud, &msg, iov, iovnum, txsize);
 		if (ret != txsize) {
 			dev_err(&sdev->interface->dev,
 				"sendmsg failed!, retval %d for %zd\n",
@@ -332,8 +331,7 @@ static int stub_send_ret_unlink(struct stub_device *sdev)
 		iov[0].iov_len  = sizeof(pdu_header);
 		txsize += sizeof(pdu_header);
 
-		ret = kernel_sendmsg(sdev->ud.tcp_socket, &msg, iov,
-				     1, txsize);
+		ret = usbip_sendmsg(&sdev->ud, &msg, iov, 1, txsize);
 		if (ret != txsize) {
 			dev_err(&sdev->interface->dev,
 				"sendmsg failed!, retval %d for %zd\n",
diff --git a/drivers/staging/usbip/usbip_common.c b/drivers/staging/usbip/usbip_common.c
index e3fc749..f46c3d2 100644
--- a/drivers/staging/usbip/usbip_common.c
+++ b/drivers/staging/usbip/usbip_common.c
@@ -339,7 +339,7 @@ void usbip_dump_header(struct usbip_header *pdu)
 EXPORT_SYMBOL_GPL(usbip_dump_header);
 
 /* Receive data over TCP/IP. */
-int usbip_recv(struct socket *sock, void *buf, int size)
+int usbip_recv(struct usbip_device *ud, void *buf, int size)
 {
 	int result;
 	struct msghdr msg;
@@ -352,34 +352,29 @@ int usbip_recv(struct socket *sock, void *buf, int size)
 
 	usbip_dbg_xmit("enter\n");
 
-	if (!sock || !buf || !size) {
-		pr_err("invalid arg, sock %p buff %p size %d\n", sock, buf,
+	if (!ud || !buf || !size) {
+		pr_err("invalid arg, ud %p buff %p size %d\n", ud, buf,
 		       size);
 		return -EINVAL;
 	}
 
-	do {
-		sock->sk->sk_allocation = GFP_NOIO;
-		iov.iov_base    = buf;
-		iov.iov_len     = size;
-		msg.msg_name    = NULL;
-		msg.msg_namelen = 0;
-		msg.msg_control = NULL;
-		msg.msg_controllen = 0;
-		msg.msg_namelen    = 0;
-		msg.msg_flags      = MSG_NOSIGNAL;
-
-		result = kernel_recvmsg(sock, &msg, &iov, 1, size, MSG_WAITALL);
-		if (result <= 0) {
-			pr_debug("receive sock %p buf %p size %u ret %d total %d\n",
-				 sock, buf, size, result, total);
-			goto err;
-		}
-
-		size -= result;
-		buf += result;
-		total += result;
-	} while (size > 0);
+	ud->tcp_socket->sk->sk_allocation = GFP_NOIO;
+	iov.iov_base    = buf;
+	iov.iov_len     = size;
+	msg.msg_name    = NULL;
+	msg.msg_namelen = 0;
+	msg.msg_control = NULL;
+	msg.msg_controllen = 0;
+	msg.msg_namelen    = 0;
+	msg.msg_flags      = MSG_NOSIGNAL;
+
+	result = usbip_recvmsg(ud, &msg, &iov, 1, size, MSG_WAITALL);
+
+	if (result < 0) {
+		pr_debug("receive sock %p buf %p size %u ret %d total %d\n",
+			 ud->tcp_socket, buf, size, result, total);
+		return result;
+	}
 
 	if (usbip_dbg_flag_xmit) {
 		if (!in_interrupt())
@@ -393,9 +388,6 @@ int usbip_recv(struct socket *sock, void *buf, int size)
 			 osize, result, size, total);
 	}
 
-	return total;
-
-err:
 	return result;
 }
 EXPORT_SYMBOL_GPL(usbip_recv);
@@ -634,6 +626,20 @@ static void usbip_pack_iso(struct usbip_iso_packet_descriptor *iso,
 	}
 }
 
+int usbip_recvmsg(struct usbip_device *ud, struct msghdr *msg, struct kvec
+		*vec, size_t num, size_t size, int flags)
+{
+	return kernel_recvmsg(ud->tcp_socket, msg, vec, num, size, flags);
+}
+EXPORT_SYMBOL_GPL(usbip_recvmsg);
+
+int usbip_sendmsg(struct usbip_device *ud, struct msghdr *msg, struct kvec
+		*vec, size_t num, size_t size)
+{
+	return kernel_sendmsg(ud->tcp_socket, msg, vec, num, size);
+}
+EXPORT_SYMBOL_GPL(usbip_sendmsg);
+
 /* must free buffer */
 struct usbip_iso_packet_descriptor*
 usbip_alloc_iso_desc_pdu(struct urb *urb, ssize_t *bufflen)
@@ -680,7 +686,7 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)
 	if (!buff)
 		return -ENOMEM;
 
-	ret = usbip_recv(ud->tcp_socket, buff, size);
+	ret = usbip_recv(ud, buff, size);
 	if (ret != size) {
 		dev_err(&urb->dev->dev, "recv iso_frame_descriptor, %d\n",
 			ret);
@@ -785,7 +791,7 @@ int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
 	if (!(size > 0))
 		return 0;
 
-	ret = usbip_recv(ud->tcp_socket, urb->transfer_buffer, size);
+	ret = usbip_recv(ud, urb->transfer_buffer, size);
 	if (ret != size) {
 		dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
 		if (ud->side == USBIP_STUB) {
diff --git a/drivers/staging/usbip/usbip_common.h b/drivers/staging/usbip/usbip_common.h
index 96c87ee..99f9dab 100644
--- a/drivers/staging/usbip/usbip_common.h
+++ b/drivers/staging/usbip/usbip_common.h
@@ -323,7 +323,7 @@ struct usbip_device {
 void usbip_dump_urb(struct urb *purb);
 void usbip_dump_header(struct usbip_header *pdu);
 
-int usbip_recv(struct socket *sock, void *buf, int size);
+int usbip_recv(struct usbip_device *ui, void *buf, int size);
 struct socket *sockfd_to_socket(unsigned int sockfd);
 
 void usbip_pack_pdu(struct usbip_header *pdu, struct urb *urb, int cmd,
@@ -344,6 +344,11 @@ void usbip_stop_eh(struct usbip_device *ud);
 void usbip_event_add(struct usbip_device *ud, unsigned long event);
 int usbip_event_happened(struct usbip_device *ud);
 
+int usbip_recvmsg(struct usbip_device *ui, struct msghdr *msg, struct kvec
+		*vec, size_t num, size_t size, int flags);
+int usbip_sendmsg(struct usbip_device *ui, struct msghdr *msg, struct kvec
+		*vec, size_t num, size_t size);
+
 static inline int interface_to_busnum(struct usb_interface *interface)
 {
 	struct usb_device *udev = interface_to_usbdev(interface);
diff --git a/drivers/staging/usbip/vhci_rx.c b/drivers/staging/usbip/vhci_rx.c
index d07fcb5..34706a8 100644
--- a/drivers/staging/usbip/vhci_rx.c
+++ b/drivers/staging/usbip/vhci_rx.c
@@ -207,7 +207,7 @@ static void vhci_rx_pdu(struct usbip_device *ud)
 	memset(&pdu, 0, sizeof(pdu));
 
 	/* receive a pdu header */
-	ret = usbip_recv(ud->tcp_socket, &pdu, sizeof(pdu));
+	ret = usbip_recv(ud, &pdu, sizeof(pdu));
 	if (ret < 0) {
 		if (ret == -ECONNRESET)
 			pr_info("connection reset by peer\n");
diff --git a/drivers/staging/usbip/vhci_tx.c b/drivers/staging/usbip/vhci_tx.c
index 409fd99..09663e6 100644
--- a/drivers/staging/usbip/vhci_tx.c
+++ b/drivers/staging/usbip/vhci_tx.c
@@ -115,7 +115,7 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
 			txsize += len;
 		}
 
-		ret = kernel_sendmsg(vdev->ud.tcp_socket, &msg, iov, 3, txsize);
+		ret = usbip_sendmsg(&vdev->ud, &msg, iov, 3, txsize);
 		if (ret != txsize) {
 			pr_err("sendmsg failed!, ret=%d for %zd\n", ret,
 			       txsize);
@@ -184,7 +184,7 @@ static int vhci_send_cmd_unlink(struct vhci_device *vdev)
 		iov[0].iov_len  = sizeof(pdu_header);
 		txsize += sizeof(pdu_header);
 
-		ret = kernel_sendmsg(vdev->ud.tcp_socket, &msg, iov, 1, txsize);
+		ret = usbip_sendmsg(&vdev->ud, &msg, iov, 1, txsize);
 		if (ret != txsize) {
 			pr_err("sendmsg failed!, ret=%d for %zd\n", ret,
 			       txsize);
-- 
1.8.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/7] staging: usbip: Add encryption support to kernel
  2013-09-19 14:11 [PATCH 0/7] staging: usbip: Extend crypto support Dominik Paulus
                   ` (3 preceding siblings ...)
  2013-09-19 14:11 ` [PATCH 4/7] staging: usbip: Wrap kernel_sendmsg()/recvmsg() Dominik Paulus
@ 2013-09-19 14:11 ` Dominik Paulus
  2013-09-23  9:59   ` Dan Carpenter
                     ` (2 more replies)
  2013-09-19 14:11 ` [PATCH 6/7] staging: usbip: Update documentation Dominik Paulus
  2013-09-19 14:11 ` [PATCH 7/7] staging: usbip: Increment version number to 1.2.1 Dominik Paulus
  6 siblings, 3 replies; 13+ messages in thread
From: Dominik Paulus @ 2013-09-19 14:11 UTC (permalink / raw)
  To: usbip-devel
  Cc: Dominik Paulus, Tobias Polzer, Greg Kroah-Hartman, Masanari Iida,
	Kurt Kanzenbach, Stefan Reif, Joe Perches, Bart Westgeest,
	Jake Champlin, Ilija Hadzic, Anthony Foiani, Bernard Blackham,
	Harvey Yang, linux-usb, devel, linux-kernel, linux-kernel

This adds code performing the actual encryption and authentication
operations in the usbip kernel code. The whole data stream may now be
encrypted and authenticated with AES-GCM and symmetric 128 bit keys.

Signed-off-by: Dominik Paulus <dominik.paulus@fau.de>
Signed-off-by: Tobias Polzer <tobias.polzer@fau.de>
---
 drivers/staging/usbip/Kconfig        |   2 +-
 drivers/staging/usbip/stub.h         |   3 +
 drivers/staging/usbip/stub_dev.c     |   8 +
 drivers/staging/usbip/usbip_common.c | 342 ++++++++++++++++++++++++++++++++++-
 drivers/staging/usbip/usbip_common.h |  22 +++
 drivers/staging/usbip/vhci_hcd.c     |   4 +-
 drivers/staging/usbip/vhci_sysfs.c   |  10 +
 7 files changed, 387 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/usbip/Kconfig b/drivers/staging/usbip/Kconfig
index 8860009..87220af 100644
--- a/drivers/staging/usbip/Kconfig
+++ b/drivers/staging/usbip/Kconfig
@@ -1,6 +1,6 @@
 config USBIP_CORE
 	tristate "USB/IP support"
-	depends on USB && NET
+	depends on USB && NET && CRYPTO_GCM && CRYPTO_AES
 	default N
 	---help---
 	  This enables pushing USB packets over IP to allow remote
diff --git a/drivers/staging/usbip/stub.h b/drivers/staging/usbip/stub.h
index cfe75d1..2aaea3a 100644
--- a/drivers/staging/usbip/stub.h
+++ b/drivers/staging/usbip/stub.h
@@ -26,6 +26,9 @@
 #include <linux/types.h>
 #include <linux/usb.h>
 #include <linux/wait.h>
+#include <linux/crypto.h>
+#include <linux/err.h>
+#include <linux/scatterlist.h>
 
 #define STUB_BUSID_OTHER 0
 #define STUB_BUSID_REMOV 1
diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
index f7bc6e1..0e61efb 100644
--- a/drivers/staging/usbip/stub_dev.c
+++ b/drivers/staging/usbip/stub_dev.c
@@ -21,6 +21,7 @@
 #include <linux/file.h>
 #include <linux/kthread.h>
 #include <linux/module.h>
+#include <linux/kfifo.h>
 
 #include "usbip_common.h"
 #include "stub.h"
@@ -137,6 +138,12 @@ static ssize_t store_sockfd(struct device *dev, struct device_attribute *attr,
 
 		spin_unlock_irq(&sdev->ud.lock);
 
+		if (sdev->ud.use_crypto) {
+			err = usbip_init_crypto(&sdev->ud, sendkey, recvkey);
+			if (err < 0)
+				goto err;
+		}
+
 		sdev->ud.tcp_rx = kthread_get_run(stub_rx_loop, &sdev->ud,
 						  "stub_rx");
 		sdev->ud.tcp_tx = kthread_get_run(stub_tx_loop, &sdev->ud,
@@ -298,6 +305,7 @@ static void stub_shutdown_connection(struct usbip_device *ud)
 	}
 
 	/* 3. free used data */
+	usbip_deinit_crypto(ud);
 	stub_device_cleanup_urbs(sdev);
 
 	/* 4. free stub_unlink */
diff --git a/drivers/staging/usbip/usbip_common.c b/drivers/staging/usbip/usbip_common.c
index f46c3d2..0729df6 100644
--- a/drivers/staging/usbip/usbip_common.c
+++ b/drivers/staging/usbip/usbip_common.c
@@ -26,6 +26,8 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <net/sock.h>
+#include <linux/scatterlist.h>
+#include <linux/crypto.h>
 
 #include "usbip_common.h"
 
@@ -626,17 +628,353 @@ static void usbip_pack_iso(struct usbip_iso_packet_descriptor *iso,
 	}
 }
 
+int usbip_init_crypto(struct usbip_device *ud, unsigned char *sendkey, unsigned
+		char *recvkey)
+{
+	int ret;
+
+	ud->use_crypto = 1;
+
+	ud->tfm_recv = crypto_alloc_aead("gcm(aes)", 0, 0);
+	if (IS_ERR(ud->tfm_recv))
+		return -PTR_ERR(ud->tfm_recv);
+	ud->tfm_send = crypto_alloc_aead("gcm(aes)", 0, 0);
+	if (IS_ERR(ud->tfm_send)) {
+		crypto_free_aead(ud->tfm_recv);
+		return -PTR_ERR(ud->tfm_send);
+	}
+	ret = kfifo_alloc(&ud->recv_queue, RECVQ_SIZE, GFP_KERNEL);
+	if (ret) {
+		crypto_free_aead(ud->tfm_recv);
+		crypto_free_aead(ud->tfm_send);
+		return ret;
+	}
+
+	if (crypto_aead_setkey(ud->tfm_send, sendkey, USBIP_KEYSIZE) != 0 ||
+			crypto_aead_setkey(ud->tfm_recv, recvkey,
+				USBIP_KEYSIZE) != 0 ||
+			crypto_aead_setauthsize(ud->tfm_send,
+				USBIP_AUTHSIZE) != 0 ||
+			crypto_aead_setauthsize(ud->tfm_recv,
+				USBIP_AUTHSIZE)) {
+		crypto_free_aead(ud->tfm_recv);
+		crypto_free_aead(ud->tfm_send);
+		kfifo_free(&ud->recv_queue);
+	}
+
+	ud->ctr_send = 0;
+	ud->ctr_recv = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usbip_init_crypto);
+
+void usbip_deinit_crypto(struct usbip_device *ud)
+{
+	if (ud->use_crypto) {
+		crypto_free_aead(ud->tfm_send);
+		crypto_free_aead(ud->tfm_recv);
+		kfifo_free(&ud->recv_queue);
+		ud->use_crypto = 0;
+	}
+}
+EXPORT_SYMBOL_GPL(usbip_deinit_crypto);
+
+struct tcrypt_result {
+	struct completion completion;
+	int err;
+};
+
+static void tcrypt_complete(struct crypto_async_request *req, int err)
+{
+	struct tcrypt_result *res = req->data;
+
+	if (err == -EINPROGRESS)
+		return;
+
+	res->err = err;
+	complete(&res->completion);
+}
+
+/*
+ * Perform encryption/decryption on one chunk of data.
+ * Uses global crypto state stored in usbip_device.
+ * Parameters:
+ * encrypt: 1 to perform encryption, 0 to perform decryption operation
+ * packetsize: Size of the encrypted packet, including the authentication tag,
+ * not including the associated data (length field).
+ * plaintext and ciphertext have to be appropiately managed by the caller
+ * (i.e. they must be at least packetsize bytes long).
+ * Returns: 0 on success
+ */
+static int usbip_crypt(struct usbip_device *ud, int encrypt, uint32_t
+		packetsize, unsigned char *plaintext, unsigned char
+		*ciphertext)
+{
+	struct crypto_aead *tfm;
+	struct aead_request *req;
+	struct tcrypt_result result;
+	struct scatterlist plain, cipher, assoc;
+	char iv[16];
+	u64 *iv_num;
+	u64 iv_net;
+	const int plainsize = packetsize - USBIP_AUTHSIZE;
+	int ret;
+
+	memset(iv, 0, sizeof(iv));
+	if (encrypt) {
+		tfm = ud->tfm_send;
+		iv_num = &ud->ctr_send;
+	} else {
+		tfm = ud->tfm_recv;
+		iv_num = &ud->ctr_recv;
+	}
+	iv_net = cpu_to_be64(*iv_num);
+	memcpy(iv, &iv_net, sizeof(iv_net));
+
+	req = aead_request_alloc(tfm, GFP_KERNEL);
+	if (IS_ERR(req))
+		return -PTR_ERR(req);
+
+	init_completion(&result.completion);
+	aead_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+			tcrypt_complete, &result);
+
+	sg_init_one(&cipher, ciphertext, packetsize);
+	sg_init_one(&plain, plaintext, plainsize);
+	crypto_aead_clear_flags(tfm, ~0);
+
+	if (encrypt)
+		aead_request_set_crypt(req, &plain, &cipher, plainsize, iv);
+	else
+		aead_request_set_crypt(req, &cipher, &plain, packetsize, iv);
+	packetsize = cpu_to_be32(packetsize);
+	sg_init_one(&assoc, &packetsize, sizeof(packetsize));
+	/* Associated data: Unencrypted length tag */
+	aead_request_set_assoc(req, &assoc, sizeof(packetsize));
+
+	if (encrypt)
+		ret = crypto_aead_encrypt(req);
+	else
+		ret = crypto_aead_decrypt(req);
+
+	switch (ret) {
+	case 0: /* Success */
+		break;
+	case -EINPROGRESS:
+	case -EBUSY:
+		wait_for_completion(&result.completion);
+		break;
+	default:
+		aead_request_free(req);
+		return ret;
+	}
+
+	aead_request_free(req);
+
+	(*iv_num)++; /* Increment IV */
+
+	return 0;
+}
+
+/*
+ * Wrapper to kernel_recvmsg. If necessary, also does the necessary decryption.
+ * If decryption is enabled, you _MUST_ pass 1 as parameter for num, i.e.
+ * only receive into a single continuous buffer.
+ */
 int usbip_recvmsg(struct usbip_device *ud, struct msghdr *msg, struct kvec
 		*vec, size_t num, size_t size, int flags)
 {
-	return kernel_recvmsg(ud->tcp_socket, msg, vec, num, size, flags);
+	int ret;
+	size_t total = 0;
+	unsigned char *plainbuf, *cipherbuf;
+
+	/* If crypto is disabled, we just wrap the normal kernel calls. */
+	if (!ud->use_crypto)
+		return kernel_recvmsg(ud->tcp_socket, msg, vec, num, size,
+				flags);
+
+	if (vec[0].iov_len < size)
+		return -EINVAL;
+	if (num != 1)
+		return -EINVAL;
+
+	plainbuf = kmalloc(USBIP_PACKETSIZE, GFP_KERNEL);
+	if (IS_ERR(plainbuf))
+		return -PTR_ERR(plainbuf);
+	cipherbuf = kmalloc(USBIP_PACKETSIZE, GFP_KERNEL);
+	if (IS_ERR(cipherbuf)) {
+		kfree(plainbuf);
+		return -PTR_ERR(cipherbuf);
+	}
+
+	while (total < size) {
+		uint32_t packetsize;
+		struct kvec recvvec;
+
+		/*
+		 * We use a global kfifo to buffer unrequested plaintext bytes.
+		 * Flush this buffer first before receiving new data.
+		 */
+		if (kfifo_len(&ud->recv_queue)) {
+			size_t next = min_t(size_t, kfifo_len(&ud->recv_queue),
+					size - total);
+			/* No error checking necessary - see previous line */
+			ret = kfifo_out(&ud->recv_queue, ((char *)
+						vec[0].iov_base)+total, next);
+			total += next;
+			continue;
+		}
+
+		/* See usbip_sendmsg() for the format of one encrypted packet */
+
+		/*
+		 * Receive size of next crypto packet
+		 */
+		recvvec.iov_base = &packetsize;
+		recvvec.iov_len = sizeof(packetsize);
+
+		ret = kernel_recvmsg(ud->tcp_socket, msg, &recvvec, 1,
+				sizeof(packetsize), flags);
+		packetsize = be32_to_cpu(packetsize);
+		if (ret <= 0) {
+			total = ret;
+			goto err;
+		} else if (ret != sizeof(packetsize)) {
+			total = -EBADMSG;
+			goto err;
+		}
+
+		if (packetsize > USBIP_PACKETSIZE) {
+			total = -EBADMSG;
+			goto err;
+		}
+
+		/*
+		 * Receive the rest of the packet
+		 */
+		recvvec.iov_base = cipherbuf;
+		recvvec.iov_len = packetsize;
+		ret = kernel_recvmsg(ud->tcp_socket, msg, &recvvec, 1,
+				packetsize, flags);
+		if (ret <= 0) {
+			total = ret;
+			goto err;
+		} else if (ret != packetsize) {
+			total = -EBADMSG;
+			goto err;
+		}
+
+		/*
+		 * Decrypt the packet. This will also authenticate the length
+		 * field
+		 */
+		ret = usbip_crypt(ud, 0, packetsize, plainbuf, cipherbuf);
+		if (ret != 0) {
+			total = ret;
+			goto err;
+		}
+
+		/*
+		 * Add this packet to our global buffer (It will be stored in
+		 * the user buffer in the next loop iteration) No error
+		 * checking necessary - we already know the packet is going to
+		 * fit.
+		 */
+		(void) kfifo_in(&ud->recv_queue, plainbuf, packetsize -
+				USBIP_AUTHSIZE);
+	}
+
+err:
+	kfree(plainbuf);
+	kfree(cipherbuf);
+
+	return total;
 }
 EXPORT_SYMBOL_GPL(usbip_recvmsg);
 
 int usbip_sendmsg(struct usbip_device *ud, struct msghdr *msg, struct kvec
 		*vec, size_t num, size_t size)
 {
-	return kernel_sendmsg(ud->tcp_socket, msg, vec, num, size);
+	int i = 0, ret, offset = 0;
+	size_t total = 0;
+	unsigned char *cipherbuf;
+
+	/* If crypto is disabled, we just wrap the normal kernel calls. */
+	if (!ud->use_crypto)
+		return kernel_sendmsg(ud->tcp_socket, msg, vec, num, size);
+
+	cipherbuf = kmalloc(USBIP_PACKETSIZE, GFP_KERNEL);
+	if (IS_ERR(cipherbuf))
+		return -PTR_ERR(cipherbuf);
+
+	/*
+	 * The receiver has to decrypt whole packets. To avoid the need
+	 * to allocate large buffers at the receiving side, we split the
+	 * data to be sent in USBIP_PACKETSIZE large chunks that can be
+	 * decrypted separately. See below for the format of each chunk.
+	 */
+
+	/* Iterate over all kvecs, splitting them up as necessary. */
+	for (i = 0; i != num && size; ) {
+		/* Compute the remaining number of bytes to send for
+		 * this kvec */
+		const size_t PLAIN_SIZE = min_t(size_t, vec[i].iov_len -
+				offset, min_t(size_t, size, USBIP_PACKETSIZE -
+					USBIP_AUTHSIZE));
+		const size_t PACKET_SIZE = PLAIN_SIZE + USBIP_AUTHSIZE;
+		uint32_t packet_size_net = cpu_to_be32(PACKET_SIZE);
+		struct kvec sendvec[2];
+
+		if (PLAIN_SIZE == 0) {
+			++i;
+			offset = 0;
+			continue;
+		}
+
+		/*
+		 * One encrypted packet consists of:
+		 *  - An unencrypted, authenticated length tag (exactly 4
+		 *    bytes) containing the length of the packet.
+		 *  - Up to USBIP_PACKETSIZE - USBIP_AUTHSIZE bytes of user
+		 *    payload, encrypted
+		 *  - Exactly USBIP_AUTHSIZE bytes authentication tag.
+		 * Note: The packet length is also authenticated, but has
+		 * - for obvious reasons - to be sent in plaintext. This
+		 * packet format will be parsed by usbip_recvmsg (see above).
+		 */
+		ret = usbip_crypt(ud, 1, PACKET_SIZE, vec[i].iov_base + offset,
+				cipherbuf);
+		if (ret != 0) {
+			kfree(cipherbuf);
+			return ret;
+		}
+
+		/* Length field */
+		sendvec[0].iov_base = &packet_size_net;
+		sendvec[0].iov_len = sizeof(packet_size_net);
+		/* Payload and authentication tag */
+		sendvec[1].iov_base = cipherbuf;
+		sendvec[1].iov_len = PACKET_SIZE;
+		ret = kernel_sendmsg(ud->tcp_socket, msg, sendvec,
+				ARRAY_SIZE(sendvec), sendvec[0].iov_len +
+				sendvec[1].iov_len);
+		if (ret < 0) {
+			kfree(cipherbuf);
+			return ret;
+		}
+		if (ret != sendvec[0].iov_len + sendvec[1].iov_len) {
+			kfree(cipherbuf);
+			return -EPROTO;
+		}
+		offset += PLAIN_SIZE;
+		size -= PLAIN_SIZE;
+		total += PLAIN_SIZE;
+	}
+
+	kfree(cipherbuf);
+
+	return total;
 }
 EXPORT_SYMBOL_GPL(usbip_sendmsg);
 
diff --git a/drivers/staging/usbip/usbip_common.h b/drivers/staging/usbip/usbip_common.h
index 99f9dab..2b71c58 100644
--- a/drivers/staging/usbip/usbip_common.h
+++ b/drivers/staging/usbip/usbip_common.h
@@ -29,15 +29,28 @@
 #include <linux/types.h>
 #include <linux/usb.h>
 #include <linux/wait.h>
+#include <linux/kfifo.h>
 
 #define USBIP_VERSION "1.0.0"
 
 /*
+ * Length of the authentication tag associated with each packet, in bytes. Can
+ * be set to 4, 8, 12, 13, 14, 15 or 16. See crypto_gcm_setauthsize in
+ * crypto/gcm.c. Increasing this will increase crypto protocol overhead.
+ */
+#define USBIP_AUTHSIZE 4
+/*
  * Length of symmetric keys. Currently, this should be fixed at 16 bytes.
  * Will break code if changed, look at userspace and stub_dev.c/vhci_sysfs.c
  * where this constant is used before changing.
  */
 #define USBIP_KEYSIZE 16
+/*
+ * Maximum size of encrypted packets. Decreasing this will increase overhead
+ * and decrease memory usage.
+ */
+#define USBIP_PACKETSIZE 1024
+#define RECVQ_SIZE (2*USBIP_PACKETSIZE)
 
 #undef pr_fmt
 
@@ -300,6 +313,11 @@ struct usbip_device {
 
 	/* Crypto support */
 	int use_crypto;
+	struct crypto_aead *tfm_recv;
+	struct crypto_aead *tfm_send;
+	/* Counters to be used as IVs */
+	u64 ctr_send, ctr_recv;
+	DECLARE_KFIFO_PTR(recv_queue, char);
 };
 
 #define kthread_get_run(threadfn, data, namefmt, ...)			   \
@@ -323,6 +341,10 @@ struct usbip_device {
 void usbip_dump_urb(struct urb *purb);
 void usbip_dump_header(struct usbip_header *pdu);
 
+int usbip_init_crypto(struct usbip_device *ud, unsigned char *sendkey,
+		unsigned char *recvkey);
+void usbip_deinit_crypto(struct usbip_device *ud);
+
 int usbip_recv(struct usbip_device *ui, void *buf, int size);
 struct socket *sockfd_to_socket(unsigned int sockfd);
 
diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index d7974cb..0b72bcf 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -786,7 +786,9 @@ static void vhci_shutdown_connection(struct usbip_device *ud)
 		kthread_stop_put(vdev->ud.tcp_tx);
 		vdev->ud.tcp_tx = NULL;
 	}
-	pr_info("stop threads\n");
+	pr_info("stopped threads\n");
+
+	usbip_deinit_crypto(&vdev->ud);
 
 	/* active connection is closed */
 	if (vdev->ud.tcp_socket) {
diff --git a/drivers/staging/usbip/vhci_sysfs.c b/drivers/staging/usbip/vhci_sysfs.c
index 1ef3f25..ce46c16 100644
--- a/drivers/staging/usbip/vhci_sysfs.c
+++ b/drivers/staging/usbip/vhci_sysfs.c
@@ -20,6 +20,8 @@
 #include <linux/kthread.h>
 #include <linux/file.h>
 #include <linux/net.h>
+#include <linux/crypto.h>
+#include <linux/kfifo.h>
 
 #include "usbip_common.h"
 #include "vhci.h"
@@ -227,6 +229,13 @@ static ssize_t store_attach(struct device *dev, struct device_attribute *attr,
 	/* begin a lock */
 	spin_lock(&the_controller->lock);
 	vdev = port_to_vdev(rhport);
+	if (use_crypto) {
+		int ret = usbip_init_crypto(&vdev->ud, sendkey, recvkey);
+		if (ret < 0) {
+			spin_unlock(&the_controller->lock);
+			return ret;
+		}
+	}
 	spin_lock(&vdev->ud.lock);
 
 	if (vdev->ud.status != VDEV_ST_NULL) {
@@ -237,6 +246,7 @@ static ssize_t store_attach(struct device *dev, struct device_attribute *attr,
 		fput(socket->file);
 
 		dev_err(dev, "port %d already used\n", rhport);
+		usbip_deinit_crypto(&vdev->ud);
 		return -EINVAL;
 	}
 
-- 
1.8.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 6/7] staging: usbip: Update documentation
  2013-09-19 14:11 [PATCH 0/7] staging: usbip: Extend crypto support Dominik Paulus
                   ` (4 preceding siblings ...)
  2013-09-19 14:11 ` [PATCH 5/7] staging: usbip: Add encryption support to kernel Dominik Paulus
@ 2013-09-19 14:11 ` Dominik Paulus
  2013-09-19 14:11 ` [PATCH 7/7] staging: usbip: Increment version number to 1.2.1 Dominik Paulus
  6 siblings, 0 replies; 13+ messages in thread
From: Dominik Paulus @ 2013-09-19 14:11 UTC (permalink / raw)
  To: usbip-devel
  Cc: Tobias Polzer, Dominik Paulus, Greg Kroah-Hartman, Masanari Iida,
	Kurt Kanzenbach, Stefan Reif, Joe Perches, Bart Westgeest,
	Jake Champlin, Ilija Hadzic, Anthony Foiani, Bernard Blackham,
	Harvey Yang, linux-usb, devel, linux-kernel, linux-kernel

From: Tobias Polzer <tobias.polzer@fau.de>

README was updated and cleaned. It now contains just one example, which
was updated to use encryption. Also, the new crypto behaviour is
documented. The usbip "port" command has been removed from the README,
as it isn't supported by newer userland versions. One dead link was
removed from the checklist section.

Signed-off-by: Tobias Polzer <tobias.polzer@fau.de>
Signed-off-by: Dominik Paulus <dominik.paulus@fau.de>
---
 drivers/staging/usbip/userspace/README       | 72 +++++++++++-----------------
 drivers/staging/usbip/userspace/doc/usbipd.8 |  4 +-
 2 files changed, 31 insertions(+), 45 deletions(-)

diff --git a/drivers/staging/usbip/userspace/README b/drivers/staging/usbip/userspace/README
index 00a1658..01ea616 100644
--- a/drivers/staging/usbip/userspace/README
+++ b/drivers/staging/usbip/userspace/README
@@ -12,56 +12,53 @@
     - sysfsutils >= 2.0.0
 	sysfsutils library
 
-    - libwrap0-dev
+    - libwrap0-dev (optional)
 	tcp wrapper library
 
     - gcc >= 4.0
 
     - libtool, automake >= 1.9, autoconf >= 2.5.0, pkg-config
 
+    - libgnutls-dev >= 3.0 (libgnutls28-dev on debian) (optional)
+
 
 [Install]
     0. Generate configuration scripts.
 	$ ./autogen.sh
 
     1. Compile & install the userspace utilities.
-	$ ./configure [--with-tcp-wrappers=no] [--with-usbids-dir=<dir>]
+	$ ./configure [--with-tcp-wrappers=no] [--with-usbids-dir=<dir>] [--with-gnutls]
 	$ make install
 
     2. Compile & install USB/IP drivers.
 
 
 [Usage]
-    server:# (Physically attach your USB device.)
-
-    server:# insmod usbip-core.ko
-    server:# insmod usbip-host.ko
-
-    server:# usbipd -D
-	- Start usbip daemon.
-
-    server:# usbip list -l
-	- List driver assignments for USB devices.
 
-    server:# usbip bind --busid 1-2
-	- Bind usbip-host.ko to the device with busid 1-2.
-	- The USB device 1-2 is now exportable to other hosts!
-	- Use `usbip unbind --busid 1-2' to stop exporting the device.
+See usbip(8) and usbipd(8).
 
-    client:# insmod usbip-core.ko
-    client:# insmod vhci-hcd.ko
+[Security considerations]
+By default, all of the usbip network traffic is unencrypted and
+unauthenticated. As it is mostly parsed in staging quality kernel code, you
+should use usbip in this mode only in absolutely trusted environments.
 
-    client:# usbip list --remote <host>
-	- List exported USB devices on the <host>.
+In addition to the usual methods for secure network tunneling - SSH tunnels,
+IPsec, etc. -, usbip version 1.2.1 supports out-of-the-box crypto for all of
+the network traffic. This requires usbip to be compiled with GnuTLS enabled
+(configure switch: --with-gnutls). Crypto support can be enabled by using
+password authentication. If the --auth flag is set, usbip will not only
+authenticate client and server using the shared passphrase, but also encrypt
+and authenticate all of the following traffic. For the userspace traffic,
+GnuTLS is used, the kernel traffic is encrypted and authenticated using AES-GCM
+with 128bit keys. The session keys are randomly generated and exchanged for
+in userspace for each connection.
 
-    client:# usbip attach --remote <host> --busid 1-2
-	- Connect the remote USB device.
-
-    client:# usbip port
-	- Show virtual port status.
-
-    client:# usbip detach --port <port>
-	- Detach the USB device.
+The encryption support has been designed to offer perfect forward secrecy and
+decent security even if using rather weak passwords. Strong passwords are
+mainly needed to provide proper authorization (this is still important, see
+above!) and to secure against man-in-the-middle attacks.
+However, the crypto code still lacks complete review and code auditing. Do not
+rely on it for strong security.
 
 
 [Example]
@@ -72,7 +69,7 @@ Physically attach your USB devices to this host.
 
     trois:# insmod path/to/usbip-core.ko
     trois:# insmod path/to/usbip-host.ko
-    trois:# usbipd -D
+    trois:# usbipd -sVerySecret -D
 
 In another terminal, let's look up what USB devices are physically
 attached to this host.
@@ -135,7 +132,7 @@ exportable on the host.
     deux:# insmod path/to/usbip-core.ko
     deux:# insmod path/to/vhci-hcd.ko
 
-    deux:# usbip list --remote 10.0.0.3
+    deux:# usbip --auth VerySecret list --remote 10.0.0.3
     Exportable USB devices
     ======================
      - 10.0.0.3
@@ -163,20 +160,9 @@ exportable on the host.
 
 Attach a remote USB device:
 
-    deux:# usbip attach --remote 10.0.0.3 --busid 1-1
+    deux:# usbip --auth VerySecret attach --remote 10.0.0.3 --busid 1-1
     port 0 attached
 
-Show the devices attached to this client:
-
-    deux:# usbip port
-    Port 00: <Port in Use> at Full Speed(12Mbps)
-	   Prolific Technology, Inc. : unknown product (067b:3507)
-	   6-1 -> usbip://10.0.0.3:3240/1-1  (remote bus/dev 001/004)
-	   6-1:1.0 used by usb-storage
-			  /sys/class/scsi_device/0:0:0:0/device
-			  /sys/class/scsi_host/host0/device
-			  /sys/block/sda/device
-
 Detach the imported device:
 
     deux:# usbip detach --port 0
@@ -184,8 +170,6 @@ Detach the imported device:
 
 
 [Checklist]
-    - See 'Debug Tips' on the project wiki.
-	- http://usbip.wiki.sourceforge.net/how-to-debug-usbip
     - usbip-host.ko must be bound to the target device.
 	- See /proc/bus/usb/devices and find "Driver=..." lines of the device.
     - Shutdown firewall.
diff --git a/drivers/staging/usbip/userspace/doc/usbipd.8 b/drivers/staging/usbip/userspace/doc/usbipd.8
index 887a1b5..25f09bf 100644
--- a/drivers/staging/usbip/userspace/doc/usbipd.8
+++ b/drivers/staging/usbip/userspace/doc/usbipd.8
@@ -32,7 +32,9 @@ Print debugging information.
 .HP
 \fB\-s\fR, \fB\-\-auth\fR
 .IP
-Sets the password to be used for client authentication. If -a is used, the server will only accept connections from authenticated clients. Note: USB traffic will still be unencrypted, this currently only serves for authentication.
+Sets the password to be used for client authentication. If -a is used, the
+server will only accept connections from authenticated clients. This will also
+encrypt the whole usbip traffic, including kernel traffic, using 128bit AES.
 .PP
 
 \fB\-h\fR, \fB\-\-help\fR
-- 
1.8.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 7/7] staging: usbip: Increment version number to 1.2.1
  2013-09-19 14:11 [PATCH 0/7] staging: usbip: Extend crypto support Dominik Paulus
                   ` (5 preceding siblings ...)
  2013-09-19 14:11 ` [PATCH 6/7] staging: usbip: Update documentation Dominik Paulus
@ 2013-09-19 14:11 ` Dominik Paulus
  6 siblings, 0 replies; 13+ messages in thread
From: Dominik Paulus @ 2013-09-19 14:11 UTC (permalink / raw)
  To: usbip-devel
  Cc: Dominik Paulus, Tobias Polzer, Greg Kroah-Hartman, Masanari Iida,
	Kurt Kanzenbach, Stefan Reif, Joe Perches, Bart Westgeest,
	Jake Champlin, Ilija Hadzic, Anthony Foiani, Bernard Blackham,
	Harvey Yang, linux-usb, devel, linux-kernel, linux-kernel

Also increment the kernel module version number to match the
userspace version, as compatibility with old userspace utilities
is now at least partially broken.

Signed-off-by: Dominik Paulus <dominik.paulus@fau.de>
Signed-off-by: Tobias Polzer <tobias.polzer@fau.de>
---
 drivers/staging/usbip/usbip_common.h         | 2 +-
 drivers/staging/usbip/userspace/configure.ac | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/usbip/usbip_common.h b/drivers/staging/usbip/usbip_common.h
index 2b71c58..82a3e12 100644
--- a/drivers/staging/usbip/usbip_common.h
+++ b/drivers/staging/usbip/usbip_common.h
@@ -31,7 +31,7 @@
 #include <linux/wait.h>
 #include <linux/kfifo.h>
 
-#define USBIP_VERSION "1.0.0"
+#define USBIP_VERSION "1.2.1"
 
 /*
  * Length of the authentication tag associated with each packet, in bytes. Can
diff --git a/drivers/staging/usbip/userspace/configure.ac b/drivers/staging/usbip/userspace/configure.ac
index a1a1267..26dabe3 100644
--- a/drivers/staging/usbip/userspace/configure.ac
+++ b/drivers/staging/usbip/userspace/configure.ac
@@ -1,7 +1,7 @@
 dnl Process this file with autoconf to produce a configure script.
 
 AC_PREREQ(2.59)
-AC_INIT([usbip-utils], [1.2.0], [linux-usb@vger.kernel.org])
+AC_INIT([usbip-utils], [1.2.1], [linux-usb@vger.kernel.org])
 
 CURRENT=0
 REVISION=1
-- 
1.8.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/7] staging: usbip: Add encryption support to kernel
  2013-09-19 14:11 ` [PATCH 5/7] staging: usbip: Add encryption support to kernel Dominik Paulus
@ 2013-09-23  9:59   ` Dan Carpenter
  2013-09-26 10:18     ` Dominik Paulus
  2013-09-23 10:35   ` Dan Carpenter
  2013-09-23 10:58   ` Dan Carpenter
  2 siblings, 1 reply; 13+ messages in thread
From: Dan Carpenter @ 2013-09-23  9:59 UTC (permalink / raw)
  To: Dominik Paulus
  Cc: usbip-devel, Anthony Foiani, devel, linux-kernel,
	Greg Kroah-Hartman, linux-usb, Kurt Kanzenbach, Tobias Polzer,
	Harvey Yang, linux-kernel, Ilija Hadzic, Bart Westgeest,
	Joe Perches, Jake Champlin, Stefan Reif, Bernard Blackham

On Thu, Sep 19, 2013 at 04:11:57PM +0200, Dominik Paulus wrote:
> +int usbip_init_crypto(struct usbip_device *ud, unsigned char *sendkey, unsigned
> +		char *recvkey)
> +{
> +	int ret;
> +
> +	ud->use_crypto = 1;
> +
> +	ud->tfm_recv = crypto_alloc_aead("gcm(aes)", 0, 0);
> +	if (IS_ERR(ud->tfm_recv))
> +		return -PTR_ERR(ud->tfm_recv);

PTR_ERR() already returns a negative.

> +	ud->tfm_send = crypto_alloc_aead("gcm(aes)", 0, 0);
> +	if (IS_ERR(ud->tfm_send)) {
> +		crypto_free_aead(ud->tfm_recv);
> +		return -PTR_ERR(ud->tfm_send);

Again.  All the uses of PTR_ERR() in this patch have the same problem.

> +	plainbuf = kmalloc(USBIP_PACKETSIZE, GFP_KERNEL);
> +	if (IS_ERR(plainbuf))
> +		return -PTR_ERR(plainbuf);

kmalloc() returns NULL on error and not an ERR_PTR.  All the calls to
kmalloc() have this problem.

> +	cipherbuf = kmalloc(USBIP_PACKETSIZE, GFP_KERNEL);
> +	if (IS_ERR(cipherbuf)) {
> +		kfree(plainbuf);
> +		return -PTR_ERR(cipherbuf);
> +	}
> +
> +	while (total < size) {
> +		uint32_t packetsize;
> +		struct kvec recvvec;
> +
> +		/*
> +		 * We use a global kfifo to buffer unrequested plaintext bytes.
> +		 * Flush this buffer first before receiving new data.
> +		 */
> +		if (kfifo_len(&ud->recv_queue)) {
> +			size_t next = min_t(size_t, kfifo_len(&ud->recv_queue),
> +					size - total);
> +			/* No error checking necessary - see previous line */
> +			ret = kfifo_out(&ud->recv_queue, ((char *)
> +						vec[0].iov_base)+total, next);


The comment assume there is only one reader and one writer at a time,
yes?  The casting is not needed:

			ret = kfifo_out(&ud->recv_queue,
					vec[0].iov_base + total, next);


v> +			total += next;
> +			continue;
> +		}
> +
> +		/* See usbip_sendmsg() for the format of one encrypted packet */
> +
> +		/*
> +		 * Receive size of next crypto packet
> +		 */
> +		recvvec.iov_base = &packetsize;
> +		recvvec.iov_len = sizeof(packetsize);
> +
> +		ret = kernel_recvmsg(ud->tcp_socket, msg, &recvvec, 1,
> +				sizeof(packetsize), flags);
> +		packetsize = be32_to_cpu(packetsize);
> +		if (ret <= 0) {

I think a return of zero should mean total = -EBADMSG;.  In other words
this check should be "if (ret < 0) {" and we hit the next else if.
Same below again.

> +			total = ret;
> +			goto err;
> +		} else if (ret != sizeof(packetsize)) {
> +			total = -EBADMSG;
> +			goto err;
> +		}
> +
> +		if (packetsize > USBIP_PACKETSIZE) {
> +			total = -EBADMSG;
> +			goto err;
> +		}
> +

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/7] staging: usbip: Add encryption support to kernel
  2013-09-19 14:11 ` [PATCH 5/7] staging: usbip: Add encryption support to kernel Dominik Paulus
  2013-09-23  9:59   ` Dan Carpenter
@ 2013-09-23 10:35   ` Dan Carpenter
  2013-09-23 10:58   ` Dan Carpenter
  2 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2013-09-23 10:35 UTC (permalink / raw)
  To: Dominik Paulus
  Cc: usbip-devel, Anthony Foiani, devel, linux-kernel,
	Greg Kroah-Hartman, linux-usb, Kurt Kanzenbach, Tobias Polzer,
	Harvey Yang, linux-kernel, Ilija Hadzic, Bart Westgeest,
	Joe Perches, Jake Champlin, Stefan Reif, Bernard Blackham

On Thu, Sep 19, 2013 at 04:11:57PM +0200, Dominik Paulus wrote:
> +int usbip_init_crypto(struct usbip_device *ud, unsigned char *sendkey, unsigned
> +		char *recvkey)
> +{
> +	int ret;
> +
> +	ud->use_crypto = 1;
> +
> +	ud->tfm_recv = crypto_alloc_aead("gcm(aes)", 0, 0);
> +	if (IS_ERR(ud->tfm_recv))
> +		return -PTR_ERR(ud->tfm_recv);
> +	ud->tfm_send = crypto_alloc_aead("gcm(aes)", 0, 0);
> +	if (IS_ERR(ud->tfm_send)) {
> +		crypto_free_aead(ud->tfm_recv);
> +		return -PTR_ERR(ud->tfm_send);
> +	}
> +	ret = kfifo_alloc(&ud->recv_queue, RECVQ_SIZE, GFP_KERNEL);
> +	if (ret) {
> +		crypto_free_aead(ud->tfm_recv);
> +		crypto_free_aead(ud->tfm_send);
> +		return ret;
> +	}
> +
> +	if (crypto_aead_setkey(ud->tfm_send, sendkey, USBIP_KEYSIZE) != 0 ||
> +			crypto_aead_setkey(ud->tfm_recv, recvkey,
> +				USBIP_KEYSIZE) != 0 ||
> +			crypto_aead_setauthsize(ud->tfm_send,
> +				USBIP_AUTHSIZE) != 0 ||
> +			crypto_aead_setauthsize(ud->tfm_recv,
> +				USBIP_AUTHSIZE)) {
> +		crypto_free_aead(ud->tfm_recv);
> +		crypto_free_aead(ud->tfm_send);
> +		kfifo_free(&ud->recv_queue);
> +	}

This returns success on error instead of failure.

The indenting is messed up.  There are three places which check " != 0"
and doesn't.  Please leave off the "!= 0" throughout the whole patch.
It should look like:

	if (crypto_aead_setkey(ud->tfm_send, sendkey, USBIP_KEYSIZE) ||
	    crypto_aead_setkey(ud->tfm_recv, recvkey, USBIP_KEYSIZE) ||
	    crypto_aead_setauthsize(ud->tfm_send, USBIP_AUTHSIZE) ||
	    crypto_aead_setauthsize(ud->tfm_recv, USBIP_AUTHSIZE)) {
	    	ret = -EINVAL;
		goto err_free_fifo;
	}

Notice how the label name is chosen based on the label location and not
the goto location.

The end of the function should look like:

	return 0;

err_free_fifo:
	kfifo_free(&ud->recv_queue);
err_free_send:
	crypto_free_aead(ud->tfm_send);
err_free_recv:
	crypto_free_aead(ud->tfm_recv);

	return ret;

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/7] staging: usbip: Add encryption support to kernel
  2013-09-19 14:11 ` [PATCH 5/7] staging: usbip: Add encryption support to kernel Dominik Paulus
  2013-09-23  9:59   ` Dan Carpenter
  2013-09-23 10:35   ` Dan Carpenter
@ 2013-09-23 10:58   ` Dan Carpenter
  2 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2013-09-23 10:58 UTC (permalink / raw)
  To: Dominik Paulus
  Cc: usbip-devel, Anthony Foiani, devel, linux-kernel,
	Greg Kroah-Hartman, linux-usb, Kurt Kanzenbach, Tobias Polzer,
	Harvey Yang, linux-kernel, Ilija Hadzic, Bart Westgeest,
	Joe Perches, Jake Champlin, Stefan Reif, Bernard Blackham

On Thu, Sep 19, 2013 at 04:11:57PM +0200, Dominik Paulus wrote:
> +/*
> + * Perform encryption/decryption on one chunk of data.
> + * Uses global crypto state stored in usbip_device.
> + * Parameters:
> + * encrypt: 1 to perform encryption, 0 to perform decryption operation

Make this a define:
#define USBIP_ENCRYPT 1
#define USBIP_ENCRYPT 0

> + * packetsize: Size of the encrypted packet, including the authentication tag,
> + * not including the associated data (length field).
> + * plaintext and ciphertext have to be appropiately managed by the caller
> + * (i.e. they must be at least packetsize bytes long).
> + * Returns: 0 on success
> + */
> +static int usbip_crypt(struct usbip_device *ud, int encrypt, uint32_t
> +		packetsize, unsigned char *plaintext, unsigned char
> +		*ciphertext)

Don't break put line breaks between the type and the variable name.  It
should be:

static int usbip_crypt(struct usbip_device *ud, int encrypt,
		uint32_t packetsize, unsigned char *plaintext,
		unsigned char *ciphertext)

This applies to earlier patches in this series as well.

> +{
> +	struct crypto_aead *tfm;
> +	struct aead_request *req;
> +	struct tcrypt_result result;
> +	struct scatterlist plain, cipher, assoc;
> +	char iv[16];
> +	u64 *iv_num;
> +	u64 iv_net;
> +	const int plainsize = packetsize - USBIP_AUTHSIZE;

Is it possible that packetsize is less than USBIP_AUTHSIZE?

> +	int ret;
> +
> +	memset(iv, 0, sizeof(iv));
> +	if (encrypt) {
> +		tfm = ud->tfm_send;
> +		iv_num = &ud->ctr_send;
> +	} else {
> +		tfm = ud->tfm_recv;
> +		iv_num = &ud->ctr_recv;
> +	}
> +	iv_net = cpu_to_be64(*iv_num);
> +	memcpy(iv, &iv_net, sizeof(iv_net));
> +
> +	req = aead_request_alloc(tfm, GFP_KERNEL);
> +	if (IS_ERR(req))
> +		return -PTR_ERR(req);
> +
> +	init_completion(&result.completion);
> +	aead_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> +			tcrypt_complete, &result);

Align this up like:

	aead_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
				  tcrypt_complete, &result);


> +
> +	sg_init_one(&cipher, ciphertext, packetsize);
> +	sg_init_one(&plain, plaintext, plainsize);
> +	crypto_aead_clear_flags(tfm, ~0);
> +
> +	if (encrypt)
> +		aead_request_set_crypt(req, &plain, &cipher, plainsize, iv);
> +	else
> +		aead_request_set_crypt(req, &cipher, &plain, packetsize, iv);
> +	packetsize = cpu_to_be32(packetsize);
> +	sg_init_one(&assoc, &packetsize, sizeof(packetsize));
> +	/* Associated data: Unencrypted length tag */
> +	aead_request_set_assoc(req, &assoc, sizeof(packetsize));
> +
> +	if (encrypt)
> +		ret = crypto_aead_encrypt(req);
> +	else
> +		ret = crypto_aead_decrypt(req);
> +

Good on you for figuring out what crypto_aead_en/decrypt() returns.
Where are these functions documented?

> +	switch (ret) {
> +	case 0: /* Success */
> +		break;
> +	case -EINPROGRESS:
> +	case -EBUSY:
> +		wait_for_completion(&result.completion);
> +		break;
> +	default:
> +		aead_request_free(req);
> +		return ret;
> +	}
> +

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/7] staging: usbip: Add encryption support to kernel
  2013-09-23  9:59   ` Dan Carpenter
@ 2013-09-26 10:18     ` Dominik Paulus
  2013-09-26 11:48       ` Dan Carpenter
  0 siblings, 1 reply; 13+ messages in thread
From: Dominik Paulus @ 2013-09-26 10:18 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Dominik Paulus, usbip-devel, Anthony Foiani, devel, linux-kernel,
	Greg Kroah-Hartman, linux-usb, Kurt Kanzenbach, Tobias Polzer,
	Harvey Yang, linux-kernel, Ilija Hadzic, Bart Westgeest,
	Joe Perches, Jake Champlin, Stefan Reif, Bernard Blackham

Hi,

thank you very much for your feedback!

On Mon, Sep 23, 2013 at 12:59:29PM +0300, Dan Carpenter wrote:
> > +	while (total < size) {
> > +		uint32_t packetsize;
> > +		struct kvec recvvec;
> > +
> > +		/*
> > +		 * We use a global kfifo to buffer unrequested plaintext bytes.
> > +		 * Flush this buffer first before receiving new data.
> > +		 */
> > +		if (kfifo_len(&ud->recv_queue)) {
> > +			size_t next = min_t(size_t, kfifo_len(&ud->recv_queue),
> > +					size - total);
> > +			/* No error checking necessary - see previous line */
> > +			ret = kfifo_out(&ud->recv_queue, ((char *)
> > +						vec[0].iov_base)+total, next);
> 
> 
> The comment assume there is only one reader and one writer at a time,
> yes?  The casting is not needed:

Actually, we have not only one reader and one writer, but exactly one
thread doing all the work on the fifo. No parallel access is done at
all.

> 			ret = kfifo_out(&ud->recv_queue,
> 					vec[0].iov_base + total, next);
> 
> 
> v> +			total += next;
> > +			continue;
> > +		}
> > +
> > +		/* See usbip_sendmsg() for the format of one encrypted packet */
> > +
> > +		/*
> > +		 * Receive size of next crypto packet
> > +		 */
> > +		recvvec.iov_base = &packetsize;
> > +		recvvec.iov_len = sizeof(packetsize);
> > +
> > +		ret = kernel_recvmsg(ud->tcp_socket, msg, &recvvec, 1,
> > +				sizeof(packetsize), flags);
> > +		packetsize = be32_to_cpu(packetsize);
> > +		if (ret <= 0) {
> 
> I think a return of zero should mean total = -EBADMSG;.  In other words
> this check should be "if (ret < 0) {" and we hit the next else if.
> Same below again.

As we are wrapping kernel_recvmsg here, we wanted to leave the semantics
intact as far as possible. The calling code already checks for the correct
size.

> > +			total = ret;
> > +			goto err;
> > +		} else if (ret != sizeof(packetsize)) {
> > +			total = -EBADMSG;
> > +			goto err;
> > +		}

On Mon, Sep 23, 2013 at 01:35:04PM +0300, Dan Carpenter wrote:
> On Thu, Sep 19, 2013 at 04:11:57PM +0200, Dominik Paulus wrote:
> > +	if (crypto_aead_setkey(ud->tfm_send, sendkey, USBIP_KEYSIZE) != 0 ||
> > +			crypto_aead_setkey(ud->tfm_recv, recvkey,
> > +				USBIP_KEYSIZE) != 0 ||
> > +			crypto_aead_setauthsize(ud->tfm_send,
> > +				USBIP_AUTHSIZE) != 0 ||
> > +			crypto_aead_setauthsize(ud->tfm_recv,
> > +				USBIP_AUTHSIZE)) {
> > +		crypto_free_aead(ud->tfm_recv);
> > +		crypto_free_aead(ud->tfm_send);
> > +		kfifo_free(&ud->recv_queue);
> > +	}
> 
> This returns success on error instead of failure.

Indeed, that was horribly broken. Thanks.

On Mon, Sep 23, 2013 at 01:58:42PM +0300, Dan Carpenter wrote:
> On Thu, Sep 19, 2013 at 04:11:57PM +0200, Dominik Paulus wrote:
> > +{
> > +	struct crypto_aead *tfm;
> > +	struct aead_request *req;
> > +	struct tcrypt_result result;
> > +	struct scatterlist plain, cipher, assoc;
> > +	char iv[16];
> > +	u64 *iv_num;
> > +	u64 iv_net;
> > +	const int plainsize = packetsize - USBIP_AUTHSIZE;
> 
> Is it possible that packetsize is less than USBIP_AUTHSIZE?

No, currently, the caller (usbip_sendmsg() / usbip_recvmsg() are the
only functions calling usbip_crypt(), which itself is static) ensures
this.
Admittedly, this isn't great design. We added a check for packetsize <
USBIP_AUTHSIZE and an appropiate return here.

> > +	if (encrypt)
> > +		ret = crypto_aead_encrypt(req);
> > +	else
> > +		ret = crypto_aead_decrypt(req);
> > +
> 
> Good on you for figuring out what crypto_aead_en/decrypt() returns.
> Where are these functions documented?
> 
> > +        switch (ret) {
> > +        case 0: /* Success */
> > +                break;
> > +        case -EINPROGRESS:
> > +        case -EBUSY:
> > +                wait_for_completion(&result.completion);
> > +                break;
> > +        default:
> > +                aead_request_free(req);
> > +                return ret;
> > +        }
> > +

They aren't, actually. Documentation/crypto/api-intro.txt refers to the
regression test module, which uses exactly those return-values in
crypto/testmgr.c.
We noticed that wait_for_completion might not be the best idea, since it could
hang indefinitely, testmgr.c uses wait_for_completion_interruptible. Do we
want 'interruptible' or 'killable' here?
(Also we forgot to error-check result.err afterwards. We also fixed that.)

Regards,
Dominik Paulus, Tobias Polzer

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/7] staging: usbip: Add encryption support to kernel
  2013-09-26 10:18     ` Dominik Paulus
@ 2013-09-26 11:48       ` Dan Carpenter
  0 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2013-09-26 11:48 UTC (permalink / raw)
  To: Dominik Paulus
  Cc: Anthony Foiani, devel, linux-usb, linux-kernel,
	Greg Kroah-Hartman, usbip-devel, Kurt Kanzenbach, Tobias Polzer,
	Harvey Yang, linux-kernel, Joe Perches, Bart Westgeest,
	Dominik Paulus, Ilija Hadzic, Jake Champlin, Stefan Reif,
	Bernard Blackham

On Thu, Sep 26, 2013 at 12:18:34PM +0200, Dominik Paulus wrote:
> > I think a return of zero should mean total = -EBADMSG;.  In other words
> > this check should be "if (ret < 0) {" and we hit the next else if.
> > Same below again.
> 
> As we are wrapping kernel_recvmsg here, we wanted to leave the semantics
> intact as far as possible. The calling code already checks for the correct
> size.

Hm...  Ok.  Sometimes zero is interpretted as a connection closed and
sometimes reading less than expected is considered a TCP error.

> No, currently, the caller (usbip_sendmsg() / usbip_recvmsg() are the
> only functions calling usbip_crypt(), which itself is static) ensures
> this.
> Admittedly, this isn't great design. We added a check for packetsize <
> USBIP_AUTHSIZE and an appropiate return here.
> 
> > > +	if (encrypt)
> > > +		ret = crypto_aead_encrypt(req);
> > > +	else
> > > +		ret = crypto_aead_decrypt(req);
> > > +
> > 
> > Good on you for figuring out what crypto_aead_en/decrypt() returns.
> > Where are these functions documented?
> > 
> > > +        switch (ret) {
> > > +        case 0: /* Success */
> > > +                break;
> > > +        case -EINPROGRESS:
> > > +        case -EBUSY:
> > > +                wait_for_completion(&result.completion);
> > > +                break;
> > > +        default:
> > > +                aead_request_free(req);
> > > +                return ret;
> > > +        }
> > > +
> 
> They aren't, actually. Documentation/crypto/api-intro.txt refers to the
> regression test module, which uses exactly those return-values in
> crypto/testmgr.c.

Well that sucks.

> We noticed that wait_for_completion might not be the best idea, since it could
> hang indefinitely, testmgr.c uses wait_for_completion_interruptible. Do we
> want 'interruptible' or 'killable' here?

I think you want the interruptible one wait_for_completion_interruptible()

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2013-09-26 11:49 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-19 14:11 [PATCH 0/7] staging: usbip: Extend crypto support Dominik Paulus
2013-09-19 14:11 ` [PATCH 1/7] staging: usbip: TLS for all userspace communication Dominik Paulus
2013-09-19 14:11 ` [PATCH 2/7] staging: usbip: Exchange session keys in userspace Dominik Paulus
2013-09-19 14:11 ` [PATCH 3/7] staging: usbip: Pass session keys to the kernel Dominik Paulus
2013-09-19 14:11 ` [PATCH 4/7] staging: usbip: Wrap kernel_sendmsg()/recvmsg() Dominik Paulus
2013-09-19 14:11 ` [PATCH 5/7] staging: usbip: Add encryption support to kernel Dominik Paulus
2013-09-23  9:59   ` Dan Carpenter
2013-09-26 10:18     ` Dominik Paulus
2013-09-26 11:48       ` Dan Carpenter
2013-09-23 10:35   ` Dan Carpenter
2013-09-23 10:58   ` Dan Carpenter
2013-09-19 14:11 ` [PATCH 6/7] staging: usbip: Update documentation Dominik Paulus
2013-09-19 14:11 ` [PATCH 7/7] staging: usbip: Increment version number to 1.2.1 Dominik Paulus

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox