diff mbox series

ALSA: firewire-lib: allocate additional entries for list of packet descriptor to avoid out-of-bounds access

Message ID 20190825065247.26840-1-o-takashi@sakamocchi.jp (mailing list archive)
State New, archived
Headers show
Series ALSA: firewire-lib: allocate additional entries for list of packet descriptor to avoid out-of-bounds access | expand

Commit Message

Takashi Sakamoto Aug. 25, 2019, 6:52 a.m. UTC
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(-)

Comments

Takashi Iwai Aug. 25, 2019, 7:15 a.m. UTC | #1
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).

Care to fix and resend?


thanks,

Takashi
Takashi Sakamoto Aug. 25, 2019, 8:41 a.m. UTC | #2
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
diff mbox series

Patch

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;