Oliver Neukum <oliver@xxxxxxxxxx> writes:
> On Thursday 14 February 2013 18:10:43 Bjørn Mork wrote:
>> Do not scribble past end of buffer. Check if the userspace buffer has
>> enough space available before attempting to move more data there. Drop
>> new data on overflow.
>>
>> Cc: stable <stable@xxxxxxxxxxxxxxx>
>> Signed-off-by: Bjørn Mork <bjorn@xxxxxxx>
>> ---
>> Oliver Neukum <oneukum@xxxxxxx> writes:
>>
>> > I am afraid your diagnosis is correct. This is a buffer overflow. Not good.
>> > The fix is a problem. It seems to me that we need to throw away the
>> > newest data and report an error.
>>
>> OK. I preferred receiving the newest data for my use case, but I trust that
>> you know what's best as usual :-)
>
> We have to let user space recover. To do so we need to indicate when
> exactly we dropped data.
The problem with that is that this is likely to happen when a client
just doesn't care. It will just continue happily ignoring the read data,
writing new commands or whatever. Then the *next* client opening the
file for reading will see this error.
OK, that may help it understand that there was some read error. But it
is still more likely to care about any data received after it opened the
file, and not the old stuff we saved.
> How about this?
>
> Regards
> Oliver
>
> From 6be64bd7522f1c11a535741840797030c0436acf Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@xxxxxxx>
> Date: Thu, 14 Feb 2013 21:26:35 +0100
> Subject: [PATCH] cdc-wdm: fix buffer overflow
>
> If a read overflows the limit under which a single transfer
> can overflow the buffer we allow no further data into the buffer
> and remember an error
>
> Signed-off-by: Oliver Neukum <oneukum@xxxxxxx>
> ---
> drivers/usb/class/cdc-wdm.c | 24 ++++++++++++++++++------
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index 5f0cb41..2a5963b 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -184,10 +184,19 @@ static void wdm_in_callback(struct urb *urb)
> }
> }
>
> + /* hopeless congestion or error*/
> + if (desc->rerr < 0)
> + goto skip_error;
> +
> desc->rerr = status;
> desc->reslength = urb->actual_length;
> + if (desc->length > desc->wMaxCommand) {
> + /* the buffer is full */
> + desc->rerr = -ENOBUFS;
> + }
> memmove(desc->ubuf + desc->length, desc->inbuf, desc->reslength);
> desc->length += desc->reslength;
Sorry, but I find that still too fragile. To know whether this solves
the problem I will have to check every possible site where desc->rerr
can be reset, including those we may add in the future. I trust that
you have verified that this works now, but it is still too easy to break
without noticing.
There should be an explicit test for buffer space here. Indirect
testing via some other variable is risky IMHO.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
