| 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
| ||
|
Message-ID: <xa1tk2it7rcb.fsf@mina86.com>
Date: Mon, 16 May 2016 21:09:08 +0200
From: Michal Nazarewicz <mina86@...a86.com>
To: Felipe Balbi <felipe.balbi@...ux.intel.com>,
Alan Stern <stern@...land.harvard.edu>
Cc: "Du\, Changbin" <changbin.du@...el.com>,
Al Viro <viro@...iv.linux.org.uk>,
"gregkh\@linuxfoundation.org" <gregkh@...uxfoundation.org>,
"rui.silva\@linaro.org" <rui.silva@...aro.org>,
"k.opasiak\@samsung.com" <k.opasiak@...sung.com>,
"lars\@metafoo.de" <lars@...afoo.de>,
"linux-usb\@vger.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: gadget: f_fs: report error if excess data received
On Mon, May 16 2016, Felipe Balbi wrote:
> Michal Nazarewicz <mina86@...a86.com> writes:
>
>>> Alan Stern <stern@...land.harvard.edu> writes:
>>>> The point is that you don't know whether the host sent more data than
>>>> expected. All you know is that the host sent more data than the user
>>>> asked the kernel for -- but maybe the user didn't ask for all the
>>>> data that he expected. Maybe the user wanted to retrieve the full
>>>> set of data using two read() system calls.
>>
>> On Mon, May 16 2016, Felipe Balbi wrote:
>>> right, but that just means we need to buffer the data instead of bailing
>>> out of the first read() completely.
>>
>> Correct.
>>
>> I have a ~4h bus ride ahead of me so I’ll try to implement it. If you
>> don’t hear from me by the end of the day, there probably wasn’t enough
>> space/comfort in the bus to use a laptop.
>
> Cool, Michal. Thanks
>
> seems like a kfifo would do well here(?)
There appears to be no kfifo support for iov_iter though, so I just went
with a simple buffer.
I haven’t looked at the patch too carefully so this is an RFC rather
than an actual patch at this point. It does compile at least.
Regardless, the more I thin about it, the more I’m under the impression
that the whole rounding up in f_fs was a mistake. And the more I’m
leaning towards ignoring the excess data set by the host.
---------- >8 ----------------------------------------------------------
Subject: usb: gadget: f_fs: buffer data from ‘oversized’ OUT requests
f_fs rounds up read(2) requests to a multiple of a max packet size
which means that host may provide more data than user has space for.
So far, the excess data has been silently ignored.
This introduces a buffer for a tail of such requests so that they are
returned on next read instead of being ignored.
Signed-off-by: Michal Nazarewicz <mina86@...a86.com>
---
drivers/usb/gadget/function/f_fs.c | 63 +++++++++++++++++++++++++++++++++-----
1 file changed, 56 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 2c314c1..7d3c51a 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -130,6 +130,12 @@ struct ffs_epfile {
struct dentry *dentry;
+ /*
+ * Buffer for holding data from partial reads which may happen since
+ * we’re rounding user read requests to a multiple of a max packet size.
+ */
+ struct ffs_buffer *read_buffer;
+
char name[5];
unsigned char in; /* P: ffs->eps_lock */
@@ -138,6 +144,12 @@ struct ffs_epfile {
unsigned char _pad;
};
+struct ffs_buffer {
+ size_t length;
+ char *data;
+ char storage[];
+};
+
/* ffs_io_data structure ***************************************************/
struct ffs_io_data {
@@ -681,6 +693,24 @@ static void ffs_epfile_async_io_complete(struct usb_ep *_ep,
schedule_work(&io_data->work);
}
+static ssize_t ffs_epfile_read_buffered(struct ffs_epfile *epfile,
+ struct iov_iter *iter)
+{
+ struct ffs_buffer *buf = epfile->read_buffer;
+ ssize_t ret = 0;
+ if (buf) {
+ ret = copy_to_iter(buf->data, buf->length, iter);
+ buf->length -= ret;
+ if (buf->length) {
+ buf->data += ret;
+ } else {
+ kfree(buf);
+ epfile->read_buffer = NULL;
+ }
+ }
+ return ret;
+}
+
static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
{
struct ffs_epfile *epfile = file->private_data;
@@ -710,6 +740,18 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
if (halt && epfile->isoc)
return -EINVAL;
+ /*
+ * Do we have buffered data from previous partial read? Check that for
+ * synchronous case only because we do not have facility to ‘wake up’
+ * a pending asynchronous read and push buffered data to it which we
+ * would need to make things behave consistently.
+ */
+ if (!halt && !io_data->aio && io_data->read) {
+ ret = ffs_epfile_read_buffered(epfile, &io_data->data);
+ if (ret)
+ return ret;
+ }
+
/* Allocate & copy */
if (!halt) {
/*
@@ -804,17 +846,24 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
interrupted = ep->status < 0;
}
- /*
- * XXX We may end up silently droping data here. Since data_len
- * (i.e. req->length) may be bigger than len (after being
- * rounded up to maxpacketsize), we may end up with more data
- * then user space has space for.
- */
ret = interrupted ? -EINTR : ep->status;
if (io_data->read && ret > 0) {
+ size_t left;
ret = copy_to_iter(data, ret, &io_data->data);
- if (!ret)
+ left = ep->status - ret;
+ if (!left) {
+ /* nop */
+ } else if (iov_iter_count(&io_data->data)) {
ret = -EFAULT;
+ } else {
+ struct ffs_buffer *buf = kmalloc(
+ sizeof(*epfile->read_buffer) + left,
+ GFP_KERNEL);
+ buf->length = left;
+ buf->data = buf->storage;
+ memcpy(buf->storage, data + ret, left);
+ epfile->read_buffer = buf;
+ }
}
goto error_mutex;
} else if (!(req = usb_ep_alloc_request(ep->ep, GFP_KERNEL))) {
--
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
Powered by blists - more mailing lists