linux-kernel - Re: [PATCHv4 10/16] staging: usbip: TLS for all userspace communication

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131025144549.GG5871@mwanda>
Date:	Fri, 25 Oct 2013 17:45:50 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Dominik Paulus <dominik.paulus@....de>
Cc:	Anthony Foiani <anthony.foiani@...il.com>,
	devel@...verdev.osuosl.org, linux-kernel@...cs.fau.de,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Kurt Kanzenbach <ly80toro@....cs.fau.de>, tobias.polzer@....de,
	linux-kernel@...r.kernel.org,
	Ilija Hadzic <ihadzic@...earch.bell-labs.com>
Subject: Re: [PATCHv4 10/16] staging: usbip: TLS for all userspace
 communication

On Sat, Oct 19, 2013 at 04:39:13PM +0200, Dominik Paulus wrote:
> @@ -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) {

Don't put a blank line between the function call and the check.  They
logically are one idea.

>  
> -	rc = usbip_net_recv(sockfd, (void *) &reply, sizeof(reply));
> +	rc = usbip_net_recv(conn, (void *) &reply, sizeof(reply));

There is no need to cast to void here, btw.  That's just noise.

>  	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

This is messy and I feel like it should be abstracted into a function
so we can hide the ifdef in a header file.

		if (sending)
			nbytes = usbip_send(conn, buff, bufflen, 0);
		else
			nbytes = usbip_recv(...

We'd still have the ifdef but hidden away.


> +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;
> +

Kernel style is more beautiful:

	ret = gnutls_credentials_set(conn->session, GNUTLS_CRD_SRP,
				     usbip_net_srp_cred);
	if (ret)
		return ret;

> +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;

What is this about?  I assume that GCC warns, but which version of GCC
are you using because that sounds horrible.

> +#endif

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/