From c61e9fb96def7b77b4001374b362141a29dcba5b Mon Sep 17 00:00:00 2001 From: Jerzy Kasenberg Date: Mon, 24 Aug 2020 09:01:03 +0200 Subject: [PATCH 1/4] audio_device: Fix descriptor limit calculation In several place p_desc_end calculation was not taking into account that starting pointer (_audiod_itf[idxDriver].p_desc) was pointing past interface association descriptor. It would result in accessing random memory. --- src/class/audio/audio_device.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/class/audio/audio_device.c b/src/class/audio/audio_device.c index 6fe5d671..f8676d4b 100644 --- a/src/class/audio/audio_device.c +++ b/src/class/audio/audio_device.c @@ -689,7 +689,7 @@ static bool audiod_set_interface(uint8_t rhport, tusb_control_request_t const * // Open new EP if necessary - EPs are only to be closed or opened for AS interfaces - Look for AS interface with correct alternate interface // Get pointer at end - uint8_t const *p_desc_end = _audiod_itf[idxDriver].p_desc + tud_audio_desc_lengths[idxDriver]; + uint8_t const *p_desc_end = _audiod_itf[idxDriver].p_desc + tud_audio_desc_lengths[idxDriver] - TUD_AUDIO_DESC_IAD_LEN; // p_desc starts at required interface with alternate setting zero while (p_desc < p_desc_end) @@ -1113,7 +1113,7 @@ static bool audiod_get_AS_interface_index(uint8_t itf, uint8_t *idxDriver, uint8 if (_audiod_itf[i].p_desc) { // Get pointer at end - uint8_t const *p_desc_end = _audiod_itf[i].p_desc + tud_audio_desc_lengths[i]; + uint8_t const *p_desc_end = _audiod_itf[i].p_desc + tud_audio_desc_lengths[i] - TUD_AUDIO_DESC_IAD_LEN; // Advance past AC descriptors uint8_t const *p_desc = tu_desc_next(_audiod_itf[i].p_desc); @@ -1178,7 +1178,7 @@ static bool audiod_verify_itf_exists(uint8_t itf, uint8_t *idxDriver) { // Get pointer at beginning and end uint8_t const *p_desc = _audiod_itf[i].p_desc; - uint8_t const *p_desc_end = _audiod_itf[i].p_desc + tud_audio_desc_lengths[i]; + uint8_t const *p_desc_end = _audiod_itf[i].p_desc + tud_audio_desc_lengths[i] - TUD_AUDIO_DESC_IAD_LEN; while (p_desc < p_desc_end) { From a4c096be377dd57554a67db92dd3d3167475c142 Mon Sep 17 00:00:00 2001 From: Jerzy Kasenberg Date: Tue, 25 Aug 2020 13:15:43 +0200 Subject: [PATCH 2/4] audio_device: Fix FIFO element size discrepancies Buffer for TX and RX FIFO was not taking into account size of element leading to out of bound access. audio_tx_done_type_I_pcm_ff_cb() reported copied bytes was not returning correct value number if channels was omitted in computation. Transfer size calculation uses simpler arithmetic. --- src/class/audio/audio_device.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/class/audio/audio_device.c b/src/class/audio/audio_device.c index f8676d4b..1f266082 100644 --- a/src/class/audio/audio_device.c +++ b/src/class/audio/audio_device.c @@ -80,7 +80,7 @@ typedef struct // FIFO #if CFG_TUD_AUDIO_EPSIZE_IN && CFG_TUD_AUDIO_TX_FIFO_SIZE tu_fifo_t tx_ff[CFG_TUD_AUDIO_N_CHANNELS_TX]; - CFG_TUSB_MEM_ALIGN uint8_t tx_ff_buf[CFG_TUD_AUDIO_N_CHANNELS_TX][CFG_TUD_AUDIO_TX_FIFO_SIZE]; + CFG_TUSB_MEM_ALIGN uint8_t tx_ff_buf[CFG_TUD_AUDIO_N_CHANNELS_TX][CFG_TUD_AUDIO_TX_FIFO_SIZE * CFG_TUD_AUDIO_TX_ITEMSIZE]; #if CFG_FIFO_MUTEX osal_mutex_def_t tx_ff_mutex[CFG_TUD_AUDIO_N_CHANNELS_TX]; #endif @@ -88,7 +88,7 @@ typedef struct #if CFG_TUD_AUDIO_EPSIZE_OUT && CFG_TUD_AUDIO_RX_FIFO_SIZE tu_fifo_t rx_ff[CFG_TUD_AUDIO_N_CHANNELS_RX]; - CFG_TUSB_MEM_ALIGN uint8_t rx_ff_buf[CFG_TUD_AUDIO_N_CHANNELS_RX][CFG_TUD_AUDIO_RX_FIFO_SIZE]; + CFG_TUSB_MEM_ALIGN uint8_t rx_ff_buf[CFG_TUD_AUDIO_N_CHANNELS_RX][CFG_TUD_AUDIO_RX_FIFO_SIZE * CFG_TUD_AUDIO_RX_ITEMSIZE]; #if CFG_FIFO_MUTEX osal_mutex_def_t rx_ff_mutex[CFG_TUD_AUDIO_N_CHANNELS_RX]; #endif @@ -404,10 +404,12 @@ static bool audio_tx_done_type_I_pcm_ff_cb(uint8_t rhport, audiod_interface_t* a TU_VERIFY(!usbd_edpt_busy(rhport, audio->ep_in)); // Determine amount of samples - uint16_t nSamplesPerChannelToSend = 0xFFFF; + uint16_t const nEndpointSampleCapacity = CFG_TUD_AUDIO_EPSIZE_IN / CFG_TUD_AUDIO_N_CHANNELS_TX / CFG_TUD_AUDIO_N_BYTES_PER_SAMPLE_TX; + uint16_t nSamplesPerChannelToSend = tu_fifo_count(&audio->tx_ff[0]); + uint16_t nBytesToSend; uint8_t cntChannel; - for (cntChannel = 0; cntChannel < CFG_TUD_AUDIO_N_CHANNELS_TX; cntChannel++) + for (cntChannel = 1; cntChannel < CFG_TUD_AUDIO_N_CHANNELS_TX; cntChannel++) { if (audio->tx_ff[cntChannel].count < nSamplesPerChannelToSend) { @@ -423,10 +425,8 @@ static bool audio_tx_done_type_I_pcm_ff_cb(uint8_t rhport, audiod_interface_t* a } // Limit to maximum sample number - THIS IS A POSSIBLE ERROR SOURCE IF TOO MANY SAMPLE WOULD NEED TO BE SENT BUT CAN NOT! - if (nSamplesPerChannelToSend * CFG_TUD_AUDIO_N_CHANNELS_TX * CFG_TUD_AUDIO_N_BYTES_PER_SAMPLE_TX > CFG_TUD_AUDIO_EPSIZE_IN) - { - nSamplesPerChannelToSend = CFG_TUD_AUDIO_EPSIZE_IN / CFG_TUD_AUDIO_N_CHANNELS_TX / CFG_TUD_AUDIO_N_BYTES_PER_SAMPLE_TX; - } + nSamplesPerChannelToSend = min(nSamplesPerChannelToSend, nEndpointSampleCapacity); + nBytesToSend = nSamplesPerChannelToSend * CFG_TUD_AUDIO_N_CHANNELS_TX * CFG_TUD_AUDIO_N_BYTES_PER_SAMPLE_TX; // Encode uint16_t cntSample; @@ -456,8 +456,8 @@ static bool audio_tx_done_type_I_pcm_ff_cb(uint8_t rhport, audiod_interface_t* a } // Schedule transmit - TU_VERIFY(usbd_edpt_xfer(rhport, audio->ep_in, audio->epin_buf, nSamplesPerChannelToSend*CFG_TUD_AUDIO_N_BYTES_PER_SAMPLE_TX)); - *n_bytes_copied = nSamplesPerChannelToSend*CFG_TUD_AUDIO_N_BYTES_PER_SAMPLE_TX; + TU_VERIFY(usbd_edpt_xfer(rhport, audio->ep_in, audio->epin_buf, nBytesToSend)); + *n_bytes_copied = nBytesToSend; return true; } From a3eff0c51a8b5ad58fa7111e770e3b5b3d959351 Mon Sep 17 00:00:00 2001 From: Jerzy Kasenberg Date: Tue, 25 Aug 2020 14:30:02 +0200 Subject: [PATCH 3/4] audio_device: Fix NULL pointer access in audiod_xfer_cb b_bytes_copied was pointer with NULL value instead of plain variable. NULL pointer was passed to audio_tx_done_cb() and dereference as well. Now variable is not a pointer. --- src/class/audio/audio_device.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/class/audio/audio_device.c b/src/class/audio/audio_device.c index 1f266082..cbb13afc 100644 --- a/src/class/audio/audio_device.c +++ b/src/class/audio/audio_device.c @@ -1002,10 +1002,10 @@ bool audiod_xfer_cb(uint8_t rhport, uint8_t ep_addr, xfer_result_t result, uint3 // This is the only place where we can fill something into the EPs buffer! // Load new data - uint16_t *n_bytes_copied = NULL; - TU_VERIFY(audio_tx_done_cb(rhport, &_audiod_itf[idxDriver], n_bytes_copied)); + uint16_t n_bytes_copied; + TU_VERIFY(audio_tx_done_cb(rhport, &_audiod_itf[idxDriver], &n_bytes_copied)); - if (*n_bytes_copied == 0) + if (n_bytes_copied == 0) { // Load with ZLP return usbd_edpt_xfer(rhport, ep_addr, NULL, 0); From b1f0d6f57eb14aec8c5d83bd54a70a56558d24e8 Mon Sep 17 00:00:00 2001 From: Jerzy Kasenberg Date: Tue, 25 Aug 2020 14:41:50 +0200 Subject: [PATCH 4/4] audio_device: Change CFG_TUD_AUDIO_TX_BUFSIZE to CFG_TUD_AUDIO_TX_FIFO_SIZE CFG_TUD_AUDIO_TX_BUFSIZE seems to be used only in 3 preprocessor condition while in other places CFG_TUD_AUDIO_TX_FIFO_SIZE is used. --- src/class/audio/audio_device.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/class/audio/audio_device.h b/src/class/audio/audio_device.h index c23cf72c..41356c61 100644 --- a/src/class/audio/audio_device.h +++ b/src/class/audio/audio_device.h @@ -171,7 +171,7 @@ uint16_t tud_audio_n_read (uint8_t itf, uint8_t channelId, void* buffer, u void tud_audio_n_read_flush (uint8_t itf, uint8_t channelId); #endif -#if CFG_TUD_AUDIO_EPSIZE_IN && CFG_TUD_AUDIO_TX_BUFSIZE +#if CFG_TUD_AUDIO_EPSIZE_IN && CFG_TUD_AUDIO_TX_FIFO_SIZE uint16_t tud_audio_n_write (uint8_t itf, uint8_t channelId, uint8_t const* buffer, uint16_t bufsize); #endif @@ -194,7 +194,7 @@ inline uint16_t tud_audio_read (void* buffer, uint16_t bufsize); inline void tud_audio_read_flush (void); #endif -#if CFG_TUD_AUDIO_EPSIZE_IN && CFG_TUD_AUDIO_TX_BUFSIZE +#if CFG_TUD_AUDIO_EPSIZE_IN && CFG_TUD_AUDIO_TX_FIFO_SIZE inline uint16_t tud_audio_write (uint8_t channelId, uint8_t const* buffer, uint16_t bufsize); #endif @@ -263,12 +263,12 @@ inline bool tud_audio_mounted(void) return tud_audio_n_mounted(0); } -#if CFG_TUD_AUDIO_EPSIZE_IN && CFG_TUD_AUDIO_TX_BUFSIZE +#if CFG_TUD_AUDIO_EPSIZE_IN && CFG_TUD_AUDIO_TX_FIFO_SIZE inline uint16_t tud_audio_write (uint8_t channelId, uint8_t const* buffer, uint16_t bufsize) // Short version if only one audio function is used { return tud_audio_n_write(0, channelId, buffer, bufsize); } -#endif // CFG_TUD_AUDIO_EPSIZE_IN && CFG_TUD_AUDIO_TX_BUFSIZE +#endif // CFG_TUD_AUDIO_EPSIZE_IN && CFG_TUD_AUDIO_TX_FIFO_SIZE #if CFG_TUD_AUDIO_EPSIZE_OUT && CFG_TUD_AUDIO_RX_BUFSIZE inline uint16_t tud_audio_available(uint8_t channelId)