linux-kernel - Re: Bug: slab-out-of-bounds in snd_seq_oss_synth

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <182E26F6-E0BC-44D3-A0E7-124C81B7F8EB@m.fudan.edu.cn>
Date: Mon, 30 Dec 2024 21:10:24 +0800
From: Kun Hu <huk23@...udan.edu.cn>
To: Takashi Iwai <tiwai@...e.de>
Cc: 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


> 
> 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;
> }
> 
> /*

Hi,

Regarding your changes, I’ve conducted multiple rounds of race condition testing using the same method. The original issue, caused by competition for shared resources, could consistently reproduce an out-of-bounds error in just a few attempts. The temporary band-aid fix and the latest changes have been tested at least 10,000 times, and no issues have occurred so far.

——
Thanks,
Kun Hu