linux-kernel - RE: [PATCH] usb: gadget: f_fs: report error if excess data received

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]

Date:	Thu, 12 May 2016 09:39:52 +0000
From:	"Du, Changbin" <changbin.du@...el.com>
To:	Felipe Balbi <felipe.balbi@...ux.intel.com>,
	Al Viro <viro@...iv.linux.org.uk>
CC:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"mina86@...a86.com" <mina86@...a86.com>,
	"rui.silva@...aro.org" <rui.silva@...aro.org>,
	"k.opasiak@...sung.com" <k.opasiak@...sung.com>,
	"lars@...afoo.de" <lars@...afoo.de>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] usb: gadget: f_fs: report error if excess data received

> >> > These all can lead host send more than device wanted bytes. For sure
> >> > it wrong at host side, but device side don't know.
> >>
> >> but none of this means we have a bug at device side. In fact, by
> >> allowing these extra bytes to reach userspace, we could be creating a
> >> possible attack vector.
> >>
> >> Your explanation is unsatisfactory, so I won't apply your patch, sorry.
> >>
> >> --
> >> balbi
> > It is fine. Then need userspace take care of all the data it received. Because
> > Kernel may drop some data for it. Kernel ffs driver is unauthentic
> sometimes.
> 
> I really cannot understand what you mean sometimes. You're saying that
> userspace needs to take care of all the data it received because kernel
> can drop data. If kernel is dropping data, there's no extra data
> reaching userspace, right?
> 
For sure, maybe I didn't describe it well so let you confused. :)

> Is the problem that we *are* giving more data than expected to
> userspace? Are we overflowing some userspace buffer? If that's the case,
> then below should be enough for the time being:
> 
No, the problem is we drop data but silently. We cannot give more data to
userspace since buffer is limited.

> diff --git a/drivers/usb/gadget/function/f_fs.c
> b/drivers/usb/gadget/function/f_fs.c
> index 73515d54e1cc..d1bd53c895ca 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -156,6 +156,8 @@ struct ffs_io_data {
>  	struct usb_request *req;
> 
>  	struct ffs_data *ffs;
> +
> +	ssize_t expected_len;
>  };
> 
>  struct ffs_desc_helper {
> @@ -730,8 +732,10 @@ static ssize_t ffs_epfile_io(struct file *file, struct
> ffs_io_data *io_data)
>  		 * Controller may require buffer size to be aligned to
>  		 * maxpacketsize of an out endpoint.
>  		 */
> -		if (io_data->read)
> +		if (io_data->read) {
> +			io_data->expected_len = data_len;
>  			data_len = usb_ep_align_maybe(gadget, ep->ep,
> data_len);
> +		}
>  		spin_unlock_irq(&epfile->ffs->eps_lock);
> 
>  		data = kmalloc(data_len, GFP_KERNEL);
> @@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct
> ffs_io_data *io_data)
>  		 */
>  		ret = interrupted ? -EINTR : ep->status;
>  		if (io_data->read && ret > 0) {
> -			ret = copy_to_iter(data, ret, &io_data->data);
> +			if (ret > io_data->expected_len)
> +				pr_debug("FFS: size mismatch: %zd for %zd",
> +						ret, io_data->expected_len);
> +
> +			ret = copy_to_iter(data, io_data->expected_len,
> +					&io_data->data);
>  			if (!ret)
>  				ret = -EFAULT;
>  		}
> 
> that we can get merged during v4.7-rc and Cc stable and backport this to
> anything containing Al's commit c993c39b8639 ("gadget/function/f_fs.c:
> use put iov_iter into io_data").
> 

The different for this code is just give warning but not return error. It is also
fine for me that at least this let development can find some key message to
find What happed under kernel. But the message should be *error* I think.

And this missed AIO path. This is identify to my patch after remove the
"return -EOVERFLOW;" line.

Byw, we not need add the field "expected_len", we can get it from the
struct ffs_io_data.

If this is fine for you, I can publish a new patch.

> --
> Balbi

Best Regards,
Du, Changbin