diff mbox series

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

Message ID 20190618153529.11418-1-dmg@turingmachine.org (mailing list archive)
State New, archived
Headers show
Series usb: Replace a < b ? a : b construct with min_t(type, a, b) in adutux driver | expand

Commit Message

dmg June 18, 2019, 3:35 p.m. UTC 
From: Daniel M German <dmg@turingmachine.org>

Use min_t to find the minimum of two values instead of using the ?: operator.

We use min_t instead of min to avoid the compilation warning 'comparison of
distinct pointer types lacks a cast'.

This change does not alter functionality. It is merely cosmetic intended to
improve the readability of the code.

Signed-off-by: Daniel M German <dmg@turingmachine.org>
---
 drivers/usb/misc/adutux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Greg KH June 18, 2019, 4:06 p.m. UTC | #1 
On Tue, Jun 18, 2019 at 08:35:29AM -0700, dmg@turingmachine.org wrote:
> From: Daniel M German <dmg@turingmachine.org>
> 
> Use min_t to find the minimum of two values instead of using the ?: operator.
> 
> We use min_t instead of min to avoid the compilation warning 'comparison of
> distinct pointer types lacks a cast'.
> 
> This change does not alter functionality. It is merely cosmetic intended to
> improve the readability of the code.
> 
> Signed-off-by: Daniel M German <dmg@turingmachine.org>
> ---
>  drivers/usb/misc/adutux.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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?

thanks,

greg k-h
dmg June 18, 2019, 4:18 p.m. UTC | #2 
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?


Hi Greg,

sorry,  I saw your comment to the other thread after I sent this one. I
agree. I'll address this issue.

Sorry for the wasted bandwidh, and thank you for the suggestion,

--dmg
dmg June 18, 2019, 5:22 p.m. UTC | #3 
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 .
Greg KH June 18, 2019, 6:13 p.m. UTC | #4 
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.

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?

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

Oh, and great report, that was nicely done.

thanks,

greg k-h
dmg June 19, 2019, 4:39 p.m. UTC | #5 
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 .
Greg KH June 19, 2019, 5:04 p.m. UTC | #6 
On Wed, Jun 19, 2019 at 09:39:55AM -0700, dmg wrote:
> 
> 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.

No need to "connect" it if you don't want to, just send the new one.

thanks,

greg k-h
diff mbox series

Patch

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);
 			i = copy_to_user(buffer, dev->read_buffer_secondary+dev->secondary_head, amount);
 			if (i) {
 				retval = -EFAULT;