* [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in adutux driver @ 2019-06-18 15:35 dmg 2019-06-18 16:06 ` Greg KH 0 siblings, 1 reply; 7+ messages in thread From: dmg @ 2019-06-18 15:35 UTC (permalink / raw) To: linux-usb; +Cc: gregkh, Daniel M German 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); i = copy_to_user(buffer, dev->read_buffer_secondary+dev->secondary_head, amount); if (i) { retval = -EFAULT; -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in adutux driver 2019-06-18 15:35 [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in adutux driver dmg @ 2019-06-18 16:06 ` Greg KH 2019-06-18 16:18 ` dmg 2019-06-18 17:22 ` dmg 0 siblings, 2 replies; 7+ messages in thread From: Greg KH @ 2019-06-18 16:06 UTC (permalink / raw) To: dmg; +Cc: linux-usb 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in adutux driver 2019-06-18 16:06 ` Greg KH @ 2019-06-18 16:18 ` dmg 2019-06-18 17:22 ` dmg 1 sibling, 0 replies; 7+ messages in thread From: dmg @ 2019-06-18 16:18 UTC (permalink / raw) To: Greg KH; +Cc: linux-usb 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in adutux driver 2019-06-18 16:06 ` Greg KH 2019-06-18 16:18 ` dmg @ 2019-06-18 17:22 ` dmg 2019-06-18 18:13 ` Greg KH 1 sibling, 1 reply; 7+ messages in thread From: dmg @ 2019-06-18 17:22 UTC (permalink / raw) To: Greg KH; +Cc: linux-usb 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 . ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in adutux driver 2019-06-18 17:22 ` dmg @ 2019-06-18 18:13 ` Greg KH 2019-06-19 16:39 ` dmg 0 siblings, 1 reply; 7+ messages in thread From: Greg KH @ 2019-06-18 18:13 UTC (permalink / raw) To: dmg; +Cc: linux-usb 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in adutux driver 2019-06-18 18:13 ` Greg KH @ 2019-06-19 16:39 ` dmg 2019-06-19 17:04 ` Greg KH 0 siblings, 1 reply; 7+ messages in thread From: dmg @ 2019-06-19 16:39 UTC (permalink / raw) To: Greg KH; +Cc: linux-usb 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 . ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in adutux driver 2019-06-19 16:39 ` dmg @ 2019-06-19 17:04 ` Greg KH 0 siblings, 0 replies; 7+ messages in thread From: Greg KH @ 2019-06-19 17:04 UTC (permalink / raw) To: dmg; +Cc: linux-usb 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-06-19 17:04 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-18 15:35 [PATCH] usb: Replace a < b ? a : b construct with min_t(type, a, b) in adutux driver dmg 2019-06-18 16:06 ` Greg KH 2019-06-18 16:18 ` dmg 2019-06-18 17:22 ` dmg 2019-06-18 18:13 ` Greg KH 2019-06-19 16:39 ` dmg 2019-06-19 17:04 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).