[PATCH] ALSA: line6: Bound capture copies by the PCM buffer size

[PATCH] ALSA: line6: Bound capture copies by the PCM buffer size

[Sinan Turunc]

line6_capture_copy() splits the copy into the ALSA ring buffer once at
the buffer end and wraps pos_done once, which is only correct when one
packet holds at most buffer_size frames. The packet size comes from the
device's endpoint, and the per-device hw structs allow 64-byte periods
with periods_min 1, so a packet can be larger than the whole buffer.
audio_in_callback() logs an oversized packet and then copies it anyway.

Clamp frames to buffer_size before the copy so the single wrap stays
inside the buffer.

Reproduced on 7.3-rc7 with a USB/IP device presenting the GuitarPort
interface layout and 1024-byte packets into a 128-byte buffer: ALSA
logged "invalid position: pos = 224, buffer size = 32" and the driver
logged "driver bug: len" on every following packet.

Fixes: 705ececd1c60 ("Staging: add line6 usb driver")
Cc: stable@vger.kernel.org
Signed-off-by: Sinan Turunc <enscrpt@pm.me>
---
 sound/usb/line6/capture.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sound/usb/line6/capture.c b/sound/usb/line6/capture.c
index 6dbaf30232f..7d8e7e83eeb 100644
--- a/sound/usb/line6/capture.c
+++ b/sound/usb/line6/capture.c
@@ -94,6 +94,17 @@ void line6_capture_copy(struct snd_line6_pcm *line6pcm, char *fbuf, int fsize)
 	if (runtime == NULL)
 		return;
 
+	/*
+	 * A packet can carry up to max_packet_size_in, which the device
+	 * chooses, while the ring buffer can be as small as one 64-byte
+	 * period. The single wrap below is only valid for
+	 * frames <= buffer_size, so keep what fits.
+	 */
+	if (unlikely(frames > runtime->buffer_size)) {
+		frames = runtime->buffer_size;
+		fsize = frames * bytes_per_frame;
+	}
+
 	if (line6pcm->in.pos_done + frames > runtime->buffer_size) {
 		/*
 		   The transferred area goes over buffer boundary,
-- 
2.56.0

Re: [PATCH] ALSA: line6: Bound capture copies by the PCM buffer size

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: line6: clamping frames to buffer_size in line6_capture_copy() stalls PCM
--

commit dece7d69941f7080a104841df1e03f3fae6b7577
Author: Sinan Turunc <enscrpt@pm.me>

ALSA: line6: Bound capture copies by the PCM buffer size

This patch clamps the number of captured frames in line6_capture_copy() to
runtime->buffer_size so that oversized incoming USB packets do not overflow
the single ring-buffer wrap calculation.

> diff --git a/sound/usb/line6/capture.c b/sound/usb/line6/capture.c
> index 17f49f567efbf..8b6a0dd2ddde2 100644
> --- a/sound/usb/line6/capture.c
> +++ b/sound/usb/line6/capture.c
> @@ -94,6 +94,17 @@ void line6_capture_copy(struct snd_line6_pcm *line6pcm, char *fbuf, int fsize)
>  	if (runtime == NULL)
>  		return;
>  
> +	/*
> +	 * A packet can carry up to max_packet_size_in, which the device
> +	 * chooses, while the ring buffer can be as small as one 64-byte
> +	 * period. The single wrap below is only valid for
> +	 * frames <= buffer_size, so keep what fits.
> +	 */
> +	if (unlikely(frames > runtime->buffer_size)) {
> +		frames = runtime->buffer_size;
> +		fsize = frames * bytes_per_frame;
> +	}

[Severity: High]
Does clamping frames to runtime->buffer_size here cause the capture stream
to stall whenever an incoming packet exceeds the buffer size (for example,
when a user configures a 1-period 64-byte capture buffer of 11 or 16 frames
and receives a regular 44- or 48-frame 1 ms packet or a larger USB/IP
packet)?

At the end of line6_capture_copy(), pos_done is updated with:

sound/usb/line6/capture.c:line6_capture_copy() {
	...
	line6pcm->in.pos_done += frames;
	if (line6pcm->in.pos_done >= runtime->buffer_size)
		line6pcm->in.pos_done -= runtime->buffer_size;
}

When frames is clamped to runtime->buffer_size, adding and immediately
subtracting runtime->buffer_size leaves line6pcm->in.pos_done unchanged on
every packet.

When audio_in_callback() then calls snd_pcm_period_elapsed(),
snd_pcm_update_hw_ptr0() reads the same position via snd_line6_pointer().
In the interrupt path, snd_pcm_update_hw_ptr0() only treats an unchanged
pointer as a full-buffer wrap when hdelta exceeds
runtime->hw_ptr_buffer_jiffies / 2 + 1:

sound/core/pcm_lib.c:snd_pcm_update_hw_ptr0() {
	...
	if (in_interrupt) {
		/* we know that one period was processed */
		/* delta = "expected next hw_ptr" for in_interrupt != 0 */
		delta = runtime->hw_ptr_interrupt + runtime->period_size;
		if (delta > new_hw_ptr) {
			/* check for double acknowledged interrupts */
			hdelta = curr_jiffies - runtime->hw_ptr_jiffies;
			if (hdelta > runtime->hw_ptr_buffer_jiffies/2 + 1) {
				hw_base += runtime->buffer_size;
				...
			}
		}
	}
	/* new_hw_ptr might be lower than old_hw_ptr in case of */
	/* pointer crosses the end of the ring buffer */
	if (runtime->status->hw_ptr == new_hw_ptr) {
		runtime->hw_ptr_jiffies = curr_jiffies;
		update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
		return 0;
	}
}

Because isochronous URBs complete every 1 ms (or 125 us), hdelta is at most
1 jiffy, so snd_pcm_update_hw_ptr0() takes the
runtime->status->hw_ptr == new_hw_ptr return path and refreshes
runtime->hw_ptr_jiffies on every URB without ever advancing hw_ptr or waking
user space.

Also, fsize is not read again after frames is computed at the top of
line6_capture_copy(), so is the assignment to fsize here unused?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103120723.328-1-enscrpt@pm.me?part=1

Re: [PATCH] ALSA: line6: Bound capture copies by the PCM buffer size

[Sinan Turunc]

> Does clamping frames to runtime->buffer_size here cause the capture
> stream to stall whenever an incoming packet exceeds the buffer size
> (for example, when a user configures a 1-period 64-byte capture buffer
> of 11 or 16 frames and receives a regular 44- or 48-frame 1 ms packet
> or a larger USB/IP packet)?

Yes. Clamping frames to buffer_size adds exactly one buffer to pos_done
and then subtracts it, so the ring position never advances and the
stream makes no progress. The same holds for a buffer of exactly one
packet, so the bound has to be two packets.

v2 takes a different approach: snd_line6_capture_open() now constrains
SNDRV_PCM_HW_PARAM_BUFFER_BYTES to at least twice the larger of the two
endpoints' packet sizes, so hw_params rejects a buffer that cannot hold
two packets, and line6_capture_copy() keeps only a guard that drops an
oversized packet with a ratelimited error instead of clamping it, for
the case that should no longer be reachable.

> Also, fsize is not read again after frames is computed at the top of
> line6_capture_copy(), so is the assignment to fsize here unused?

fsize is read in the single-chunk branch (memcpy(..., fbuf, fsize)),
which runs when pos_done is zero, so the assignment was used. It goes
away in v2 with the clamp.

v2 tested on 7.3-rc7 against 1024-byte packets: a 1024-byte buffer is
now refused at hw_params with -EINVAL, and a 2048-byte buffer streams
for 30 s with no KASAN report, with hw_ptr sampled once a second at
positions 0 and 256 inside the 512-frame buffer and advancing by about
450000 frames per second.