Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in adutux driver

[dmg <dmg@turingmachine.org>]

Greg KH <gregkh@linuxfoundation.org> writes:

> On Tue, Jun 18, 2019 at 10:22:52AM -0700, dmg wrote:
>>
>> Greg KH <gregkh@linuxfoundation.org> 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 .