Re: Suspected heap overflow in snd_midi_event_encode_byte()

[Mike Frysinger <vapier@chromium.org>]

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