usbus/dfu: fix underflow condition while updating firmware #17128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
Area: sys
dylad
added
the
Type: bug
bergzand
reviewed
Nov 3, 2021
|
Please squash! |
Reports dfuERROR if underflow is detected or if flash write failed Implement DFU control CLRSTATUS while at it to clear dfuERROR by the host
dylad
force-pushed
the
pr/usbus/dfu/fix_underflow_condition
branch
from
d545639
to
64a82c9
Compare
bergzand
added
the
CI: ready for build
bergzand
approved these changes
Nov 4, 2021
There was a problem hiding this comment.
Tested this on an nrf52840dk with the bootloaders/riotboot_dfu example. Sending a 2 byte payload now properly returns an error to the host:
dfu-utils
koen@morgen ~/dev/RIOT-review/bootloaders/riotboot_dfu $ dfu-util -D payload.bin -a 1
dfu-util 0.9
Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2016 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to http:https://sourceforge.net/p/dfu-util/tickets/
dfu-util: File too short for DFU suffix
dfu-util: A valid DFU suffix will be required in a future dfu-util release!!!
Opening DFU capable USB device...
ID 1209:7d02
Run-time device DFU version 0110
Claiming USB DFU Interface...
Setting Alternate Setting #1 ...
Determining device status: state = dfuIDLE, status = 0
dfuIDLE, continuing
DFU mode device DFU version 0110
Device returned transfer size 64
Copying data from PC to DFU device
Download [ ] 0% 0 bytesdfu-util: Error during download
koen@morgen ~/dev/RIOT-review/bootloaders/riotboot_dfu $ dfu-util -D payload.bin -a 1
dfu-util 0.9
Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2016 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to http:https://sourceforge.net/p/dfu-util/tickets/
dfu-util: File too short for DFU suffix
dfu-util: A valid DFU suffix will be required in a future dfu-util release!!!
Opening DFU capable USB device...
ID 1209:7d02
Run-time device DFU version 0110
Claiming USB DFU Interface...
Setting Alternate Setting #1 ...
Determining device status: state = dfuERROR, status = 0
dfuERROR, clearing status
Determining device status: state = dfuIDLE, status = 0
dfuIDLE, continuing
DFU mode device DFU version 0110
Device returned transfer size 64
Copying data from PC to DFU device
Download [ ] 0% 0 bytesdfu-util: Error during download
|
Thanks for the quick fix @dylad |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
This PR fixes an underflow condition which can occurs if host sends less than 4 bytes of data during a DFU download operation.
While loading a new firmware through a
DFU_DNLOADrequest, DFU skips theRIOTBOOT_FLASHWRITE_SKIPLENbytes as we don't need them in flash. This skip wasn't guarded so if theDFU_DNLOADrequest contains less thanRIOTBOOT_FLASHWRITE_SKIPLENbytes, an underflow integer occurs with unexpected behaviour. Thus, add a proper check to prevent this situation from happening by stalling the request if it has less thanRIOTBOOT_FLASHWRITE_SKIPLENbytes of data.edit:
The internal DFU state machine was also updated and will now returns dfuERROR in case of failure (underflow or flash write failure) to the host (through GET_STATUS request).
A new request handling was added to clear this error and return to dfuIDLE (CLRSTATUS request).
This way dfu-util safely abort the dfu upgrade procedure and indicate that something was wrong. A new dfu update can be made right after without restarting the device.
Testing procedure
Issues/PRs references
Reported-by @szymonh through the forum, many thanks for his report !