Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
> On Tue, Jun 18, 2019 at 10:22:52AM -0700, dmg wrote:
>>
>> Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
>>
>> >> diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
>> >> index 9465fb95d70a..4a9fa3152f2a 100644
>> >> --- a/drivers/usb/misc/adutux.c
>> >> +++ b/drivers/usb/misc/adutux.c
>> >> @@ -379,7 +379,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
>> >>
>> >> if (data_in_secondary) {
>> >> /* drain secondary buffer */
>> >> - int amount = bytes_to_read < data_in_secondary ? bytes_to_read : data_in_secondary;
>> >> + int amount = min_t(size_t, bytes_to_read, data_in_secondary);
>> >
>> > Shouldn't amount and data_in_secondary be of size_t type? Then you can
>> > just use min() here, right?
>>
>>
>> I looked at the code.
>>
>> there is an implicit assertion that
>>
>> dev->secondary_tail >= dev->secondary_head
>>
>>
>> (which are pointers to the secondary buffer)
>
> No, those are integers for the buffer, secondary_tail seems just to be
> the "length" of the buffer, and secondary_head is the current "start" of
> the buffer that has not been sent yet.
>
> So these can not ever "wrap", thank goodness.
I looked at the code and yes, the cannot wrap.
>
> But really, ick ick ick, this code is odd. Seems like they are trying
> to go with a "flip buffer" scheme when there are many simpler ways to do
> all of this...
>
> Oh well, we deal with what we have...
>
>> while (bytes_to_read) {
>> int data_in_secondary = dev->secondary_tail - dev->secondary_head;
>> dev_dbg(&dev->udev->dev,
>> "%s : while, data_in_secondary=%d, status=%d\n",
>> __func__, data_in_secondary,
>> dev->interrupt_in_urb->status);
>>
>> if (data_in_secondary) {
>> /* drain secondary buffer */
>> int amount = bytes_to_read < data_in_secondary ? bytes_to_read : data_in_secondary;
>> i = copy_to_user(buffer, dev->read_buffer_secondary+dev->secondary_head, amount);
>> if (i) {
>> retval = -EFAULT;
>> goto exit;
>> }
>> dev->secondary_head += (amount - i);
>> bytes_read += (amount - i);
>> bytes_to_read -= (amount - i);
>> } else {
>> /* we check the primary buffer */
>>
>> As computed, data_in_secondary is the number of bytes left in the secondary
>> buffer and hence, it should always be larger or equal 0.
>
> Yes.
>
>> The if statement seems to imply this fact. There is no handling of the case
>> where data_in_secondary is negative--if that was the case, copy_to_user will be
>> called with a negative number, which I assume will return an error.
>
> Looking at the code, it never can be.
>
> And no, copy_to_user() with a negative number is just a really BIG
> number, and bad things happen, we never want to do that as it's usually
> a security issue then.
>
>> This is interesting. It means that if the pointers are incorrect (head points
>> after tail), the current code will probably be able to recover from the bug with
>> an error (i assume copy_to_user will return != 0 if called with a negative
>> number).
>>
>> If we change data_in_secondary to size_t, and the head points after the tail,
>> the data_in_secondary will be invalid (but positive) and copy_to_user will try
>> to read those number of bytes. I don't know what would happen in that case.
>
> Looking at the code, I do not see how this can happen, do you?
I agree. No, it cannot.
>
>> Amount is number of bytes to read from this buffer, so it does not make sense for it to be
>> negative. So it being size_t sounds ok.
>>
>> Barring that potential bug in the values of the pointers of the head and the
>> tail, it appears it is safe to change the type of both data_in_secondary and
>> amount to size_t, and use min instead. It will also require to change the printf-style
>> modifier (%d => %lu)
>>
>> Also, note the use of "amount -i" the end of the if statement. At this point i
>> is equal to zero. the "- i" can be dropped from all these computations.
>
> That is someone who did not know what copy_to_user() returned and
> assumed it was the number of bytes copied :(
>
>> please let me know if this is something that sounds ok, and I'll prepare it and
>> submit a new patch.
>
> It sounds ok. And if you want to fix anything else up here in this
> mess, it's always appreciated. Odds are no one uses this driver
> anymore, but that's no reason to keep it being such a mess :)
I have created a new patch. But I am not sure what is the best way to 'connect'
my new patch to this one. I am currently using git-send-mail to avoid
interference from my mail client. Unless you think there is a better way, I'll
send it as I have before.
thank you again,
--dmg
>
> Oh, and great report, that was nicely done.
>
> thanks,
>
> greg k-h
--
Daniel M. German "Wrong turns are part
Adam Savage -> of every journey."
http://turingmachine.org/
http://silvernegative.com/
dmg (at) uvic (dot) ca
replace (at) with @ and (dot) with .
