Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 3 Feb 2018 00:30:44 -0800
From:   Eric Biggers <ebiggers3@gmail.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        Secunia Research <vuln@secunia.com>,
        Shuah Khan <shuahkh@osg.samsung.com>
Subject: Re: [PATCH 4.4 02/74] usbip: prevent vhci_hcd driver from leaking a
 socket pointer address
Message-ID: <20180203083044.GA6126@zzz.localdomain>
References: <20180129123847.507563674@linuxfoundation.org>
 <20180129123847.617716392@linuxfoundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180129123847.617716392@linuxfoundation.org>
User-Agent: Mutt/1.9.3 (2018-01-21)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jan 29, 2018 at 01:56:07PM +0100, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Shuah Khan <shuahkh@osg.samsung.com>
> 
> commit 2f2d0088eb93db5c649d2a5e34a3800a8a935fc5 upstream.
> 
> When a client has a USB device attached over IP, the vhci_hcd driver is
> locally leaking a socket pointer address via the
> 
> /sys/devices/platform/vhci_hcd/status file (world-readable) and in debug
> output when "usbip --debug port" is run.
> 
> Fix it to not leak. The socket pointer address is not used at the moment
> and it was made visible as a convenient way to find IP address from socket
> pointer address by looking up /proc/net/{tcp,tcp6}.
> 
> As this opens a security hole, the fix replaces socket pointer address with
> sockfd.
> 
> Reported-by: Secunia Research <vuln@secunia.com>
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> 
> ---
>  drivers/usb/usbip/usbip_common.h     |    1 +
>  drivers/usb/usbip/vhci_sysfs.c       |   25 +++++++++++++++----------
>  tools/usb/usbip/libsrc/vhci_driver.c |    8 ++++----
>  3 files changed, 20 insertions(+), 14 deletions(-)
> 
> --- a/drivers/usb/usbip/usbip_common.h
> +++ b/drivers/usb/usbip/usbip_common.h
> @@ -261,6 +261,7 @@ struct usbip_device {
>  	/* lock for status */
>  	spinlock_t lock;
>  
> +	int sockfd;
>  	struct socket *tcp_socket;
>  
>  	struct task_struct *tcp_rx;
> --- a/drivers/usb/usbip/vhci_sysfs.c
> +++ b/drivers/usb/usbip/vhci_sysfs.c
> @@ -39,16 +39,20 @@ static ssize_t status_show(struct device
>  
>  	/*
>  	 * output example:
> -	 * prt sta spd dev socket           local_busid
> -	 * 000 004 000 000         c5a7bb80 1-2.3
> -	 * 001 004 000 000         d8cee980 2-3.4
> +	 * port sta spd dev      sockfd local_busid
> +	 * 0000 004 000 00000000 000003 1-2.3
> +	 * 0001 004 000 00000000 000004 2-3.4
>  	 *
> -	 * IP address can be retrieved from a socket pointer address by looking
> -	 * up /proc/net/{tcp,tcp6}. Also, a userland program may remember a
> -	 * port number and its peer IP address.
> +	 * Output includes socket fd instead of socket pointer address to
> +	 * avoid leaking kernel memory address in:
> +	 *	/sys/devices/platform/vhci_hcd.0/status and in debug output.
> +	 * The socket pointer address is not used at the moment and it was
> +	 * made visible as a convenient way to find IP address from socket
> +	 * pointer address by looking up /proc/net/{tcp,tcp6}. As this opens
> +	 * a security hole, the change is made to use sockfd instead.
>  	 */
>  	out += sprintf(out,
> -		       "prt sta spd bus dev socket           local_busid\n");
> +		       "prt sta spd bus dev sockfd local_busid\n");
>  
>  	for (i = 0; i < VHCI_NPORTS; i++) {
>  		struct vhci_device *vdev = port_to_vdev(i);
> @@ -60,11 +64,11 @@ static ssize_t status_show(struct device
>  			out += sprintf(out, "%03u %08x ",
>  				       vdev->speed, vdev->devid);
>  			out += sprintf(out, "%16p ", vdev->ud.tcp_socket);
> +			out += sprintf(out, "%06u", vdev->ud.sockfd);
>  			out += sprintf(out, "%s", dev_name(&vdev->udev->dev));

This backport is wrong; it's still printing the pointer...

Eric