Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 05 Sep 2022 08:06:40 +0200
Message-ID: <87zgfejqxb.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     butt3rflyh4ck <butterflyhuangxx@gmail.com>
Cc:     cuibixuan@linux.alibaba.com, LKML <linux-kernel@vger.kernel.org>,
        alsa-devel@alsa-project.org, tiwai@suse.com
Subject: Re: A new null-ptr-deref Write bug in snd_pcm_format_set_silence
In-Reply-To: <CAFcO6XPxCN7DEXbUvtRTKq+ku0OJA21gCz_dqMacDpsNUrsUhg@mail.gmail.com>
References: <CAFcO6XN7JDM4xSXGhtusQfS2mSBcx50VJKwQpCq=WeLt57aaZA@mail.gmail.com>
        <87edwrl9i8.wl-tiwai@suse.de>
        <CAFcO6XPxCN7DEXbUvtRTKq+ku0OJA21gCz_dqMacDpsNUrsUhg@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Sun, 04 Sep 2022 19:06:05 +0200,
butt3rflyh4ck wrote:
> 
> I have tested this patch, it works well.

Great, I'm going to submit a proper patch.


thanks,

Takashi


> Regards,
>  butt3rflyh4ck.
> 
> On Sun, Sep 4, 2022 at 6:27 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Sun, 04 Sep 2022 11:48:37 +0200,
> > butt3rflyh4ck wrote:
> > >
> > > Hi, there is a new null-ptr-deref Write bug in
> > > snd_pcm_format_set_slience in sound/core/pcm_misc.c in the latest
> > > upstream kernel and can reproduce it.
> > > We call SNDCTL_DSP_SYNC and SNDCTL_DSP_SPEED in multiple threads to
> > > trigger the vulnerability.
> > >
> > > See the Call Trace:
> > > ==================================================================
> > > Call Trace:
> > >  <TASK>
> > >  __dump_stack lib/dump_stack.c:88 [inline]
> > >  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> > >  kasan_report+0xb1/0x1e0 mm/kasan/report.c:495
> > >  check_region_inline mm/kasan/generic.c:183 [inline]
> > >  kasan_check_range+0x13d/0x180 mm/kasan/generic.c:189
> > >  memset+0x20/0x40 mm/kasan/shadow.c:44
> > >  snd_pcm_format_set_silence sound/core/pcm_misc.c:441 [inline]
> > >  snd_pcm_format_set_silence+0x215/0x350 sound/core/pcm_misc.c:424
> > >  snd_pcm_oss_sync+0x60e/0x800 sound/core/oss/pcm_oss.c:1690
> > >  snd_pcm_oss_ioctl+0x2087/0x3420 sound/core/oss/pcm_oss.c:2634
> > >  vfs_ioctl fs/ioctl.c:51 [inline]
> > >  __do_sys_ioctl fs/ioctl.c:870 [inline]
> > >  __se_sys_ioctl fs/ioctl.c:856 [inline]
> > >  __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856
> > >  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > >  do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> > >  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > > ==================================================================
> > > We can see the function snd_pcm_format_set_silence code below:
> > > ```
> > > int snd_pcm_format_set_silence(snd_pcm_format_t format, void *data,
> > > unsigned int samples)
> > > {
> > >         int width;
> > >         unsigned char *dst;
> > >         const unsigned char *pat;
> > >
> > >         if (!valid_format(format))
> > >                 return -EINVAL;
> > >         if (samples == 0)
> > >                 return 0;
> > >         width = pcm_formats[(INT)format].phys; /* physical width */
> > >         pat = pcm_formats[(INT)format].silence;
> > >         if (!width || !pat)
> > >                 return -EINVAL;
> > >         /* signed or 1 byte data */
> > >         if (pcm_formats[(INT)format].signd == 1 || width <= 8) {
> > >                 unsigned int bytes = samples * width / 8;
> > >                 memset(data, *pat, bytes);    ///// [1] ---------> data is NULL
> > >                 return 0;
> > >         }
> > >        ......
> > > }
> > > ```
> > > [1], the data pointer is NULL, we can know snd_pcm_format_set_silence
> > > called in line 1690 in sound/core/oss/pcm_oss.c from call stack trace.
> > > let we see code below:
> > > ```
> > > static int snd_pcm_oss_sync(struct snd_pcm_oss_file *pcm_oss_file)
> > > {
> > >         int err = 0;
> > >         unsigned int saved_f_flags;
> > >         struct snd_pcm_substream *substream;
> > >         struct snd_pcm_runtime *runtime;
> > >         snd_pcm_format_t format;
> > >         unsigned long width;
> > >         size_t size;
> > >
> > >         substream = pcm_oss_file->streams[SNDRV_PCM_STREAM_PLAYBACK];
> > >         if (substream != NULL) {
> > >                 runtime = substream->runtime;
> > >                 if (atomic_read(&substream->mmap_count))
> > >                         goto __direct;
> > >                 err = snd_pcm_oss_make_ready(substream);
> > >                 if (err < 0)
> > >                         return err;
> > >                 atomic_inc(&runtime->oss.rw_ref);
> > >                 if (mutex_lock_interruptible(&runtime->oss.params_lock)) {
> > >                         atomic_dec(&runtime->oss.rw_ref);
> > >                         return -ERESTARTSYS;
> > >                 }
> > >                 format = snd_pcm_oss_format_from(runtime->oss.format);
> > >                 width = snd_pcm_format_physical_width(format);
> > >                 if (runtime->oss.buffer_used > 0) {
> > > #ifdef OSS_DEBUG
> > >                         pcm_dbg(substream->pcm, "sync: buffer_used\n");
> > > #endif
> > >                         size = (8 * (runtime->oss.period_bytes -
> > > runtime->oss.buffer_used) + 7) / width;
> > >                         snd_pcm_format_set_silence(format,
> > >                                                    runtime->oss.buffer
> > > + runtime->oss.buffer_used,   ///// [2]
> > >                                                    size);
> > >                         err = snd_pcm_oss_sync1(substream,
> > > runtime->oss.period_bytes);
> > >                         if (err < 0)
> > >                                 goto unlock;
> > >                 } else if (runtime->oss.period_ptr > 0) {
> > >
> > > ```
> > > [2]  runtime->oss.buffer + runtime->oss.buffer_used is the data
> > > pointer, but runtime->oss.buffer is NULL here but it doesn't make
> > > sense.
> > > runtime->oss.buffter is allocated by kvzalloc, if runtime->oss_buffer
> > > is NULL, it would return an ENOMEM error.
> > > Maybe I think there is a race condition, the runtime->oss.buffer is
> > > freed and set to NULL but we can use runtime->oss.buffter via ioctl.
> >
> > Yeah, likely it's in a small race window between two calls.
> >
> > Could you try the patch below?
> >
> >
> > thanks,
> >
> > Takashi
> >
> > ---
> > --- a/sound/core/oss/pcm_oss.c
> > +++ b/sound/core/oss/pcm_oss.c
> > @@ -1672,14 +1672,14 @@ static int snd_pcm_oss_sync(struct snd_pcm_oss_file *pcm_oss_file)
> >                 runtime = substream->runtime;
> >                 if (atomic_read(&substream->mmap_count))
> >                         goto __direct;
> > -               err = snd_pcm_oss_make_ready(substream);
> > -               if (err < 0)
> > -                       return err;
> >                 atomic_inc(&runtime->oss.rw_ref);
> >                 if (mutex_lock_interruptible(&runtime->oss.params_lock)) {
> >                         atomic_dec(&runtime->oss.rw_ref);
> >                         return -ERESTARTSYS;
> >                 }
> > +               err = snd_pcm_oss_make_ready_locked(substream);
> > +               if (err < 0)
> > +                       goto unlock;
> >                 format = snd_pcm_oss_format_from(runtime->oss.format);
> >                 width = snd_pcm_format_physical_width(format);
> >                 if (runtime->oss.buffer_used > 0) {
> 
> 
> 
> -- 
> Active Defense Lab of Venustech
>