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

[Takashi Iwai <tiwai@xxxxxxx>]

On Wed, 25 Dec 2024 06:37:47 +0100,
Kun Hu wrote:
> 
> Hello,
> 
> > BUG: KASAN: slab-out-of-bounds in snd_seq_oss_synth_sysex+0x5d1/0x6c0 sound/core/seq/oss/seq_oss_synth.c:516
> 
> We further analyzed the issue at line 516 in ./sound/core/seq/oss/seq_oss_synth.c. 
> The slab-out-of-bounds crash occurs in line 509, when sysex->len = 128. Specifically, the write operation to dest[0] accesses memory beyond the bounds of sysex->buf (128 byte).
> To resolve this issue, we suggest adding 6 lines of code to validate the legality of the address write to sysex->buf before entering the loop:
> 
> if (sysex->len >= MAX_SYSEX_BUFLEN) { 
>    sysex->len = 0; 
>    sysex->skip = 1; 
>    return -EINVAL;  /* Exit early if sysex->len is out of bounds */ 
> }

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);