Thanks Takashi,

I can confirm that this patch fixes the heap overflow issue. Could you kindly submit this patch into the alsa-lib tree?

Best regards.

On Wed, Aug 22, 2018 at 12:52 AM, Takashi Iwai tiwai@suse.de wrote:

On Tue, 21 Aug 2018 23:24:14 +0200, Prashant Malani wrote:

...

Hi,

The Chromium fuzzers detected a potential heap overflow in snd_midi_event_encode_byte() when attempting to encode an invalid data sequence. The potential bug was observed in alsa-lib-1.1.5 (the source seems similar to alsa-lib-1.1.6 so it is likely present there too).

Code to reproduce (condensed to fit in an email):

std::array<int, 4> arr{ {0x0A, 0x0B, 0x0C, 0x0D} }; snd_midi_event_t* encoder; snd_midi_event_new(arr.size(), &encoder); for (int i = 0; i < arr.size(); i++) { snd_seq_event_t event; int result = snd_midi_event_encode_byte(encoder, arr[i], &event); if (result < 0) { // Log error and return.... } if ( result == 1) { // Send the completed message and return. } } ....

The first call to snd_midi_event_encode_byte() using byte 0x0A causes the |encoder->qlen| to underflow and become a large unsigned value, and |encoder->read| to become 2. Subsequent bytes processed will get written to the |encoder->buf| buffer, with |dev->read| getting incremented after every byte. As a result, by the time we get to byte 0x0D, |encoder->read| is already 4, and this results in a heap overflow (relevant line in alsa-lib is src/seq/seq_midi_event.c:425)

I'm not sure if the above input array is a valid input to snd_midi_event_encode_byte(), but I'm guessing we should be doing some error checking to make sure we're not processing incorrect/unexpected/invalid bytes.

Any suggestions about how one can submit a fix for this?

Does the patch below help? The first hunk should suffice for your case. The latter is a similar issue for decoding.

thanks,

Takashi

-- 8< -- diff --git a/src/seq/seq_midi_event.c b/src/seq/seq_midi_event.c index 2e7d1035442a..5a12a18ce781 100644 --- a/src/seq/seq_midi_event.c +++ b/src/seq/seq_midi_event.c @@ -35,7 +35,7 @@

/* midi status */ struct snd_midi_event {

•   size_t qlen;    /* queue length */
  

•   ssize_t qlen;   /* queue length */
    size_t read;    /* chars read */
    int type;       /* current event type */
    unsigned char lastcmd;
  

@@ -606,6 +606,8 @@ long snd_midi_event_decode(snd_midi_event_t *dev, unsigned char *buf, long count status_event[type].decode(ev, xbuf + 0); qlen = status_event[type].qlen; }

•           if (qlen <= 0)
  

•                   return 0;
            if (count < qlen)
                    return -ENOMEM;
            memcpy(buf, xbuf, qlen);
  
  

Thanks.

Curious to know: Whats the URL for alsa-lib git tree? I'd love to be able to check it out.

On Wed, Aug 22, 2018 at 11:46 PM, Takashi Iwai tiwai@suse.de wrote:

On Wed, 22 Aug 2018 23:32:35 +0200, Prashant Malani wrote:

...

Thanks Takashi,

I can confirm that this patch fixes the heap overflow issue. Could you kindly submit this patch into the alsa-lib tree?

Now applied the following patch to git tree.

thanks,

Takashi

-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] seq: Fix signedness in MIDI encoder/decoder

The qlen field of struct snd_midi_event was declared as size_t while status_events[] assigns the qlen to -1 indicating to skip. This leads to the misinterpretation since size_t is unsigned, hence it passes the check "dev.qlen > 0" incorrectly in snd_midi_event_encode_byte(), which eventually results in a memory corruption.

Also, snd_midi_event_decode() doesn't consider about a negative qlen value and tries to copy the size as is.

This patch fixes these issues: the first one is addressed by simply replacing size_t with ssize_t in snd_midi_event struct. For the latter, a check "qlen <= 0" is added to bail out; this is also good as a slight optimization.

Reported-by: Prashant Malani pmalani@chromium.org Signed-off-by: Takashi Iwai tiwai@suse.de

---

src/seq/seq_midi_event.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/seq/seq_midi_event.c b/src/seq/seq_midi_event.c index 2e7d1035442a..5a12a18ce781 100644 --- a/src/seq/seq_midi_event.c +++ b/src/seq/seq_midi_event.c @@ -35,7 +35,7 @@

/* midi status */ struct snd_midi_event {

•   size_t qlen;    /* queue length */
  

•   ssize_t qlen;   /* queue length */
    size_t read;    /* chars read */
    int type;       /* current event type */
    unsigned char lastcmd;
  

@@ -606,6 +606,8 @@ long snd_midi_event_decode(snd_midi_event_t *dev, unsigned char *buf, long count status_event[type].decode(ev, xbuf + 0); qlen = status_event[type].qlen; }

•           if (qlen <= 0)
  

•                   return 0;
            if (count < qlen)
                    return -ENOMEM;
            memcpy(buf, xbuf, qlen);
  

-- 2.18.0

the preferred flow looks something like: - we put together the fix - send fix to upstream (alsa) project - upstream reviews/merges - if we get a new release, update to that - if new release is going to be a while, backport the patch to latest ebuild and send a PR to Gentoo's GH (github.com/gentoo/gentoo) - once Gentoo merges the PR, roll the fix into portage-stable

otherwise if things are up for debate along the way, we have chromiumos-overlay to hold our own forked packages. -mike

On Mon, Aug 27, 2018 at 8:33 AM Takashi Iwai tiwai@suse.de wrote:

On Thu, 23 Aug 2018 22:41:15 +0200, Prashant Malani wrote:

...

Thanks.

Curious to know: Whats the URL for alsa-lib git tree? I'd love to be able to check it out.

You can get it from git.alsa-project.org

Takashi

...

On Wed, Aug 22, 2018 at 11:46 PM, Takashi Iwai tiwai@suse.de wrote:

...

On Wed, 22 Aug 2018 23:32:35 +0200, Prashant Malani wrote:

...

Thanks Takashi,

I can confirm that this patch fixes the heap overflow issue. Could you kindly submit this patch into the alsa-lib tree?

Now applied the following patch to git tree.

thanks,

Takashi

-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] seq: Fix signedness in MIDI encoder/decoder

The qlen field of struct snd_midi_event was declared as size_t while status_events[] assigns the qlen to -1 indicating to skip. This leads to the misinterpretation since size_t is unsigned, hence it passes the check "dev.qlen > 0" incorrectly in snd_midi_event_encode_byte(), which eventually results in a memory corruption.

Also, snd_midi_event_decode() doesn't consider about a negative qlen value and tries to copy the size as is.

This patch fixes these issues: the first one is addressed by simply replacing size_t with ssize_t in snd_midi_event struct. For the latter, a check "qlen <= 0" is added to bail out; this is also good as a slight optimization.

Reported-by: Prashant Malani pmalani@chromium.org Signed-off-by: Takashi Iwai tiwai@suse.de

---

src/seq/seq_midi_event.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/seq/seq_midi_event.c b/src/seq/seq_midi_event.c index 2e7d1035442a..5a12a18ce781 100644 --- a/src/seq/seq_midi_event.c +++ b/src/seq/seq_midi_event.c @@ -35,7 +35,7 @@

/* midi status */ struct snd_midi_event {

•   size_t qlen;    /* queue length */
  

•   ssize_t qlen;   /* queue length */
    size_t read;    /* chars read */
    int type;       /* current event type */
    unsigned char lastcmd;
  

@@ -606,6 +606,8 @@ long snd_midi_event_decode(snd_midi_event_t *dev, unsigned char *buf, long count status_event[type].decode(ev, xbuf +

0);

...

...

                    qlen = status_event[type].qlen;
            }

•           if (qlen <= 0)
  

•                   return 0;
            if (count < qlen)
                    return -ENOMEM;
            memcpy(buf, xbuf, qlen);
  

-- 2.18.0

---

Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel