[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

[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.