Initialize the buffer before passing it to usb_read_cmd() function(s) to
fix the uninit-was-stored issue in asix_read_cmd().
Fixes: KMSAN: kernel-infoleak in raw_ioctl
Reported by: syzbot+a7e220df5a81d1ab400e@syzkaller.appspotmail.com
Signed-off-by: Himadri Pandya <redacted>
---
drivers/net/usb/asix_common.c | 2 ++
1 file changed, 2 insertions(+)
On Sun, Aug 23, 2020 at 10:21 AM Himadri Pandya
[off-list ref] wrote:
quoted hunk
Initialize the buffer before passing it to usb_read_cmd() function(s) to
fix the uninit-was-stored issue in asix_read_cmd().
Fixes: KMSAN: kernel-infoleak in raw_ioctl
Reported by: syzbot+a7e220df5a81d1ab400e@syzkaller.appspotmail.com
Signed-off-by: Himadri Pandya <redacted>
---
drivers/net/usb/asix_common.c | 2 ++
1 file changed, 2 insertions(+)
Hi Himadri,
I think the proper fix is to check
usbnet_read_cmd/usbnet_read_cmd_nopm return value instead.
Memsetting data helps to fix the warning at hand, but the device did
not send these 0's and we use them as if the device did send them.
Perhaps we need a separate helper function (of a bool flag) that will
fail on incomplete reads. Maybe even in the common USB layer because I
think we've seen this type of bug lots of times and I guess there are
dozens more.
if (!in_pm)
fn = usbnet_read_cmd;
else
--
2.17.1
--
You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20200823082042.20816-1-himadrispandya%40gmail.com.
On Sun, Aug 23, 2020 at 11:26:27AM +0200, Dmitry Vyukov wrote:
On Sun, Aug 23, 2020 at 10:21 AM Himadri Pandya
[off-list ref] wrote:
quoted
Initialize the buffer before passing it to usb_read_cmd() function(s) to
fix the uninit-was-stored issue in asix_read_cmd().
Fixes: KMSAN: kernel-infoleak in raw_ioctl
Reported by: syzbot+a7e220df5a81d1ab400e@syzkaller.appspotmail.com
Signed-off-by: Himadri Pandya <redacted>
---
drivers/net/usb/asix_common.c | 2 ++
1 file changed, 2 insertions(+)
Hi Himadri,
I think the proper fix is to check
usbnet_read_cmd/usbnet_read_cmd_nopm return value instead.
Memsetting data helps to fix the warning at hand, but the device did
not send these 0's and we use them as if the device did send them.
But, for broken/abusive devices, that really is the safest thing to do
here. They are returning something that is obviously not correct, so
either all callers need to check the size received really is the size
they asked for, or we just plod onward with a 0 value like this. Or we
could pick some other value, but that could cause other problems if it
is treated as an actual value.
Perhaps we need a separate helper function (of a bool flag) that will
fail on incomplete reads. Maybe even in the common USB layer because I
think we've seen this type of bug lots of times and I guess there are
dozens more.
It's not always a failure, some devices have protocols that are "I could
return up to a max X bytes but could be shorter" types of messages, so
it's up to the caller to check that they got what they really asked for.
Yes, it's more work to do this checking. However converting the world
over to a "give me an error value if you don't read X number of bytes"
function would also be the same amount of work, right?
So personally, I think this patch is right for when you are trying to
abuse the USB driver stack :)
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Sun, Aug 23, 2020 at 12:19 PM Greg Kroah-Hartman
[off-list ref] wrote:
On Sun, Aug 23, 2020 at 11:26:27AM +0200, Dmitry Vyukov wrote:
quoted
On Sun, Aug 23, 2020 at 10:21 AM Himadri Pandya
[off-list ref] wrote:
quoted
Initialize the buffer before passing it to usb_read_cmd() function(s) to
fix the uninit-was-stored issue in asix_read_cmd().
Fixes: KMSAN: kernel-infoleak in raw_ioctl
Reported by: syzbot+a7e220df5a81d1ab400e@syzkaller.appspotmail.com
Signed-off-by: Himadri Pandya <redacted>
---
drivers/net/usb/asix_common.c | 2 ++
1 file changed, 2 insertions(+)
Hi Himadri,
I think the proper fix is to check
usbnet_read_cmd/usbnet_read_cmd_nopm return value instead.
Memsetting data helps to fix the warning at hand, but the device did
not send these 0's and we use them as if the device did send them.
But, for broken/abusive devices, that really is the safest thing to do
here. They are returning something that is obviously not correct, so
either all callers need to check the size received really is the size
they asked for, or we just plod onward with a 0 value like this. Or we
could pick some other value, but that could cause other problems if it
is treated as an actual value.
Do we want callers to do at least some error check (e.g. device did
not return anything at all, broke, hang)?
If yes, then with a separate helper function that fails on short
reads, we can get both benefits at no additional cost. User code will
say "I want 4 bytes, anything that is not 4 bytes is an error" and
then 1 error check will do. In fact, it seems that that was the
intention of whoever wrote this code (they assumed no short reads),
it's just they did not actually implement that "anything that is not 4
bytes is an error" part.
quoted
Perhaps we need a separate helper function (of a bool flag) that will
fail on incomplete reads. Maybe even in the common USB layer because I
think we've seen this type of bug lots of times and I guess there are
dozens more.
It's not always a failure, some devices have protocols that are "I could
return up to a max X bytes but could be shorter" types of messages, so
it's up to the caller to check that they got what they really asked for.
Yes, that's why I said _separate_ helper function. There seems to be
lots of callers that want exactly this -- "I want 4 bytes, anything
else is an error". With the current API it's harder to do - you need
additional checks, additional code, maybe even additional variables to
store the required size. APIs should make correct code easy to write.
Yes, it's more work to do this checking. However converting the world
over to a "give me an error value if you don't read X number of bytes"
function would also be the same amount of work, right?
Should this go into the common USB layer then?
It's weird to have such a special convention on the level of a single
driver. Why are rules for this single driver so special?...
So personally, I think this patch is right for when you are trying to
abuse the USB driver stack :)
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Sun, Aug 23, 2020 at 12:31:03PM +0200, Dmitry Vyukov wrote:
On Sun, Aug 23, 2020 at 12:19 PM Greg Kroah-Hartman
quoted
It's not always a failure, some devices have protocols that are "I could
return up to a max X bytes but could be shorter" types of messages, so
it's up to the caller to check that they got what they really asked for.
Yes, that's why I said _separate_ helper function. There seems to be
lots of callers that want exactly this -- "I want 4 bytes, anything
else is an error". With the current API it's harder to do - you need
additional checks, additional code, maybe even additional variables to
store the required size. APIs should make correct code easy to write.
One note on this, will respond to the rest of the email later.
It should be the same exact amount of code in the driver to handle this
either way:
Today's correctly written driver:
data_size = 4;
data = kmalloc(data_size, GFP_KERNEL);
...
retval = usb_control_msg(....., data, data_size, ...);
if (retval < buf_size) {
/* SOMETHING WENT WRONG! */
}
With your new function:
data_size = 4;
data = kmalloc(data_size, GFP_KERNEL);
...
retval = usb_control_msg_error_on_short(....., data, data_size, ...);
if (retval < 0) {
/* SOMETHING WENT WRONG! */
}
Catch the difference, it's only in checking for retval, either way you
are writing the exact same logic in the driver, you still have to tell
the USB layer the size of the buffer you want to read into, still have
to pass in the buffer, and everything else. You already know the size
of the data you want, and you already are doing the check, those things
you have to do no matter what, it's not extra work.
We can write a wrapper around usb_control_msg() for something like this
that does the transformation of a short read into an error, but really,
does that give us all that much here?
Yes, I want to make it easy to write correct drivers, and hard to get
things wrong, but in this case, I don't see the new way any "harder" to
get wrong.
Unless you know of a simpler way here?
thanks,
greg k-h
On Sun, Aug 23, 2020 at 12:31:03PM +0200, Dmitry Vyukov wrote:
On Sun, Aug 23, 2020 at 12:19 PM Greg Kroah-Hartman
[off-list ref] wrote:
quoted
On Sun, Aug 23, 2020 at 11:26:27AM +0200, Dmitry Vyukov wrote:
quoted
On Sun, Aug 23, 2020 at 10:21 AM Himadri Pandya
[off-list ref] wrote:
quoted
Initialize the buffer before passing it to usb_read_cmd() function(s) to
fix the uninit-was-stored issue in asix_read_cmd().
Fixes: KMSAN: kernel-infoleak in raw_ioctl
Reported by: syzbot+a7e220df5a81d1ab400e@syzkaller.appspotmail.com
Signed-off-by: Himadri Pandya <redacted>
---
drivers/net/usb/asix_common.c | 2 ++
1 file changed, 2 insertions(+)
Hi Himadri,
I think the proper fix is to check
usbnet_read_cmd/usbnet_read_cmd_nopm return value instead.
Memsetting data helps to fix the warning at hand, but the device did
not send these 0's and we use them as if the device did send them.
But, for broken/abusive devices, that really is the safest thing to do
here. They are returning something that is obviously not correct, so
either all callers need to check the size received really is the size
they asked for, or we just plod onward with a 0 value like this. Or we
could pick some other value, but that could cause other problems if it
is treated as an actual value.
Do we want callers to do at least some error check (e.g. device did
not return anything at all, broke, hang)?
If yes, then with a separate helper function that fails on short
reads, we can get both benefits at no additional cost. User code will
say "I want 4 bytes, anything that is not 4 bytes is an error" and
then 1 error check will do. In fact, it seems that that was the
intention of whoever wrote this code (they assumed no short reads),
it's just they did not actually implement that "anything that is not 4
bytes is an error" part.
quoted
quoted
Perhaps we need a separate helper function (of a bool flag) that will
fail on incomplete reads. Maybe even in the common USB layer because I
think we've seen this type of bug lots of times and I guess there are
dozens more.
It's not always a failure, some devices have protocols that are "I could
return up to a max X bytes but could be shorter" types of messages, so
it's up to the caller to check that they got what they really asked for.
Yes, that's why I said _separate_ helper function. There seems to be
lots of callers that want exactly this -- "I want 4 bytes, anything
else is an error". With the current API it's harder to do - you need
additional checks, additional code, maybe even additional variables to
store the required size. APIs should make correct code easy to write.
I guess I already answered both of these in my previous email...
quoted
Yes, it's more work to do this checking. However converting the world
over to a "give me an error value if you don't read X number of bytes"
function would also be the same amount of work, right?
Should this go into the common USB layer then?
It's weird to have such a special convention on the level of a single
driver. Why are rules for this single driver so special?...
They aren't special at all, so yes, we should be checking for a short
read everywhere. That would be the "correct" thing to do, I was just
suggesting a "quick fix" here, sorry.
Himadri, want to fix up all callers to properly check the size of the
message recieved before they access it? That will fix this issue
properly.
thanks,
greg k-h
On Sun, Aug 23, 2020 at 12:57 PM Greg Kroah-Hartman
[off-list ref] wrote:
On Sun, Aug 23, 2020 at 12:31:03PM +0200, Dmitry Vyukov wrote:
quoted
On Sun, Aug 23, 2020 at 12:19 PM Greg Kroah-Hartman
[off-list ref] wrote:
quoted
On Sun, Aug 23, 2020 at 11:26:27AM +0200, Dmitry Vyukov wrote:
quoted
On Sun, Aug 23, 2020 at 10:21 AM Himadri Pandya
[off-list ref] wrote:
quoted
Initialize the buffer before passing it to usb_read_cmd() function(s) to
fix the uninit-was-stored issue in asix_read_cmd().
Fixes: KMSAN: kernel-infoleak in raw_ioctl
Reported by: syzbot+a7e220df5a81d1ab400e@syzkaller.appspotmail.com
Signed-off-by: Himadri Pandya <redacted>
---
drivers/net/usb/asix_common.c | 2 ++
1 file changed, 2 insertions(+)
Hi Himadri,
I think the proper fix is to check
usbnet_read_cmd/usbnet_read_cmd_nopm return value instead.
Memsetting data helps to fix the warning at hand, but the device did
not send these 0's and we use them as if the device did send them.
But, for broken/abusive devices, that really is the safest thing to do
here. They are returning something that is obviously not correct, so
either all callers need to check the size received really is the size
they asked for, or we just plod onward with a 0 value like this. Or we
could pick some other value, but that could cause other problems if it
is treated as an actual value.
Do we want callers to do at least some error check (e.g. device did
not return anything at all, broke, hang)?
If yes, then with a separate helper function that fails on short
reads, we can get both benefits at no additional cost. User code will
say "I want 4 bytes, anything that is not 4 bytes is an error" and
then 1 error check will do. In fact, it seems that that was the
intention of whoever wrote this code (they assumed no short reads),
it's just they did not actually implement that "anything that is not 4
bytes is an error" part.
quoted
quoted
Perhaps we need a separate helper function (of a bool flag) that will
fail on incomplete reads. Maybe even in the common USB layer because I
think we've seen this type of bug lots of times and I guess there are
dozens more.
It's not always a failure, some devices have protocols that are "I could
return up to a max X bytes but could be shorter" types of messages, so
it's up to the caller to check that they got what they really asked for.
Yes, that's why I said _separate_ helper function. There seems to be
lots of callers that want exactly this -- "I want 4 bytes, anything
else is an error". With the current API it's harder to do - you need
additional checks, additional code, maybe even additional variables to
store the required size. APIs should make correct code easy to write.
I guess I already answered both of these in my previous email...
quoted
quoted
Yes, it's more work to do this checking. However converting the world
over to a "give me an error value if you don't read X number of bytes"
function would also be the same amount of work, right?
Should this go into the common USB layer then?
It's weird to have such a special convention on the level of a single
driver. Why are rules for this single driver so special?...
They aren't special at all, so yes, we should be checking for a short
read everywhere. That would be the "correct" thing to do, I was just
suggesting a "quick fix" here, sorry.
Re quick fix, I guess it depends on the amount of work for the larger
fix and if we can find volunteers (thanks Himadri!). We need to be
practical as well.
Re:
retval = usb_control_msg(....., data, data_size, ...);
if (retval < buf_size) {
There may be a fine line between interfaces and what code they
provoke. Let me describe my reasoning.
Yes, the current interface allows writing correct code with moderate
amount of effort. Yet we see cases where it's used incorrectly, maybe
people were just a little bit lazy, or maybe they did not understand
how to use it properly (nobody reads the docs, and it's also
reasonable to assume that if you ask for N bytes and the function does
not fail, then you get N bytes).
Currently to write correct code (1) we need a bit of duplication,
which gets worse if data_size is actually some lengthy expression
(X+Y*Z), maybe one will need an additional variable to use it
correctly.
(2) one needs to understand the contract;
(3) may be subject to the following class of bugs (after some copy-paste:
retval = usb_control_msg(....., data, 4, ...);
if (retval < 2) {
This class of bugs won't be necessary immediately caught by kernel
testing systems (can have long life-time).
I would add a "default" function (with shorter name) that does full read:
if (!usb_control_msg(, ...., data, 4))
and a function with longer name to read variable-size data:
n = usb_control_msg_variable_length(, ...., data, sizeof(data)));
The full read should be "the default" (shorter name), because if you
need full read and use the wrong function, it won't be caught by
testing (most likely long-lived bug). Whereas if you use full read for
lengthy variable size data read, this will be immediately caught
during any testing (even manual) -- you ask for 4K, you get fewer
bytes, all your reads fail.
So having "full read" easier to spell will lead to fewer bugs by design.
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-08-24 18:17:05
On Sun, 23 Aug 2020 13:50:42 +0530 Himadri Pandya wrote:
Initialize the buffer before passing it to usb_read_cmd() function(s) to
fix the uninit-was-stored issue in asix_read_cmd().
Fixes: KMSAN: kernel-infoleak in raw_ioctl
Regardless of the ongoing discussion - could you please make this line
a correct Fixes tag?
Right now integration scripts are complaining that it doesn't contain a
valid git hash.
On Mon, Aug 24, 2020 at 11:16:55AM -0700, Jakub Kicinski wrote:
On Sun, 23 Aug 2020 13:50:42 +0530 Himadri Pandya wrote:
quoted
Initialize the buffer before passing it to usb_read_cmd() function(s) to
fix the uninit-was-stored issue in asix_read_cmd().
Fixes: KMSAN: kernel-infoleak in raw_ioctl
Regardless of the ongoing discussion - could you please make this line
a correct Fixes tag?
Right now integration scripts are complaining that it doesn't contain a
valid git hash.
On Mon, Aug 24, 2020 at 10:55:28AM +0200, Dmitry Vyukov wrote:
On Sun, Aug 23, 2020 at 12:57 PM Greg Kroah-Hartman
[off-list ref] wrote:
quoted
On Sun, Aug 23, 2020 at 12:31:03PM +0200, Dmitry Vyukov wrote:
quoted
On Sun, Aug 23, 2020 at 12:19 PM Greg Kroah-Hartman
[off-list ref] wrote:
quoted
On Sun, Aug 23, 2020 at 11:26:27AM +0200, Dmitry Vyukov wrote:
quoted
On Sun, Aug 23, 2020 at 10:21 AM Himadri Pandya
[off-list ref] wrote:
quoted
Initialize the buffer before passing it to usb_read_cmd() function(s) to
fix the uninit-was-stored issue in asix_read_cmd().
Fixes: KMSAN: kernel-infoleak in raw_ioctl
Reported by: syzbot+a7e220df5a81d1ab400e@syzkaller.appspotmail.com
Signed-off-by: Himadri Pandya <redacted>
---
drivers/net/usb/asix_common.c | 2 ++
1 file changed, 2 insertions(+)
Hi Himadri,
I think the proper fix is to check
usbnet_read_cmd/usbnet_read_cmd_nopm return value instead.
Memsetting data helps to fix the warning at hand, but the device did
not send these 0's and we use them as if the device did send them.
But, for broken/abusive devices, that really is the safest thing to do
here. They are returning something that is obviously not correct, so
either all callers need to check the size received really is the size
they asked for, or we just plod onward with a 0 value like this. Or we
could pick some other value, but that could cause other problems if it
is treated as an actual value.
Do we want callers to do at least some error check (e.g. device did
not return anything at all, broke, hang)?
If yes, then with a separate helper function that fails on short
reads, we can get both benefits at no additional cost. User code will
say "I want 4 bytes, anything that is not 4 bytes is an error" and
then 1 error check will do. In fact, it seems that that was the
intention of whoever wrote this code (they assumed no short reads),
it's just they did not actually implement that "anything that is not 4
bytes is an error" part.
quoted
quoted
Perhaps we need a separate helper function (of a bool flag) that will
fail on incomplete reads. Maybe even in the common USB layer because I
think we've seen this type of bug lots of times and I guess there are
dozens more.
It's not always a failure, some devices have protocols that are "I could
return up to a max X bytes but could be shorter" types of messages, so
it's up to the caller to check that they got what they really asked for.
Yes, that's why I said _separate_ helper function. There seems to be
lots of callers that want exactly this -- "I want 4 bytes, anything
else is an error". With the current API it's harder to do - you need
additional checks, additional code, maybe even additional variables to
store the required size. APIs should make correct code easy to write.
I guess I already answered both of these in my previous email...
quoted
quoted
Yes, it's more work to do this checking. However converting the world
over to a "give me an error value if you don't read X number of bytes"
function would also be the same amount of work, right?
Should this go into the common USB layer then?
It's weird to have such a special convention on the level of a single
driver. Why are rules for this single driver so special?...
They aren't special at all, so yes, we should be checking for a short
read everywhere. That would be the "correct" thing to do, I was just
suggesting a "quick fix" here, sorry.
Re quick fix, I guess it depends on the amount of work for the larger
fix and if we can find volunteers (thanks Himadri!). We need to be
practical as well.
Re:
retval = usb_control_msg(....., data, data_size, ...);
if (retval < buf_size) {
There may be a fine line between interfaces and what code they
provoke. Let me describe my reasoning.
Yes, the current interface allows writing correct code with moderate
amount of effort. Yet we see cases where it's used incorrectly, maybe
people were just a little bit lazy, or maybe they did not understand
how to use it properly (nobody reads the docs, and it's also
reasonable to assume that if you ask for N bytes and the function does
not fail, then you get N bytes).
I did a quick scan of the tree, and in short, I think it's worse than we
both imagined, more below...
Currently to write correct code (1) we need a bit of duplication,
which gets worse if data_size is actually some lengthy expression
(X+Y*Z), maybe one will need an additional variable to use it
correctly.
(2) one needs to understand the contract;
(3) may be subject to the following class of bugs (after some copy-paste:
retval = usb_control_msg(....., data, 4, ...);
if (retval < 2) {
This class of bugs won't be necessary immediately caught by kernel
testing systems (can have long life-time).
I would add a "default" function (with shorter name) that does full read:
if (!usb_control_msg(, ...., data, 4))
and a function with longer name to read variable-size data:
n = usb_control_msg_variable_length(, ...., data, sizeof(data)));
The full read should be "the default" (shorter name), because if you
need full read and use the wrong function, it won't be caught by
testing (most likely long-lived bug). Whereas if you use full read for
lengthy variable size data read, this will be immediately caught
during any testing (even manual) -- you ask for 4K, you get fewer
bytes, all your reads fail.
So having "full read" easier to spell will lead to fewer bugs by design.
Originally I would sick to my first proposal that "all is fine" and the
api is "easy enough", but in auditing the tree, it's horrid.
The error checking for this function call is almost non-existant. And,
to make things more difficult, this is a bi-directional call, it is a
read or write call, depending on what USB endpoint the user asks for (or
both for some endpoints.) So trying to automatically scan the tree for
valid error handling is really really hard.
Combine that with the need of many subsystems to "wrap" this function in
a helper call, because the USB core isn't providing a useful call it
could call directly, and we have a total mess.
At first glance, I think this can all be cleaned up, but it will take a
bit of tree-wide work. I agree, we need a "read this message and error
if the whole thing is not there", as well as a "send this message and
error if the whole thing was not sent", and also a way to handle
stack-provided data, which seems to be the primary reason subsystems
wrap this call (they want to make it easier on their drivers to use it.)
Let me think about this in more detail, but maybe something like:
usb_control_msg_read()
usb_control_msg_send()
is a good first step (as the caller knows this) and stack provided data
would be allowed, and it would return an error if the whole message was
not read/sent properly. That way we can start converting everything
over to a sane, and checkable, api and remove a bunch of wrapper
functions as well.
thanks,
greg k-h
On Tue, Aug 25, 2020 at 08:51:35AM +0200, Greg Kroah-Hartman wrote:
On Mon, Aug 24, 2020 at 10:55:28AM +0200, Dmitry Vyukov wrote:
quoted
On Sun, Aug 23, 2020 at 12:57 PM Greg Kroah-Hartman
[off-list ref] wrote:
quoted
On Sun, Aug 23, 2020 at 12:31:03PM +0200, Dmitry Vyukov wrote:
quoted
On Sun, Aug 23, 2020 at 12:19 PM Greg Kroah-Hartman
[off-list ref] wrote:
quoted
On Sun, Aug 23, 2020 at 11:26:27AM +0200, Dmitry Vyukov wrote:
quoted
On Sun, Aug 23, 2020 at 10:21 AM Himadri Pandya
[off-list ref] wrote:
quoted
Initialize the buffer before passing it to usb_read_cmd() function(s) to
fix the uninit-was-stored issue in asix_read_cmd().
Fixes: KMSAN: kernel-infoleak in raw_ioctl
Reported by: syzbot+a7e220df5a81d1ab400e@syzkaller.appspotmail.com
Signed-off-by: Himadri Pandya <redacted>
---
drivers/net/usb/asix_common.c | 2 ++
1 file changed, 2 insertions(+)
Hi Himadri,
I think the proper fix is to check
usbnet_read_cmd/usbnet_read_cmd_nopm return value instead.
Memsetting data helps to fix the warning at hand, but the device did
not send these 0's and we use them as if the device did send them.
But, for broken/abusive devices, that really is the safest thing to do
here. They are returning something that is obviously not correct, so
either all callers need to check the size received really is the size
they asked for, or we just plod onward with a 0 value like this. Or we
could pick some other value, but that could cause other problems if it
is treated as an actual value.
Do we want callers to do at least some error check (e.g. device did
not return anything at all, broke, hang)?
If yes, then with a separate helper function that fails on short
reads, we can get both benefits at no additional cost. User code will
say "I want 4 bytes, anything that is not 4 bytes is an error" and
then 1 error check will do. In fact, it seems that that was the
intention of whoever wrote this code (they assumed no short reads),
it's just they did not actually implement that "anything that is not 4
bytes is an error" part.
quoted
quoted
Perhaps we need a separate helper function (of a bool flag) that will
fail on incomplete reads. Maybe even in the common USB layer because I
think we've seen this type of bug lots of times and I guess there are
dozens more.
It's not always a failure, some devices have protocols that are "I could
return up to a max X bytes but could be shorter" types of messages, so
it's up to the caller to check that they got what they really asked for.
Yes, that's why I said _separate_ helper function. There seems to be
lots of callers that want exactly this -- "I want 4 bytes, anything
else is an error". With the current API it's harder to do - you need
additional checks, additional code, maybe even additional variables to
store the required size. APIs should make correct code easy to write.
I guess I already answered both of these in my previous email...
quoted
quoted
Yes, it's more work to do this checking. However converting the world
over to a "give me an error value if you don't read X number of bytes"
function would also be the same amount of work, right?
Should this go into the common USB layer then?
It's weird to have such a special convention on the level of a single
driver. Why are rules for this single driver so special?...
They aren't special at all, so yes, we should be checking for a short
read everywhere. That would be the "correct" thing to do, I was just
suggesting a "quick fix" here, sorry.
Re quick fix, I guess it depends on the amount of work for the larger
fix and if we can find volunteers (thanks Himadri!). We need to be
practical as well.
Re:
retval = usb_control_msg(....., data, data_size, ...);
if (retval < buf_size) {
There may be a fine line between interfaces and what code they
provoke. Let me describe my reasoning.
Yes, the current interface allows writing correct code with moderate
amount of effort. Yet we see cases where it's used incorrectly, maybe
people were just a little bit lazy, or maybe they did not understand
how to use it properly (nobody reads the docs, and it's also
reasonable to assume that if you ask for N bytes and the function does
not fail, then you get N bytes).
I did a quick scan of the tree, and in short, I think it's worse than we
both imagined, more below...
quoted
Currently to write correct code (1) we need a bit of duplication,
which gets worse if data_size is actually some lengthy expression
(X+Y*Z), maybe one will need an additional variable to use it
correctly.
(2) one needs to understand the contract;
(3) may be subject to the following class of bugs (after some copy-paste:
retval = usb_control_msg(....., data, 4, ...);
if (retval < 2) {
This class of bugs won't be necessary immediately caught by kernel
testing systems (can have long life-time).
I would add a "default" function (with shorter name) that does full read:
if (!usb_control_msg(, ...., data, 4))
and a function with longer name to read variable-size data:
n = usb_control_msg_variable_length(, ...., data, sizeof(data)));
The full read should be "the default" (shorter name), because if you
need full read and use the wrong function, it won't be caught by
testing (most likely long-lived bug). Whereas if you use full read for
lengthy variable size data read, this will be immediately caught
during any testing (even manual) -- you ask for 4K, you get fewer
bytes, all your reads fail.
So having "full read" easier to spell will lead to fewer bugs by design.
Originally I would sick to my first proposal that "all is fine" and the
api is "easy enough", but in auditing the tree, it's horrid.
The error checking for this function call is almost non-existant. And,
to make things more difficult, this is a bi-directional call, it is a
read or write call, depending on what USB endpoint the user asks for (or
both for some endpoints.) So trying to automatically scan the tree for
valid error handling is really really hard.
Combine that with the need of many subsystems to "wrap" this function in
a helper call, because the USB core isn't providing a useful call it
could call directly, and we have a total mess.
At first glance, I think this can all be cleaned up, but it will take a
bit of tree-wide work. I agree, we need a "read this message and error
if the whole thing is not there", as well as a "send this message and
error if the whole thing was not sent", and also a way to handle
stack-provided data, which seems to be the primary reason subsystems
wrap this call (they want to make it easier on their drivers to use it.)
Let me think about this in more detail, but maybe something like:
usb_control_msg_read()
usb_control_msg_send()
is a good first step (as the caller knows this) and stack provided data
would be allowed, and it would return an error if the whole message was
not read/sent properly. That way we can start converting everything
over to a sane, and checkable, api and remove a bunch of wrapper
functions as well.
Oh, and if you want to start creating a bunch of syzbot bugs to report,
like this one, just start doing "short reads" on almost any control
message request...
greg k-h
From: Alan Stern <stern@rowland.harvard.edu> Date: 2020-08-25 14:39:55
On Tue, Aug 25, 2020 at 08:51:35AM +0200, Greg Kroah-Hartman wrote:
At first glance, I think this can all be cleaned up, but it will take a
bit of tree-wide work. I agree, we need a "read this message and error
if the whole thing is not there", as well as a "send this message and
error if the whole thing was not sent", and also a way to handle
stack-provided data, which seems to be the primary reason subsystems
wrap this call (they want to make it easier on their drivers to use it.)
Let me think about this in more detail, but maybe something like:
usb_control_msg_read()
usb_control_msg_send()
is a good first step (as the caller knows this) and stack provided data
would be allowed, and it would return an error if the whole message was
not read/sent properly. That way we can start converting everything
over to a sane, and checkable, api and remove a bunch of wrapper
functions as well.
Suggestion: _read and _send are not a natural pair. Consider instead
_read and _write. _recv and _send don't feel right either, because it
both cases the host sends the control message -- the difference lies
in who sends the data.
Alan Stern
On Tue, Aug 25, 2020 at 10:39:46AM -0400, Alan Stern wrote:
On Tue, Aug 25, 2020 at 08:51:35AM +0200, Greg Kroah-Hartman wrote:
quoted
At first glance, I think this can all be cleaned up, but it will take a
bit of tree-wide work. I agree, we need a "read this message and error
if the whole thing is not there", as well as a "send this message and
error if the whole thing was not sent", and also a way to handle
stack-provided data, which seems to be the primary reason subsystems
wrap this call (they want to make it easier on their drivers to use it.)
Let me think about this in more detail, but maybe something like:
usb_control_msg_read()
usb_control_msg_send()
is a good first step (as the caller knows this) and stack provided data
would be allowed, and it would return an error if the whole message was
not read/sent properly. That way we can start converting everything
over to a sane, and checkable, api and remove a bunch of wrapper
functions as well.
Suggestion: _read and _send are not a natural pair. Consider instead
_read and _write. _recv and _send don't feel right either, because it
both cases the host sends the control message -- the difference lies
in who sends the data.
Yes, naming is hard :)
usb_control_read_msg()
usb_control_write_msg()
feels good to me, let me try this out and see if it actually makes sense
to do this on a few in-usb-core files and various drivers...
thanks,
greg k-h
On Tue, Aug 25, 2020 at 04:44:37PM +0200, Greg Kroah-Hartman wrote:
On Tue, Aug 25, 2020 at 10:39:46AM -0400, Alan Stern wrote:
quoted
On Tue, Aug 25, 2020 at 08:51:35AM +0200, Greg Kroah-Hartman wrote:
quoted
At first glance, I think this can all be cleaned up, but it will take a
bit of tree-wide work. I agree, we need a "read this message and error
if the whole thing is not there", as well as a "send this message and
error if the whole thing was not sent", and also a way to handle
stack-provided data, which seems to be the primary reason subsystems
wrap this call (they want to make it easier on their drivers to use it.)
Let me think about this in more detail, but maybe something like:
usb_control_msg_read()
usb_control_msg_send()
is a good first step (as the caller knows this) and stack provided data
would be allowed, and it would return an error if the whole message was
not read/sent properly. That way we can start converting everything
over to a sane, and checkable, api and remove a bunch of wrapper
functions as well.
Suggestion: _read and _send are not a natural pair. Consider instead
_read and _write. _recv and _send don't feel right either, because it
both cases the host sends the control message -- the difference lies
in who sends the data.
Yes, naming is hard :)
usb_control_read_msg()
usb_control_write_msg()
feels good to me, let me try this out and see if it actually makes sense
to do this on a few in-usb-core files and various drivers...
Turns out we have a long history of using snd/rcv for USB control
messages:
usb_rcvctrlpipe()
usb_sndctrlpipe()
so while _recv and _send might feel a bit "odd", it is what we are used
to using, and when converting existing users, I can drop the pipe macro
from the calls, turning something like:
usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0), ...);
into:
usb_control_send_msg(hdev, 0, ...);
or maybe:
usb_control_msg_send(hdev, 0, ...);
with a full noun_verb pairing, instead of noun_verb_noun.
thanks,
greg k-h