| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87pll9wi4x.wl-tiwai@suse.de>
Date: Mon, 30 Dec 2024 11:59:58 +0100
From: Takashi Iwai <tiwai@...e.de>
To: Kun Hu <huk23@...udan.edu.cn>
Cc: Takashi Iwai <tiwai@...e.de>,
perex@...ex.cz,
tiwai@...e.com,
linux-sound@...r.kernel.org,
linux-kernel@...r.kernel.org,
"jjtan24@...udan.edu.cn" <jjtan24@...udan.edu.cn>
Subject: Re: Bug: slab-out-of-bounds in snd_seq_oss_synth_sysex
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;
}
/*
Powered by blists - more mailing lists