Fix additional bad descriptor read issues #1340
Conversation
addisoncrump
force-pushed
the
various-descriptor-oddities
branch
from
83c4e8f
to
98b0f38
Compare
| @@ -212,7 +216,19 @@ static int parse_interface(libusb_context *ctx, | |||
| usb_interface->altsetting = altsetting; | |||
|
|
|||
| ifp = altsetting + usb_interface->num_altsetting; | |||
|
|
|||
| usb_interface->num_altsetting++; | |||
There was a problem hiding this comment.
Moving this above ensures that the altsetting is cleared in the case of an error.
There was a problem hiding this comment.
All comments should be /* */.
| uint8_t stashedNumEndpoints = ifp->bNumEndpoints; | ||
| ifp->bNumEndpoints = 0; |
There was a problem hiding this comment.
Stashing this ensures that we clear only the endpoints that are actually instantiated.
There was a problem hiding this comment.
Line 228 says: stash /in case/ we have to clear it. Then line 231 goes on to clear it. This is confusing.
bNumEndpoints will represent the number of initialised endpoints
Can't we call it e.g. numInitialisedEndpoints then, instead of "stashedNumEndpoints"?
| if (!endpoint) { | ||
| r = LIBUSB_ERROR_NO_MEM; | ||
| goto err; | ||
| } | ||
|
|
||
| ifp->endpoint = endpoint; | ||
| for (i = 0; i < ifp->bNumEndpoints; i++) { | ||
| r = parse_endpoint(ctx, endpoint + i, buffer, size); | ||
| for (; ifp->bNumEndpoints < stashedNumEndpoints; ifp->bNumEndpoints++) { |
There was a problem hiding this comment.
Making this explicit with the loop ensures that the endpoints are cleared in the case of an error.
| uint8_t stashedNumInterfaces = config->bNumInterfaces; | ||
| config->bNumInterfaces = 0; | ||
|
|
||
| for (; config->bNumInterfaces < stashedNumInterfaces; config->bNumInterfaces++) { |
There was a problem hiding this comment.
Same as previous: ensures that the interfaces are cleared in the case of an error.
| @@ -1218,7 +1229,7 @@ static int parse_iad_array(struct libusb_context *ctx, | |||
|
|
|||
| // First pass: Iterate through desc list, count number of IADs | |||
| iad_array->length = 0; | |||
| while (consumed < size) { | |||
| while (consumed + 1 < size) { | |||
There was a problem hiding this comment.
Prevents off-by-one heap overflow in the case that the amount consumed less than size, but by less than the descriptor we're about to read.
| @@ -1242,7 +1253,7 @@ static int parse_iad_array(struct libusb_context *ctx, | |||
| // Second pass: Iterate through desc list, fill IAD structures | |||
| consumed = 0; | |||
| i = 0; | |||
| while (consumed < size) { | |||
| while (consumed + 9 < size) { | |||
There was a problem hiding this comment.
Prevents nine byte eight heap overflow in the case that the amount consumed less than size, but by less than the descriptor we're about to read.
|
Instead of github conversations those would better be added as comments in the code. No one is going to come back to this PR to figure out why the code is structured the way it is. |
|
Fair point, it hadn't been commented in my previous PR so I just assumed it wasn't necessary here. Will update and add a comment to my previous PR section as well. |
|
Well, in the previous PR https://github.com/libusb/libusb/pull/1308/files the changed code is quite self-explanatory. This is not always the case here IMO. If there are e.g. subtle ordering requirements it is good to spell that out, since someone else might come later and change the code without thinking about that. Am I the only one who think we could find better names than stashed_xxx multiple times? I understand it is for saving or backing up a value, but maybe e.g. the reason for doing so could be reflected in the name? Anyway it is just a matter of taste, and sometimes |
|
Okay, comments added. Apologies for the delay. |
addisoncrump
force-pushed
the
various-descriptor-oddities
branch
from
23c02d9
to
748c732
Compare
|
This macOS test seems to be flaky... It did not fail in the previous CI run, and all that was changed in the last commit was some formatting items. |
|
Yes, nothing to worry about. It is some kind of race condition that kicks in sometimes, and maybe limited to virtual machines. |
|
@tormodvolden Looks like some of this will overlap/conflict with #1460. I propose getting 1460 merged first, then rebase this one. |
|
Yes, this one needs some fixing up now that parse_descriptor() is gone. |
|
The buffer overruns this PR fixes are hopefully already fixed by #1460 |
|
Sadly the code I wrote to test these cases is now wildly out of date. I will need to update it sometime after next week. |
Extends #1308.
This fixes a few additional potential memory issues associated with bad descriptor reads. I will annotate the commit below.