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

[Takashi Iwai <tiwai@suse.de>]

On Mon, 30 Dec 2024 14:10:24 +0100,
Kun Hu wrote:
> 
> 
> > 
> > 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.

OK, that's convincing enough.
I'm going to submit the proper patch.  It won't be queued for 6.13 but
for 6.14, though.


thanks,

Takashi