Skip to content

usb: dfu: disable USB DFU uploading by default #42424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 18, 2022
Merged

usb: dfu: disable USB DFU uploading by default #42424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43da6a3
usb: do not add ZLP if lenght to be transmitted is just zero
jfischer-no Feb 2, 2022
0307c8f
usb: dfu: rename FLASH_AREA_ID macro
jfischer-no Feb 2, 2022
d6e20de
usb: dfu: disable USB DFU uploading by default
jfischer-no Feb 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions samples/subsys/usb/dfu/README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ partition layout. Refer to :ref:`flash_map_api` for details about
partition layout. You SoC must run MCUboot as the stage 1 bootloader.
This sample is built as an application for the MCUboot bootloader.

.. note::
This example explicitly turns :kconfig:`CONFIG_USB_DFU_ENABLE_UPLOAD` on.

Building and Testing
********************

Expand Down
1 change: 1 addition & 0 deletions samples/subsys/usb/dfu/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ CONFIG_GPIO=y
CONFIG_USB_DEVICE_STACK=y
CONFIG_USB_DEVICE_PRODUCT="Zephyr DFU sample"
CONFIG_USB_DFU_CLASS=y
CONFIG_USB_DFU_ENABLE_UPLOAD=y
CONFIG_FLASH=y
CONFIG_IMG_MANAGER=y
CONFIG_FLASH_PAGE_LAYOUT=y
Expand Down
8 changes: 8 additions & 0 deletions subsys/usb/class/dfu/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,12 @@ config USB_DFU_DEFAULT_POLLTIMEOUT
help
Default value for bwPollTimeout (in ms)

config USB_DFU_ENABLE_UPLOAD
bool "Enable firmware uploading to the host"
help
Enabling this option allows to upload firmware image to the host.
Be aware that upload capability can be a security risk because
the executable image is always decrypted despite the image
encryption is enabled.

endif # USB_DFU_CLASS
37 changes: 24 additions & 13 deletions subsys/usb/class/dfu/usb_dfu.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,15 @@ LOG_MODULE_REGISTER(usb_dfu);

#define INTERMITTENT_CHECK_DELAY 50

#if IS_ENABLED(CONFIG_USB_DFU_ENABLE_UPLOAD)
#define DFU_DESC_ATTRIBUTES (DFU_ATTR_CAN_DNLOAD | \
DFU_ATTR_CAN_UPLOAD | \
DFU_ATTR_MANIFESTATION_TOLERANT)
#else
#define DFU_DESC_ATTRIBUTES (DFU_ATTR_CAN_DNLOAD | \
DFU_ATTR_MANIFESTATION_TOLERANT)
#endif

static struct k_poll_event dfu_event;
static struct k_poll_signal dfu_signal;
static struct k_timer dfu_timer;
Expand Down Expand Up @@ -100,9 +109,7 @@ USBD_CLASS_DESCR_DEFINE(primary, 0) struct usb_dfu_config dfu_cfg = {
.dfu_descr = {
.bLength = sizeof(struct dfu_runtime_descriptor),
.bDescriptorType = DFU_FUNC_DESC,
.bmAttributes = DFU_ATTR_CAN_DNLOAD |
DFU_ATTR_CAN_UPLOAD |
DFU_ATTR_MANIFESTATION_TOLERANT,
.bmAttributes = DFU_DESC_ATTRIBUTES,
.wDetachTimeOut =
sys_cpu_to_le16(CONFIG_USB_DFU_DETACH_TIMEOUT),
.wTransferSize =
Expand Down Expand Up @@ -191,9 +198,7 @@ struct dev_dfu_mode_descriptor dfu_mode_desc = {
.dfu_descr = {
.bLength = sizeof(struct dfu_runtime_descriptor),
.bDescriptorType = DFU_FUNC_DESC,
.bmAttributes = DFU_ATTR_CAN_DNLOAD |
DFU_ATTR_CAN_UPLOAD |
DFU_ATTR_MANIFESTATION_TOLERANT,
.bmAttributes = DFU_DESC_ATTRIBUTES,
.wDetachTimeOut =
sys_cpu_to_le16(CONFIG_USB_DFU_DETACH_TIMEOUT),
.wTransferSize =
Expand Down Expand Up @@ -308,16 +313,16 @@ struct dfu_data_t {
};

#if FLASH_AREA_LABEL_EXISTS(image_1)
#define UPLOAD_FLASH_AREA_ID FLASH_AREA_ID(image_1)
#define DOWNLOAD_FLASH_AREA_ID FLASH_AREA_ID(image_1)
#else
#define UPLOAD_FLASH_AREA_ID FLASH_AREA_ID(image_0)
#define DOWNLOAD_FLASH_AREA_ID FLASH_AREA_ID(image_0)
#endif


static struct dfu_data_t dfu_data = {
.state = appIDLE,
.status = statusOK,
.flash_area_id = UPLOAD_FLASH_AREA_ID,
.flash_area_id = DOWNLOAD_FLASH_AREA_ID,
.alt_setting = 0,
.bwPollTimeout = CONFIG_USB_DFU_DEFAULT_POLLTIMEOUT,
};
Expand Down Expand Up @@ -427,6 +432,13 @@ static int dfu_class_handle_to_host(struct usb_setup_packet *setup,
LOG_DBG("DFU_UPLOAD block %d, len %d, state %d",
setup->wValue, setup->wLength, dfu_data.state);

if (!IS_ENABLED(CONFIG_USB_DFU_ENABLE_UPLOAD)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a good idea to not even compile in the upload code. Then an upload is impossible even if someone manages to bypass this check. Altho the compiler will probably optimize away the unused code, that does require those optimizations to be enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, my wish would be that underlying flash API simply has no access to this area if not enabled.

LOG_WRN("Firmware uploading is not enabled");
dfu_data.status = errSTALLEDPKT;
dfu_data.state = dfuERROR;
return -ENOTSUP;
}

if (dfu_check_app_state()) {
return -EINVAL;
}
Expand Down Expand Up @@ -562,7 +574,7 @@ static int dfu_class_handle_to_device(struct usb_setup_packet *setup,
dfu_reset_counters();
k_poll_signal_reset(&dfu_signal);

if (dfu_data.flash_area_id != UPLOAD_FLASH_AREA_ID) {
if (dfu_data.flash_area_id != DOWNLOAD_FLASH_AREA_ID) {
dfu_data.status = errWRITE;
dfu_data.state = dfuERROR;
LOG_ERR("This area can not be overwritten");
Expand Down Expand Up @@ -730,8 +742,7 @@ static int dfu_custom_handle_req(struct usb_setup_packet *setup,
break;
#if FLASH_AREA_LABEL_EXISTS(image_1)
case 1:
dfu_data.flash_area_id =
UPLOAD_FLASH_AREA_ID;
dfu_data.flash_area_id = DOWNLOAD_FLASH_AREA_ID;
break;
#endif
default:
Expand Down Expand Up @@ -800,7 +811,7 @@ static void dfu_work_handler(struct k_work *item)
* image collection, so not erase whole bank at DFU beginning
*/
#ifndef CONFIG_IMG_ERASE_PROGRESSIVELY
if (boot_erase_img_bank(UPLOAD_FLASH_AREA_ID)) {
if (boot_erase_img_bank(DOWNLOAD_FLASH_AREA_ID)) {
dfu_data.state = dfuERROR;
dfu_data.status = errERASE;
break;
Expand Down
2 changes: 1 addition & 1 deletion subsys/usb/usb_device.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ static void usb_data_to_host(void)
* last chunk is wMaxPacketSize long, to indicate the last
* packet.
*/
if (!usb_dev.data_buf_residue &&
if (!usb_dev.data_buf_residue && chunk &&
usb_dev.setup.wLength > usb_dev.data_buf_len) {
/* Send less data as requested during the Setup stage */
if (!(usb_dev.data_buf_len % USB_MAX_CTRL_MPS)) {
Expand Down