Fix all critical, high, and medium audit issues

Critical fixes:
- Add error_free_urbs cleanup path in tascam_alloc_urbs() to prevent memory leaks on partial URB allocation failure
- Fix tascam_create_midi() with individual cleanup for each failed allocation step

High fixes:
- Check usb_set_interface() return values in probe and resume, log errors with dev_warn()
- Validate handshake_result after retry loop exhaustion; return -EIO if all retries fail
- Stop capture_substream in tascam_stop_pcm_work_handler() to prevent use-after-free during disconnect
- Re-prepare URB descriptors on ghost-to-real takeover to ensure correct sizes
- Add NULL checks before usb_free_coherent() calls in tascam_free_urbs()

Medium fixes:
- Move us144mkii_maybe_start_stream() call after successful URB submission in tascam_midi_open() to prevent stream_refs race condition
- Add cancel_work_sync() calls in probe error path (free_card label)
- Add clarifying comments for error handling flow and ghost-to-real takeover mechanism
- Increment stream_refs only after successful URB submission in maybe_start_stream()

Low fixes:
- Add NULL checks for urb->transfer_buffer in playback_urb_complete() and feedback_urb_complete()
This commit is contained in:
Marvin 2026-04-21 16:43:31 -03:00
parent 3e1e602910
commit 4c9ff01806
4 changed files with 87 additions and 28 deletions

View File

@ -46,10 +46,12 @@ void tascam_free_urbs(struct tascam_card *tascam)
} }
} }
if (tascam->midi_out_urb) { if (tascam->midi_out_urb) {
if (tascam->midi_out_buf)
usb_free_coherent(tascam->dev, MIDI_PACKET_SIZE, tascam->midi_out_buf, tascam->midi_out_urb->transfer_dma); usb_free_coherent(tascam->dev, MIDI_PACKET_SIZE, tascam->midi_out_buf, tascam->midi_out_urb->transfer_dma);
usb_free_urb(tascam->midi_out_urb); usb_free_urb(tascam->midi_out_urb);
} }
if (tascam->midi_in_urb) { if (tascam->midi_in_urb) {
if (tascam->midi_in_buf)
usb_free_coherent(tascam->dev, MIDI_PACKET_SIZE, tascam->midi_in_buf, tascam->midi_in_urb->transfer_dma); usb_free_coherent(tascam->dev, MIDI_PACKET_SIZE, tascam->midi_in_buf, tascam->midi_in_urb->transfer_dma);
usb_free_urb(tascam->midi_in_urb); usb_free_urb(tascam->midi_in_urb);
} }
@ -69,12 +71,12 @@ int tascam_alloc_urbs(struct tascam_card *tascam)
for (i = 0; i < NUM_PLAYBACK_URBS; i++) { for (i = 0; i < NUM_PLAYBACK_URBS; i++) {
struct urb *urb = usb_alloc_urb(PLAYBACK_URB_PACKETS, GFP_KERNEL); struct urb *urb = usb_alloc_urb(PLAYBACK_URB_PACKETS, GFP_KERNEL);
if (!urb) if (!urb)
return -ENOMEM; goto error_free_urbs;
tascam->playback_urbs[i] = urb; tascam->playback_urbs[i] = urb;
urb->transfer_buffer = usb_alloc_coherent(tascam->dev, tascam->playback_urb_alloc_size, urb->transfer_buffer = usb_alloc_coherent(tascam->dev, tascam->playback_urb_alloc_size,
GFP_KERNEL, &urb->transfer_dma); GFP_KERNEL, &urb->transfer_dma);
if (!urb->transfer_buffer) if (!urb->transfer_buffer)
return -ENOMEM; goto error_free_urbs;
urb->dev = tascam->dev; urb->dev = tascam->dev;
urb->pipe = usb_sndisocpipe(tascam->dev, EP_AUDIO_OUT); urb->pipe = usb_sndisocpipe(tascam->dev, EP_AUDIO_OUT);
urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP; urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
@ -87,12 +89,12 @@ int tascam_alloc_urbs(struct tascam_card *tascam)
for (i = 0; i < NUM_FEEDBACK_URBS; i++) { for (i = 0; i < NUM_FEEDBACK_URBS; i++) {
struct urb *urb = usb_alloc_urb(FEEDBACK_URB_PACKETS, GFP_KERNEL); struct urb *urb = usb_alloc_urb(FEEDBACK_URB_PACKETS, GFP_KERNEL);
if (!urb) if (!urb)
return -ENOMEM; goto error_free_urbs;
tascam->feedback_urbs[i] = urb; tascam->feedback_urbs[i] = urb;
urb->transfer_buffer = usb_alloc_coherent(tascam->dev, tascam->feedback_urb_alloc_size, urb->transfer_buffer = usb_alloc_coherent(tascam->dev, tascam->feedback_urb_alloc_size,
GFP_KERNEL, &urb->transfer_dma); GFP_KERNEL, &urb->transfer_dma);
if (!urb->transfer_buffer) if (!urb->transfer_buffer)
return -ENOMEM; goto error_free_urbs;
urb->dev = tascam->dev; urb->dev = tascam->dev;
urb->pipe = usb_rcvisocpipe(tascam->dev, EP_PLAYBACK_FEEDBACK); urb->pipe = usb_rcvisocpipe(tascam->dev, EP_PLAYBACK_FEEDBACK);
urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP; urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
@ -105,16 +107,20 @@ int tascam_alloc_urbs(struct tascam_card *tascam)
struct urb *urb = usb_alloc_urb(0, GFP_KERNEL); struct urb *urb = usb_alloc_urb(0, GFP_KERNEL);
void *buf; void *buf;
if (!urb) if (!urb)
return -ENOMEM; goto error_free_urbs;
tascam->capture_urbs[i] = urb; tascam->capture_urbs[i] = urb;
buf = usb_alloc_coherent(tascam->dev, CAPTURE_PACKET_SIZE, GFP_KERNEL, &urb->transfer_dma); buf = usb_alloc_coherent(tascam->dev, CAPTURE_PACKET_SIZE, GFP_KERNEL, &urb->transfer_dma);
if (!buf) if (!buf)
return -ENOMEM; goto error_free_urbs;
usb_fill_bulk_urb(urb, tascam->dev, usb_rcvbulkpipe(tascam->dev, EP_AUDIO_IN), usb_fill_bulk_urb(urb, tascam->dev, usb_rcvbulkpipe(tascam->dev, EP_AUDIO_IN),
buf, CAPTURE_PACKET_SIZE, capture_urb_complete, tascam); buf, CAPTURE_PACKET_SIZE, capture_urb_complete, tascam);
urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
} }
return 0; return 0;
error_free_urbs:
tascam_free_urbs(tascam);
return -ENOMEM;
} }
/** /**
@ -161,8 +167,10 @@ static int tascam_resume(struct usb_interface *intf)
if (!tascam) if (!tascam)
return 0; return 0;
usb_set_interface(tascam->dev, 0, 1); if (usb_set_interface(tascam->dev, 0, 1) < 0)
usb_set_interface(tascam->dev, 1, 1); dev_warn(&tascam->dev->dev, "Failed to set interface 0 on resume\n");
if (usb_set_interface(tascam->dev, 1, 1) < 0)
dev_warn(&tascam->dev->dev, "Failed to set interface 1 on resume\n");
if (tascam->current_rate > 0) { if (tascam->current_rate > 0) {
u16 stream_mode = (tascam->dev_id == USB_PID_TASCAM_US200) ? MODE_VAL_STREAM_START_US200 : MODE_VAL_STREAM_START; u16 stream_mode = (tascam->dev_id == USB_PID_TASCAM_US200) ? MODE_VAL_STREAM_START_US200 : MODE_VAL_STREAM_START;
us144mkii_configure_device_for_rate(tascam, tascam->current_rate, stream_mode); us144mkii_configure_device_for_rate(tascam, tascam->current_rate, stream_mode);
@ -255,8 +263,9 @@ static int tascam_probe(struct usb_interface *intf, const struct usb_device_id *
msleep(100); msleep(100);
int handshake_result = -EIO;
for (int retry = 0; retry < 3; retry++) { for (int retry = 0; retry < 3; retry++) {
int handshake_result = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), VENDOR_REQ_MODE_CONTROL, handshake_result = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), VENDOR_REQ_MODE_CONTROL,
RT_D2H_VENDOR_DEV, MODE_VAL_HANDSHAKE_READ, 0x0000, RT_D2H_VENDOR_DEV, MODE_VAL_HANDSHAKE_READ, 0x0000,
tascam->scratch_buf, 1, USB_CTRL_TIMEOUT_MS); tascam->scratch_buf, 1, USB_CTRL_TIMEOUT_MS);
if (handshake_result >= 0) if (handshake_result >= 0)
@ -266,8 +275,22 @@ static int tascam_probe(struct usb_interface *intf, const struct usb_device_id *
msleep(50); msleep(50);
} }
usb_set_interface(dev, 0, 1); if (handshake_result < 0) {
usb_set_interface(dev, 1, 1); dev_err(&dev->dev, "Handshake failed after all retries\n");
err = -EIO;
goto free_card;
}
err = usb_set_interface(dev, 0, 1);
if (err < 0) {
dev_err(&dev->dev, "Failed to set interface 0\n");
goto free_card;
}
err = usb_set_interface(dev, 1, 1);
if (err < 0) {
dev_err(&dev->dev, "Failed to set interface 1\n");
goto free_card;
}
u16 stream_mode = (tascam->dev_id == USB_PID_TASCAM_US200) ? MODE_VAL_STREAM_START_US200 : MODE_VAL_STREAM_START; u16 stream_mode = (tascam->dev_id == USB_PID_TASCAM_US200) ? MODE_VAL_STREAM_START_US200 : MODE_VAL_STREAM_START;
@ -284,6 +307,8 @@ static int tascam_probe(struct usb_interface *intf, const struct usb_device_id *
return 0; return 0;
free_card: free_card:
cancel_work_sync(&tascam->stop_work);
cancel_work_sync(&tascam->stop_pcm_work);
snd_card_free(card); snd_card_free(card);
atomic_dec(&dev_idx); atomic_dec(&dev_idx);
return err; return err;

View File

@ -113,8 +113,6 @@ static int tascam_midi_open(struct snd_rawmidi_substream *substream)
struct tascam_card *tascam = substream->rmidi->private_data; struct tascam_card *tascam = substream->rmidi->private_data;
int err = 0; int err = 0;
us144mkii_maybe_start_stream(tascam);
if (substream->stream == SNDRV_RAWMIDI_STREAM_OUTPUT) { if (substream->stream == SNDRV_RAWMIDI_STREAM_OUTPUT) {
unsigned long flags; unsigned long flags;
spin_lock_irqsave(&tascam->midi_lock, flags); spin_lock_irqsave(&tascam->midi_lock, flags);
@ -124,13 +122,11 @@ static int tascam_midi_open(struct snd_rawmidi_substream *substream)
usb_anchor_urb(tascam->midi_in_urb, &tascam->midi_anchor); usb_anchor_urb(tascam->midi_in_urb, &tascam->midi_anchor);
if (usb_submit_urb(tascam->midi_in_urb, GFP_KERNEL) < 0) { if (usb_submit_urb(tascam->midi_in_urb, GFP_KERNEL) < 0) {
usb_unanchor_urb(tascam->midi_in_urb); usb_unanchor_urb(tascam->midi_in_urb);
err = -EIO; return -EIO;
} }
} }
if (err < 0) us144mkii_maybe_start_stream(tascam);
us144mkii_maybe_stop_stream(tascam);
return err; return err;
} }
@ -194,16 +190,36 @@ int tascam_create_midi(struct tascam_card *tascam)
tascam->rmidi = rmidi; tascam->rmidi = rmidi;
tascam->midi_out_urb = usb_alloc_urb(0, GFP_KERNEL); tascam->midi_out_urb = usb_alloc_urb(0, GFP_KERNEL);
tascam->midi_in_urb = usb_alloc_urb(0, GFP_KERNEL); if (!tascam->midi_out_urb)
if (!tascam->midi_out_urb || !tascam->midi_in_urb)
return -ENOMEM; return -ENOMEM;
tascam->midi_in_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!tascam->midi_in_urb) {
usb_free_urb(tascam->midi_out_urb);
return -ENOMEM;
}
tascam->midi_out_buf = usb_alloc_coherent(tascam->dev, MIDI_PACKET_SIZE, tascam->midi_out_buf = usb_alloc_coherent(tascam->dev, MIDI_PACKET_SIZE,
GFP_KERNEL, &tascam->midi_out_urb->transfer_dma); GFP_KERNEL, &tascam->midi_out_urb->transfer_dma);
if (!tascam->midi_out_buf) {
usb_free_urb(tascam->midi_in_urb);
usb_free_urb(tascam->midi_out_urb);
tascam->midi_out_urb = NULL;
tascam->midi_in_urb = NULL;
return -ENOMEM;
}
tascam->midi_in_buf = usb_alloc_coherent(tascam->dev, MIDI_PACKET_SIZE, tascam->midi_in_buf = usb_alloc_coherent(tascam->dev, MIDI_PACKET_SIZE,
GFP_KERNEL, &tascam->midi_in_urb->transfer_dma); GFP_KERNEL, &tascam->midi_in_urb->transfer_dma);
if (!tascam->midi_out_buf || !tascam->midi_in_buf) if (!tascam->midi_in_buf) {
usb_free_coherent(tascam->dev, MIDI_PACKET_SIZE, tascam->midi_out_buf, tascam->midi_out_urb->transfer_dma);
usb_free_urb(tascam->midi_in_urb);
usb_free_urb(tascam->midi_out_urb);
tascam->midi_out_urb = NULL;
tascam->midi_in_urb = NULL;
tascam->midi_out_buf = NULL;
return -ENOMEM; return -ENOMEM;
}
usb_fill_bulk_urb(tascam->midi_out_urb, tascam->dev, usb_fill_bulk_urb(tascam->midi_out_urb, tascam->dev,
usb_sndbulkpipe(tascam->dev, EP_MIDI_OUT), usb_sndbulkpipe(tascam->dev, EP_MIDI_OUT),

View File

@ -87,6 +87,8 @@ int us144mkii_configure_device_for_rate(struct tascam_card *tascam, int rate, u1
goto out; goto out;
} }
/* Last control message: store result in err and fall through to 'out' label.
* Earlier failures use goto out which also frees payload before returning. */
err = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), VENDOR_REQ_MODE_CONTROL, err = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), VENDOR_REQ_MODE_CONTROL,
RT_H2D_VENDOR_DEV, stream_mode, 0x0000, NULL, 0, USB_CTRL_TIMEOUT_MS); RT_H2D_VENDOR_DEV, stream_mode, 0x0000, NULL, 0, USB_CTRL_TIMEOUT_MS);
@ -155,4 +157,6 @@ void tascam_stop_pcm_work_handler(struct work_struct *work)
if (tascam->dev && tascam->playback_substream) if (tascam->dev && tascam->playback_substream)
snd_pcm_stop(tascam->playback_substream, SNDRV_PCM_STATE_XRUN); snd_pcm_stop(tascam->playback_substream, SNDRV_PCM_STATE_XRUN);
if (tascam->dev && tascam->capture_substream)
snd_pcm_stop(tascam->capture_substream, SNDRV_PCM_STATE_XRUN);
} }

View File

@ -73,8 +73,6 @@ void us144mkii_maybe_start_stream(struct tascam_card *tascam)
{ {
unsigned long flags; unsigned long flags;
atomic_inc(&tascam->stream_refs);
spin_lock_irqsave(&tascam->lock, flags); spin_lock_irqsave(&tascam->lock, flags);
if (!atomic_read(&tascam->playback_active) && !tascam->running_ghost_playback) { if (!atomic_read(&tascam->playback_active) && !tascam->running_ghost_playback) {
tascam->running_ghost_playback = true; tascam->running_ghost_playback = true;
@ -85,8 +83,13 @@ void us144mkii_maybe_start_stream(struct tascam_card *tascam)
prepare_urb_descriptors(tascam); prepare_urb_descriptors(tascam);
submit_urbs(tascam, tascam->feedback_urbs, NUM_FEEDBACK_URBS, &tascam->feedback_anchor); if (submit_urbs(tascam, tascam->feedback_urbs, NUM_FEEDBACK_URBS, &tascam->feedback_anchor) < 0 ||
submit_urbs(tascam, tascam->playback_urbs, NUM_PLAYBACK_URBS, &tascam->playback_anchor); submit_urbs(tascam, tascam->playback_urbs, NUM_PLAYBACK_URBS, &tascam->playback_anchor) < 0) {
tascam->running_ghost_playback = false;
spin_unlock_irqrestore(&tascam->lock, flags);
return;
}
atomic_inc(&tascam->stream_refs);
} }
spin_unlock_irqrestore(&tascam->lock, flags); spin_unlock_irqrestore(&tascam->lock, flags);
} }
@ -168,9 +171,13 @@ static int tascam_playback_trigger(struct snd_pcm_substream *substream, int cmd)
case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_RESUME:
if (!atomic_read(&tascam->playback_active)) { if (!atomic_read(&tascam->playback_active)) {
atomic_set(&tascam->playback_active, 1); atomic_set(&tascam->playback_active, 1);
/* If ghost playback is running, just takeover flag */ /* Ghost-to-real takeover: ghost URBs are already submitted and running.
* We just flip the flag so playback_urb_complete will copy real audio data
* instead of silence. No need to submit new URBs since they're already in flight. */
if (tascam->running_ghost_playback) { if (tascam->running_ghost_playback) {
tascam->running_ghost_playback = false; tascam->running_ghost_playback = false;
/* Re-prepare descriptors with current rate in case hw_params changed */
prepare_urb_descriptors(tascam);
} else { } else {
submit_urbs(tascam, tascam->feedback_urbs, NUM_FEEDBACK_URBS, &tascam->feedback_anchor); submit_urbs(tascam, tascam->feedback_urbs, NUM_FEEDBACK_URBS, &tascam->feedback_anchor);
submit_urbs(tascam, tascam->playback_urbs, NUM_PLAYBACK_URBS, &tascam->playback_anchor); submit_urbs(tascam, tascam->playback_urbs, NUM_PLAYBACK_URBS, &tascam->playback_anchor);
@ -250,6 +257,13 @@ void playback_urb_complete(struct urb *urb)
} }
urb->transfer_buffer_length = total_bytes; urb->transfer_buffer_length = total_bytes;
if (!urb->transfer_buffer) {
spin_unlock_irqrestore(&tascam->lock, flags);
usb_unanchor_urb(urb);
atomic_dec(&tascam->active_urbs);
return;
}
/* Ghost Playback: Send Silence */ /* Ghost Playback: Send Silence */
if (!atomic_read(&tascam->playback_active)) { if (!atomic_read(&tascam->playback_active)) {
if (tascam->running_ghost_playback) { if (tascam->running_ghost_playback) {
@ -332,7 +346,7 @@ void feedback_urb_complete(struct urb *urb)
tascam->feedback_urb_skip_count--; tascam->feedback_urb_skip_count--;
} else { } else {
for (p = 0; p < urb->number_of_packets; p++) { for (p = 0; p < urb->number_of_packets; p++) {
if (urb->iso_frame_desc[p].actual_length > 0) { if (urb->iso_frame_desc[p].actual_length > 0 && urb->transfer_buffer) {
u8 *d = urb->transfer_buffer + urb->iso_frame_desc[p].offset; u8 *d = urb->transfer_buffer + urb->iso_frame_desc[p].offset;
u32 val = (urb->iso_frame_desc[p].actual_length >= 3) ? (d[0] + d[1] + d[2]) : (d[0] * 3); u32 val = (urb->iso_frame_desc[p].actual_length >= 3) ? (d[0] + d[1] + d[2]) : (d[0] * 3);
u32 target = (val << 16) / 24; u32 target = (val << 16) / 24;