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:

>> 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)

	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.

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.

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.

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.

please let me know if this is something that sounds ok, and I'll prepare it and
submit a new patch.

>
> thanks,
>
> greg k-h


--
Daniel M. German                  "The fear is not losing.
   Phil Jackson ->                 The fear is not producing the effort needed."
http://turingmachine.org/
http://silvernegative.com/
dmg (at) uvic (dot) ca
replace (at) with @ and (dot) with .