Re: Bug: slab-out-of-bounds in snd_seq_oss_synth_sysex

[Takashi Iwai <tiwai@suse.de>]

On Mon, 30 Dec 2024 10:47:08 +0100,
Kun Hu wrote:
> 
> 
>     The only place incrementing sysex->len is that loop and it has already
>     the bounce check which resets sysex->len to 0.  That is, the likely
>     reason of the buffer overflow is rather the racy calls of this
>     function.
>    
>     If so, a potential fix would be rather to protect the concurrency,
>     e.g. a simplest form is something like below.
>     Could you check whether it addresses your problem?
> 
>     thanks,
>    
>     Takashi
>    
>     -- 8< --
>     --- a/sound/core/seq/oss/seq_oss_synth.c
>     +++ b/sound/core/seq/oss/seq_oss_synth.c
>     @@ -66,6 +66,7 @@ static struct seq_oss_synth midi_synth_dev = {
>     };
>    
>     static DEFINE_SPINLOCK(register_lock);
>     +static DEFINE_MUTEX(sysex_mutex);
>    
>     /*
>      * prototypes
>     @@ -497,6 +498,7 @@ snd_seq_oss_synth_sysex(struct seq_oss_devinfo *dp,
>     int dev, unsigned char *buf,
>     if (!info)
>     return -ENXIO;
>    
>     + guard(mutex)(&sysex_mutex);
>     sysex = info->sysex;
>     if (sysex == NULL) {
>     sysex = kzalloc(sizeof(*sysex), GFP_KERNEL);
> 
> Hi,
> 
> I did a lot of reproduction testing and it was very easy to reproduce before
> the lock was added, after the lock was added the error was no longer
> reproducible.

Good to hear.  Then I'm going to submit this as a band-aid fix; this
is simple and easy to backport to older stable releases, too.

Meanwhile, we may have a different fix for the long term.  Essentially
the sysex handling in OSS layer is unnecessarily complex, and we can
send a packet immediately at each time instead of combining them.

Could you try the patch below instead of the previous one?


thanks,

Takashi

-- 8< --
diff --git a/sound/core/seq/oss/seq_oss_device.h b/sound/core/seq/oss/seq_oss_device.h
index 98dd20b42976..c0b1cce127a3 100644
--- a/sound/core/seq/oss/seq_oss_device.h
+++ b/sound/core/seq/oss/seq_oss_device.h
@@ -55,7 +55,6 @@ struct seq_oss_chinfo {
 struct seq_oss_synthinfo {
 	struct snd_seq_oss_arg arg;
 	struct seq_oss_chinfo *ch;
-	struct seq_oss_synth_sysex *sysex;
 	int nr_voices;
 	int opened;
 	int is_midi;
diff --git a/sound/core/seq/oss/seq_oss_synth.c b/sound/core/seq/oss/seq_oss_synth.c
index e3394919daa0..02a90c960992 100644
--- a/sound/core/seq/oss/seq_oss_synth.c
+++ b/sound/core/seq/oss/seq_oss_synth.c
@@ -26,13 +26,6 @@
  * definition of synth info records
  */
 
-/* sysex buffer */
-struct seq_oss_synth_sysex {
-	int len;
-	int skip;
-	unsigned char buf[MAX_SYSEX_BUFLEN];
-};
-
 /* synth info */
 struct seq_oss_synth {
 	int seq_device;
@@ -318,8 +311,6 @@ snd_seq_oss_synth_cleanup(struct seq_oss_devinfo *dp)
 			}
 			snd_use_lock_free(&rec->use_lock);
 		}
-		kfree(info->sysex);
-		info->sysex = NULL;
 		kfree(info->ch);
 		info->ch = NULL;
 	}
@@ -395,8 +386,6 @@ snd_seq_oss_synth_reset(struct seq_oss_devinfo *dp, int dev)
 	info = get_synthinfo_nospec(dp, dev);
 	if (!info || !info->opened)
 		return;
-	if (info->sysex)
-		info->sysex->len = 0; /* reset sysex */
 	reset_channels(info);
 	if (info->is_midi) {
 		if (midi_synth_dev.opened <= 0)
@@ -408,8 +397,6 @@ snd_seq_oss_synth_reset(struct seq_oss_devinfo *dp, int dev)
 					  dp->file_mode) < 0) {
 			midi_synth_dev.opened--;
 			info->opened = 0;
-			kfree(info->sysex);
-			info->sysex = NULL;
 			kfree(info->ch);
 			info->ch = NULL;
 		}
@@ -482,63 +469,26 @@ snd_seq_oss_synth_info(struct seq_oss_devinfo *dp, int dev)
 
 /*
  * receive OSS 6 byte sysex packet:
- * the full sysex message will be sent if it reaches to the end of data
- * (0xff).
+ * the event is filled and prepared for sending immediately
+ * (i.e. sysex messages are fragmented)
  */
 int
 snd_seq_oss_synth_sysex(struct seq_oss_devinfo *dp, int dev, unsigned char *buf, struct snd_seq_event *ev)
 {
-	int i, send;
-	unsigned char *dest;
-	struct seq_oss_synth_sysex *sysex;
-	struct seq_oss_synthinfo *info;
+	unsigned char *p;
+	int len = 6;
 
-	info = snd_seq_oss_synth_info(dp, dev);
-	if (!info)
-		return -ENXIO;
+	p = memchr(buf, 0xff, 6);
+	if (p)
+		len = p - buf + 1;
 
-	sysex = info->sysex;
-	if (sysex == NULL) {
-		sysex = kzalloc(sizeof(*sysex), GFP_KERNEL);
-		if (sysex == NULL)
-			return -ENOMEM;
-		info->sysex = sysex;
-	}
-
-	send = 0;
-	dest = sysex->buf + sysex->len;
-	/* copy 6 byte packet to the buffer */
-	for (i = 0; i < 6; i++) {
-		if (buf[i] == 0xff) {
-			send = 1;
-			break;
-		}
-		dest[i] = buf[i];
-		sysex->len++;
-		if (sysex->len >= MAX_SYSEX_BUFLEN) {
-			sysex->len = 0;
-			sysex->skip = 1;
-			break;
-		}
-	}
-
-	if (sysex->len && send) {
-		if (sysex->skip) {
-			sysex->skip = 0;
-			sysex->len = 0;
-			return -EINVAL; /* skip */
-		}
-		/* copy the data to event record and send it */
-		ev->flags = SNDRV_SEQ_EVENT_LENGTH_VARIABLE;
-		if (snd_seq_oss_synth_addr(dp, dev, ev))
-			return -EINVAL;
-		ev->data.ext.len = sysex->len;
-		ev->data.ext.ptr = sysex->buf;
-		sysex->len = 0;
-		return 0;
-	}
-
-	return -EINVAL; /* skip */
+	/* copy the data to event record and send it */
+	if (snd_seq_oss_synth_addr(dp, dev, ev))
+		return -EINVAL;
+	ev->flags = SNDRV_SEQ_EVENT_LENGTH_VARIABLE;
+	ev->data.ext.len = len;
+	ev->data.ext.ptr = buf;
+	return 0;
 }
 
 /*