From 2e47210c1af88b88b6f2d92fe2b6d08117041363 Mon Sep 17 00:00:00 2001 From: Nathaniel Brough Date: Fri, 13 Jan 2023 13:37:55 -0800 Subject: [PATCH 1/2] fix: Replace device calls to memcpy with tu_memcpy_s Introduces a new function tu_memcpy_s, which is effectively a backport of memcpy_s. The change also refactors calls to memcpy over to the more secure tu_memcpy_s. --- src/class/audio/audio_device.c | 7 ++----- src/class/dfu/dfu_rt_device.c | 2 +- src/class/hid/hid_device.c | 18 +++++++----------- src/class/midi/midi_device.c | 2 +- src/class/msc/msc_device.c | 10 +++++----- src/common/tusb_common.h | 8 ++++++++ src/device/dcd.h | 2 +- src/device/usbd_control.c | 4 +++- src/tusb.c | 23 ++++++++++++++++++++++- 9 files changed, 50 insertions(+), 26 deletions(-) diff --git a/src/class/audio/audio_device.c b/src/class/audio/audio_device.c index 698fba566c..81df951397 100644 --- a/src/class/audio/audio_device.c +++ b/src/class/audio/audio_device.c @@ -823,10 +823,7 @@ uint16_t tud_audio_int_ctr_n_write(uint8_t func_id, uint8_t const* buffer, uint1 // We write directly into the EP's buffer - abort if previous transfer not complete TU_VERIFY(!usbd_edpt_busy(_audiod_fct[func_id].rhport, _audiod_fct[func_id].ep_int_ctr)); - // Check length - TU_VERIFY(len <= CFG_TUD_AUDIO_INT_CTR_EP_IN_SW_BUFFER_SIZE); - - memcpy(_audiod_fct[func_id].ep_int_ctr_buf, buffer, len); + TU_VERIFY(tu_memcpy_s(_audiod_fct[func_id].ep_int_ctr_buf, CFG_TUD_AUDIO_INT_CTR_EP_IN_SW_BUFFER_SIZE, buffer, len)==0); // Schedule transmit TU_VERIFY(usbd_edpt_xfer(_audiod_fct[func_id].rhport, _audiod_fct[func_id].ep_int_ctr, _audiod_fct[func_id].ep_int_ctr_buf, len)); @@ -2202,7 +2199,7 @@ bool tud_audio_buffer_and_schedule_control_xfer(uint8_t rhport, tusb_control_req if (len > _audiod_fct[func_id].ctrl_buf_sz) len = _audiod_fct[func_id].ctrl_buf_sz; // Copy into buffer - memcpy((void *)_audiod_fct[func_id].ctrl_buf, data, (size_t)len); + TU_VERIFY(tu_memcpy_s(_audiod_fct[func_id].ctrl_buf, sizeof(_audiod_fct[func_id].ctrl_buf), data, (size_t)len)==0); // Schedule transmit return tud_control_xfer(rhport, p_request, (void*)_audiod_fct[func_id].ctrl_buf, len); diff --git a/src/class/dfu/dfu_rt_device.c b/src/class/dfu/dfu_rt_device.c index b9cd6096bd..7b77b3f8f7 100644 --- a/src/class/dfu/dfu_rt_device.c +++ b/src/class/dfu/dfu_rt_device.c @@ -110,7 +110,7 @@ bool dfu_rtd_control_xfer_cb(uint8_t rhport, uint8_t stage, tusb_control_request TU_LOG2(" DFU RT Request: GETSTATUS\r\n"); dfu_status_response_t resp; // Status = OK, Poll timeout is ignored during RT, State = APP_IDLE, IString = 0 - memset(&resp, 0x00, sizeof(dfu_status_response_t)); + TU_VERIFY(tu_memset_s(&resp, sizeof(resp), 0x00, sizeof(resp))==0); tud_control_xfer(rhport, request, &resp, sizeof(dfu_status_response_t)); } break; diff --git a/src/class/hid/hid_device.c b/src/class/hid/hid_device.c index 8077e4deb3..40654eef91 100644 --- a/src/class/hid/hid_device.c +++ b/src/class/hid/hid_device.c @@ -92,16 +92,12 @@ bool tud_hid_n_report(uint8_t instance, uint8_t report_id, void const* report, u // prepare data if (report_id) { - len = tu_min16(len, CFG_TUD_HID_EP_BUFSIZE-1); - p_hid->epin_buf[0] = report_id; - memcpy(p_hid->epin_buf+1, report, len); + TU_VERIFY(tu_memcpy_s(p_hid->epin_buf+1, CFG_TUD_HID_EP_BUFSIZE-1, report, len)==0); len++; }else { - // If report id = 0, skip ID field - len = tu_min16(len, CFG_TUD_HID_EP_BUFSIZE); - memcpy(p_hid->epin_buf, report, len); + TU_VERIFY(tu_memcpy_s(p_hid->epin_buf, CFG_TUD_HID_EP_BUFSIZE, report, len)==0); } return usbd_edpt_xfer(rhport, p_hid->ep_in, p_hid->epin_buf, len); @@ -126,7 +122,7 @@ bool tud_hid_n_keyboard_report(uint8_t instance, uint8_t report_id, uint8_t modi if ( keycode ) { - memcpy(report.keycode, keycode, 6); + TU_VERIFY(tu_memcpy_s(report.keycode, sizeof(report.keycode), keycode, sizeof(report.keycode))==0); }else { tu_memclr(report.keycode, 6); @@ -151,8 +147,7 @@ bool tud_hid_n_mouse_report(uint8_t instance, uint8_t report_id, } bool tud_hid_n_gamepad_report(uint8_t instance, uint8_t report_id, - int8_t x, int8_t y, int8_t z, int8_t rz, int8_t rx, int8_t ry, uint8_t hat, uint32_t buttons) -{ + int8_t x, int8_t y, int8_t z, int8_t rz, int8_t rx, int8_t ry, uint8_t hat, uint32_t buttons) { hid_gamepad_report_t report = { .x = x, @@ -183,11 +178,12 @@ void hidd_reset(uint8_t rhport) } uint16_t hidd_open(uint8_t rhport, tusb_desc_interface_t const * desc_itf, uint16_t max_len) -{ + { TU_VERIFY(TUSB_CLASS_HID == desc_itf->bInterfaceClass, 0); // len = interface + hid + n*endpoints - uint16_t const drv_len = (uint16_t) (sizeof(tusb_desc_interface_t) + sizeof(tusb_hid_descriptor_hid_t) + + uint16_t const drv_len = + (uint16_t) (sizeof(tusb_desc_interface_t) + sizeof(tusb_hid_descriptor_hid_t) + desc_itf->bNumEndpoints * sizeof(tusb_desc_endpoint_t)); TU_ASSERT(max_len >= drv_len, 0); diff --git a/src/class/midi/midi_device.c b/src/class/midi/midi_device.c index de41706e86..0b52be1817 100644 --- a/src/class/midi/midi_device.c +++ b/src/class/midi/midi_device.c @@ -182,7 +182,7 @@ uint32_t tud_midi_n_stream_read(uint8_t itf, uint8_t cable_num, void* buffer, ui uint8_t const count = (uint8_t) tu_min32(stream->total - stream->index, bufsize); // Skip the header (1st byte) in the buffer - memcpy(buf8, stream->buffer + 1 + stream->index, count); + TU_VERIFY(tu_memcpy_s(buf8, bufsize, stream->buffer + 1 + stream->index, count)==0); total_read += count; stream->index += count; diff --git a/src/class/msc/msc_device.c b/src/class/msc/msc_device.c index 00b0a1d06d..ed4a3e86ce 100644 --- a/src/class/msc/msc_device.c +++ b/src/class/msc/msc_device.c @@ -707,7 +707,7 @@ static int32_t proc_builtin_scsi(uint8_t lun, uint8_t const scsi_cmd[16], uint8_ read_capa10.block_size = tu_htonl(block_size); resplen = sizeof(read_capa10); - memcpy(buffer, &read_capa10, (size_t) resplen); + TU_VERIFY(tu_memcpy_s(buffer, bufsize, &read_capa10, (size_t) resplen)); } } break; @@ -741,7 +741,7 @@ static int32_t proc_builtin_scsi(uint8_t lun, uint8_t const scsi_cmd[16], uint8_ read_fmt_capa.block_size_u16 = tu_htons(block_size); resplen = sizeof(read_fmt_capa); - memcpy(buffer, &read_fmt_capa, (size_t) resplen); + TU_VERIFY(tu_memcpy_s(buffer, bufsize, &read_fmt_capa, (size_t) resplen)==0); } } break; @@ -764,7 +764,7 @@ static int32_t proc_builtin_scsi(uint8_t lun, uint8_t const scsi_cmd[16], uint8_ tud_msc_inquiry_cb(lun, inquiry_rsp.vendor_id, inquiry_rsp.product_id, inquiry_rsp.product_rev); resplen = sizeof(inquiry_rsp); - memcpy(buffer, &inquiry_rsp, (size_t) resplen); + TU_VERIFY(tu_memcpy_s(buffer, bufsize, &inquiry_rsp, (size_t) resplen)==0); } break; @@ -788,7 +788,7 @@ static int32_t proc_builtin_scsi(uint8_t lun, uint8_t const scsi_cmd[16], uint8_ mode_resp.write_protected = !writable; resplen = sizeof(mode_resp); - memcpy(buffer, &mode_resp, (size_t) resplen); + TU_VERIFY(tu_memcpy_s(buffer, bufsize, &mode_resp, (size_t) resplen)==0); } break; @@ -806,7 +806,7 @@ static int32_t proc_builtin_scsi(uint8_t lun, uint8_t const scsi_cmd[16], uint8_ sense_rsp.add_sense_qualifier = p_msc->add_sense_qualifier; resplen = sizeof(sense_rsp); - memcpy(buffer, &sense_rsp, (size_t) resplen); + TU_VERIFY(tu_memcpy_s(buffer, bufsize, &sense_rsp, (size_t) resplen)==0); // request sense callback could overwrite the sense data if (tud_msc_request_sense_cb) diff --git a/src/common/tusb_common.h b/src/common/tusb_common.h index b1ee40a1a5..78fdcfe362 100644 --- a/src/common/tusb_common.h +++ b/src/common/tusb_common.h @@ -83,6 +83,14 @@ #define tu_memclr(buffer, size) memset((buffer), 0, (size)) #define tu_varclr(_var) tu_memclr(_var, sizeof(*(_var))) +// This is a backport of memset_s from c11 +int32_t tu_memset_s(void *dest, size_t destsz, int ch, size_t count); + +// This is a backport of memcpy_s from c11 +int32_t tu_memcpy_s(void *dest, size_t destsz, + const void * src, size_t count ); + + //------------- Bytes -------------// TU_ATTR_ALWAYS_INLINE static inline uint32_t tu_u32(uint8_t b3, uint8_t b2, uint8_t b1, uint8_t b0) { diff --git a/src/device/dcd.h b/src/device/dcd.h index c1780f656b..3a7e6c5df2 100644 --- a/src/device/dcd.h +++ b/src/device/dcd.h @@ -193,7 +193,7 @@ TU_ATTR_ALWAYS_INLINE static inline void dcd_event_bus_reset (uint8_t rhport, t TU_ATTR_ALWAYS_INLINE static inline void dcd_event_setup_received(uint8_t rhport, uint8_t const * setup, bool in_isr) { dcd_event_t event = { .rhport = rhport, .event_id = DCD_EVENT_SETUP_RECEIVED }; - memcpy(&event.setup_received, setup, 8); + memcpy(&event.setup_received, setup, sizeof(tusb_control_request_t)); dcd_event_handler(&event, in_isr); } diff --git a/src/device/usbd_control.c b/src/device/usbd_control.c index 0995ef6697..ce4ddab667 100644 --- a/src/device/usbd_control.c +++ b/src/device/usbd_control.c @@ -93,7 +93,9 @@ static bool _data_stage_xact(uint8_t rhport) if ( _ctrl_xfer.request.bmRequestType_bit.direction == TUSB_DIR_IN ) { ep_addr = EDPT_CTRL_IN; - if ( xact_len ) memcpy(_usbd_ctrl_buf, _ctrl_xfer.buffer, xact_len); + if ( xact_len ) { + TU_VERIFY(tu_memcpy_s(_usbd_ctrl_buf, CFG_TUD_ENDPOINT0_SIZE, _ctrl_xfer.buffer, xact_len)==0); + } } return usbd_edpt_xfer(rhport, ep_addr, xact_len ? _usbd_ctrl_buf : NULL, xact_len); diff --git a/src/tusb.c b/src/tusb.c index a5c820b8d0..c918b0248a 100644 --- a/src/tusb.c +++ b/src/tusb.c @@ -24,6 +24,7 @@ * This file is part of the TinyUSB stack. */ +#include "common/tusb_common.h" #include "tusb_option.h" #if CFG_TUH_ENABLED || CFG_TUD_ENABLED @@ -460,7 +461,7 @@ void tu_print_mem(void const *buf, uint32_t count, uint8_t indent) tu_printf("%04X: ", 16*i/item_per_line); } - memcpy(&value, buf8, size); + tu_memcpy_s(&value, sizeof(value), buf8, size); buf8 += size; tu_printf(" "); @@ -486,3 +487,23 @@ void tu_print_mem(void const *buf, uint32_t count, uint8_t indent) #endif #endif // host or device enabled + +//--------------------------------------------------------------------+ +// Common +//--------------------------------------------------------------------+ + +int32_t tu_memset_s(void *dest, size_t destsz, int ch, size_t count) { + if (count > destsz) { + return -1; + } + memset(dest, ch, count); + return 0; +} + +int32_t tu_memcpy_s(void *dest, size_t destsz, const void *src, size_t count) { + if (count > destsz) { + return -1; + } + memcpy(dest, src, count); + return 0; +} \ No newline at end of file From e34aeb5cf69315d2d7711f0a1d040a28492d026d Mon Sep 17 00:00:00 2001 From: hathach Date: Mon, 27 Feb 2023 09:11:35 +0700 Subject: [PATCH 2/2] minor clean up --- src/class/audio/audio_device.c | 2 +- src/class/hid/hid_device.c | 6 +++--- src/class/midi/midi_device.c | 2 +- src/class/msc/msc_device.c | 10 +++++----- src/common/tusb_common.h | 21 ++++++++++++++++++--- src/device/usbd_control.c | 2 +- src/tusb.c | 21 --------------------- 7 files changed, 29 insertions(+), 35 deletions(-) diff --git a/src/class/audio/audio_device.c b/src/class/audio/audio_device.c index 81df951397..de5a20d6da 100644 --- a/src/class/audio/audio_device.c +++ b/src/class/audio/audio_device.c @@ -2199,7 +2199,7 @@ bool tud_audio_buffer_and_schedule_control_xfer(uint8_t rhport, tusb_control_req if (len > _audiod_fct[func_id].ctrl_buf_sz) len = _audiod_fct[func_id].ctrl_buf_sz; // Copy into buffer - TU_VERIFY(tu_memcpy_s(_audiod_fct[func_id].ctrl_buf, sizeof(_audiod_fct[func_id].ctrl_buf), data, (size_t)len)==0); + TU_VERIFY(0 == tu_memcpy_s(_audiod_fct[func_id].ctrl_buf, sizeof(_audiod_fct[func_id].ctrl_buf), data, (size_t)len)); // Schedule transmit return tud_control_xfer(rhport, p_request, (void*)_audiod_fct[func_id].ctrl_buf, len); diff --git a/src/class/hid/hid_device.c b/src/class/hid/hid_device.c index 40654eef91..37a22b6098 100644 --- a/src/class/hid/hid_device.c +++ b/src/class/hid/hid_device.c @@ -93,11 +93,11 @@ bool tud_hid_n_report(uint8_t instance, uint8_t report_id, void const* report, u if (report_id) { p_hid->epin_buf[0] = report_id; - TU_VERIFY(tu_memcpy_s(p_hid->epin_buf+1, CFG_TUD_HID_EP_BUFSIZE-1, report, len)==0); + TU_VERIFY(0 == tu_memcpy_s(p_hid->epin_buf+1, CFG_TUD_HID_EP_BUFSIZE-1, report, len)); len++; }else { - TU_VERIFY(tu_memcpy_s(p_hid->epin_buf, CFG_TUD_HID_EP_BUFSIZE, report, len)==0); + TU_VERIFY(0 == tu_memcpy_s(p_hid->epin_buf, CFG_TUD_HID_EP_BUFSIZE, report, len)); } return usbd_edpt_xfer(rhport, p_hid->ep_in, p_hid->epin_buf, len); @@ -122,7 +122,7 @@ bool tud_hid_n_keyboard_report(uint8_t instance, uint8_t report_id, uint8_t modi if ( keycode ) { - TU_VERIFY(tu_memcpy_s(report.keycode, sizeof(report.keycode), keycode, sizeof(report.keycode))==0); + memcpy(report.keycode, keycode, sizeof(report.keycode)); }else { tu_memclr(report.keycode, 6); diff --git a/src/class/midi/midi_device.c b/src/class/midi/midi_device.c index 0b52be1817..92689dfb01 100644 --- a/src/class/midi/midi_device.c +++ b/src/class/midi/midi_device.c @@ -182,7 +182,7 @@ uint32_t tud_midi_n_stream_read(uint8_t itf, uint8_t cable_num, void* buffer, ui uint8_t const count = (uint8_t) tu_min32(stream->total - stream->index, bufsize); // Skip the header (1st byte) in the buffer - TU_VERIFY(tu_memcpy_s(buf8, bufsize, stream->buffer + 1 + stream->index, count)==0); + TU_VERIFY(0 == tu_memcpy_s(buf8, bufsize, stream->buffer + 1 + stream->index, count)); total_read += count; stream->index += count; diff --git a/src/class/msc/msc_device.c b/src/class/msc/msc_device.c index ed4a3e86ce..ecc95377ff 100644 --- a/src/class/msc/msc_device.c +++ b/src/class/msc/msc_device.c @@ -707,7 +707,7 @@ static int32_t proc_builtin_scsi(uint8_t lun, uint8_t const scsi_cmd[16], uint8_ read_capa10.block_size = tu_htonl(block_size); resplen = sizeof(read_capa10); - TU_VERIFY(tu_memcpy_s(buffer, bufsize, &read_capa10, (size_t) resplen)); + TU_VERIFY(0 == tu_memcpy_s(buffer, bufsize, &read_capa10, (size_t) resplen)); } } break; @@ -741,7 +741,7 @@ static int32_t proc_builtin_scsi(uint8_t lun, uint8_t const scsi_cmd[16], uint8_ read_fmt_capa.block_size_u16 = tu_htons(block_size); resplen = sizeof(read_fmt_capa); - TU_VERIFY(tu_memcpy_s(buffer, bufsize, &read_fmt_capa, (size_t) resplen)==0); + TU_VERIFY(0 == tu_memcpy_s(buffer, bufsize, &read_fmt_capa, (size_t) resplen)); } } break; @@ -764,7 +764,7 @@ static int32_t proc_builtin_scsi(uint8_t lun, uint8_t const scsi_cmd[16], uint8_ tud_msc_inquiry_cb(lun, inquiry_rsp.vendor_id, inquiry_rsp.product_id, inquiry_rsp.product_rev); resplen = sizeof(inquiry_rsp); - TU_VERIFY(tu_memcpy_s(buffer, bufsize, &inquiry_rsp, (size_t) resplen)==0); + TU_VERIFY(0 == tu_memcpy_s(buffer, bufsize, &inquiry_rsp, (size_t) resplen)); } break; @@ -788,7 +788,7 @@ static int32_t proc_builtin_scsi(uint8_t lun, uint8_t const scsi_cmd[16], uint8_ mode_resp.write_protected = !writable; resplen = sizeof(mode_resp); - TU_VERIFY(tu_memcpy_s(buffer, bufsize, &mode_resp, (size_t) resplen)==0); + TU_VERIFY(0 == tu_memcpy_s(buffer, bufsize, &mode_resp, (size_t) resplen)); } break; @@ -806,7 +806,7 @@ static int32_t proc_builtin_scsi(uint8_t lun, uint8_t const scsi_cmd[16], uint8_ sense_rsp.add_sense_qualifier = p_msc->add_sense_qualifier; resplen = sizeof(sense_rsp); - TU_VERIFY(tu_memcpy_s(buffer, bufsize, &sense_rsp, (size_t) resplen)==0); + TU_VERIFY(0 == tu_memcpy_s(buffer, bufsize, &sense_rsp, (size_t) resplen)); // request sense callback could overwrite the sense data if (tud_msc_request_sense_cb) diff --git a/src/common/tusb_common.h b/src/common/tusb_common.h index 78fdcfe362..3e5e1d4276 100644 --- a/src/common/tusb_common.h +++ b/src/common/tusb_common.h @@ -84,11 +84,26 @@ #define tu_varclr(_var) tu_memclr(_var, sizeof(*(_var))) // This is a backport of memset_s from c11 -int32_t tu_memset_s(void *dest, size_t destsz, int ch, size_t count); +TU_ATTR_ALWAYS_INLINE static inline int tu_memset_s(void *dest, size_t destsz, int ch, size_t count) +{ + // TODO may check if desst and src is not NULL + if (count > destsz) { + return -1; + } + memset(dest, ch, count); + return 0; +} // This is a backport of memcpy_s from c11 -int32_t tu_memcpy_s(void *dest, size_t destsz, - const void * src, size_t count ); +TU_ATTR_ALWAYS_INLINE static inline int tu_memcpy_s(void *dest, size_t destsz, const void * src, size_t count ) +{ + // TODO may check if desst and src is not NULL + if (count > destsz) { + return -1; + } + memcpy(dest, src, count); + return 0; +} //------------- Bytes -------------// diff --git a/src/device/usbd_control.c b/src/device/usbd_control.c index ce4ddab667..b8a2008452 100644 --- a/src/device/usbd_control.c +++ b/src/device/usbd_control.c @@ -94,7 +94,7 @@ static bool _data_stage_xact(uint8_t rhport) { ep_addr = EDPT_CTRL_IN; if ( xact_len ) { - TU_VERIFY(tu_memcpy_s(_usbd_ctrl_buf, CFG_TUD_ENDPOINT0_SIZE, _ctrl_xfer.buffer, xact_len)==0); + TU_VERIFY(0 == tu_memcpy_s(_usbd_ctrl_buf, CFG_TUD_ENDPOINT0_SIZE, _ctrl_xfer.buffer, xact_len)); } } diff --git a/src/tusb.c b/src/tusb.c index c918b0248a..8318e5275f 100644 --- a/src/tusb.c +++ b/src/tusb.c @@ -24,7 +24,6 @@ * This file is part of the TinyUSB stack. */ -#include "common/tusb_common.h" #include "tusb_option.h" #if CFG_TUH_ENABLED || CFG_TUD_ENABLED @@ -487,23 +486,3 @@ void tu_print_mem(void const *buf, uint32_t count, uint8_t indent) #endif #endif // host or device enabled - -//--------------------------------------------------------------------+ -// Common -//--------------------------------------------------------------------+ - -int32_t tu_memset_s(void *dest, size_t destsz, int ch, size_t count) { - if (count > destsz) { - return -1; - } - memset(dest, ch, count); - return 0; -} - -int32_t tu_memcpy_s(void *dest, size_t destsz, const void *src, size_t count) { - if (count > destsz) { - return -1; - } - memcpy(dest, src, count); - return 0; -} \ No newline at end of file