[alsa-devel] [PATCH] ALSA: firewire-lib: allocate additional entries for list of packet descriptor to avoid out-of-bounds access
This patch is for Linux v5.3-rc7.
In a case of delay to execute scheduled tasklet for isoc context, it's possible to handle queued packets than 16 (=INTERRUPT_INTERVAL). In the case, out-of-bounds access occurs because the list of packet descriptor is allocated just for 16 packets. This causes any negative effects in kernel memory or software IRQ context.
It's quite rare because current implementation allows user processes to flush the queued packet in process context by executing several PCM ioctl(2) commands. However, it's good to prevent.
This commit is a prevention against this bug. For safe, the allocation is done for 16 plus 12 packets, equivalent to 1.5 msec plus. Furthermore, when detecting the case, packet streaming is cancelled and kernel log is printed to notice to users and developers.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/amdtp-stream.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c index 1a92855c7647..f03321888997 100644 --- a/sound/firewire/amdtp-stream.c +++ b/sound/firewire/amdtp-stream.c @@ -56,6 +56,11 @@ #define INTERRUPT_INTERVAL 16 #define QUEUE_LENGTH 48
+// For jitter of software IRQ execution, keep more entries for the list +// of packet descriptor equivalent to 1.5 msec to avoid out-of-bounds +// access to process queued packets. +#define DESC_COUNT (INTERRUPT_INTERVAL + 12) + // For iso header, tstamp and 2 CIP header. #define IR_CTX_HEADER_SIZE_CIP 16 // For iso header and tstamp. @@ -779,12 +784,22 @@ static void out_stream_callback(struct fw_iso_context *context, u32 tstamp, { struct amdtp_stream *s = private_data; const __be32 *ctx_header = header; - unsigned int packets = header_length / sizeof(*ctx_header); + unsigned int packets; int i;
if (s->packet_index < 0) return;
+ // The number of packets in buffer. + packets = header_length / sizeof(*ctx_header); + if (packets > DESC_COUNT) { + cancel_stream(s); + dev_info(&s->unit->device, + "out-stream: Unexpected count of packet: %d\n", + packets); + return; + } + generate_ideal_pkt_descs(s, s->pkt_descs, ctx_header, packets);
process_ctx_payloads(s, s->pkt_descs, packets); @@ -830,6 +845,13 @@ static void in_stream_callback(struct fw_iso_context *context, u32 tstamp,
// The number of packets in buffer. packets = header_length / s->ctx_data.tx.ctx_header_size; + if (packets > DESC_COUNT) { + cancel_stream(s); + dev_info(&s->unit->device, + "in-stream: Unexpected count of packet: %d\n", + packets); + return; + }
err = generate_device_pkt_descs(s, s->pkt_descs, ctx_header, packets); if (err < 0) { @@ -981,8 +1003,7 @@ static int amdtp_stream_start(struct amdtp_stream *s, int channel, int speed) else s->tag = TAG_CIP;
- s->pkt_descs = kcalloc(INTERRUPT_INTERVAL, sizeof(*s->pkt_descs), - GFP_KERNEL); + s->pkt_descs = kcalloc(DESC_COUNT, sizeof(*s->pkt_descs), GFP_KERNEL); if (!s->pkt_descs) { err = -ENOMEM; goto err_context;
Hi,
On Sun, Aug 25, 2019 at 09:15:38AM +0200, Takashi Iwai wrote:
On Sun, 25 Aug 2019 08:52:47 +0200, Takashi Sakamoto wrote:
This patch is for Linux v5.3-rc7.
The patch doesn't seem cleanly applicable to for-linus branch (or 5.3-rc6).
Oops. Against the commit comment, I rebased this patch onto your 'for-next' branch...
Care to fix and resend?
Yes, but in this time would you please abandon this patch. With a bit consideration after the posting, I concluded that there's another side of the out-of-bounds bug. It comes from patchset for AMDTP domain which I'm working for v5.4. I'd like to investigate and work further for the bug.
Thanks and sorry to bother your work.
Takashi Sakamoto
participants (2)
-
Takashi Iwai
-
Takashi Sakamoto