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:   Tue, 11 Oct 2022 09:06:33 +0200
Message-ID: <87k056dekm.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Mauro Carvalho Chehab <mchehab@kernel.org>
Cc:     Hyunwoo Kim <imv4bel@gmail.com>, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] media: dvb-core: Fix UAF due to refcount races at releasing
In-Reply-To: <87sfklgozd.wl-tiwai@suse.de>
References: <20220908132754.30532-1-tiwai@suse.de>
        <87sfklgozd.wl-tiwai@suse.de>
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 Wed, 21 Sep 2022 09:34:30 +0200,
Takashi Iwai wrote:
> 
> On Thu, 08 Sep 2022 15:27:54 +0200,
> Takashi Iwai wrote:
> > 
> > The dvb-core tries to sync the releases of opened files at
> > dvb_dmxdev_release() with two refcounts: dvbdev->users and
> > dvr_dvbdev->users.  A problem is present in those two syncs: when yet
> > another dvb_demux_open() is called during those sync waits,
> > dvb_demux_open() continues to process even if the device is being
> > closed.  This includes the increment of the former refcount, resulting
> > in the leftover refcount after the sync of the latter refcount at
> > dvb_dmxdev_release().  It ends up with use-after-free, since the
> > function believes that all usages were gone and releases the
> > resources.
> > 
> > This patch addresses the problem by adding the check of dmxdev->exit
> > flag at dvb_demux_open(), just like dvb_dvr_open() already does.  With
> > the exit flag check, the second call of dvb_demux_open() fails, hence
> > the further corruption can be avoided.
> > 
> > Also for avoiding the races of the dmxdev->exit flag reference, this
> > patch serializes the dmxdev->exit set up and the sync waits with the
> > dmxdev->mutex lock at dvb_dmxdev_release().  Without the mutex lock,
> > dvb_demux_open() (or dvb_dvr_open()) may run concurrently with
> > dvb_dmxdev_release(), which allows to skip the exit flag check and
> > continue the open process that is being closed.
> > 
> > Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Any review on this?
> 
> FWIW, now CVE-2022-41218 is assigned for those bugs as a security
> issue.

A gentle ping again.

Or if any other fix for this security issue is already available,
please let me know.


thanks,

Takashi

> 
> 
> thanks,
> 
> Takashi
> 
> > ---
> >  drivers/media/dvb-core/dmxdev.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c
> > index f6ee678107d3..9ce5f010de3f 100644
> > --- a/drivers/media/dvb-core/dmxdev.c
> > +++ b/drivers/media/dvb-core/dmxdev.c
> > @@ -790,6 +790,11 @@ static int dvb_demux_open(struct inode *inode, struct file *file)
> >  	if (mutex_lock_interruptible(&dmxdev->mutex))
> >  		return -ERESTARTSYS;
> >  
> > +	if (dmxdev->exit) {
> > +		mutex_unlock(&dmxdev->mutex);
> > +		return -ENODEV;
> > +	}
> > +
> >  	for (i = 0; i < dmxdev->filternum; i++)
> >  		if (dmxdev->filter[i].state == DMXDEV_STATE_FREE)
> >  			break;
> > @@ -1448,7 +1453,10 @@ EXPORT_SYMBOL(dvb_dmxdev_init);
> >  
> >  void dvb_dmxdev_release(struct dmxdev *dmxdev)
> >  {
> > +	mutex_lock(&dmxdev->mutex);
> >  	dmxdev->exit = 1;
> > +	mutex_unlock(&dmxdev->mutex);
> > +
> >  	if (dmxdev->dvbdev->users > 1) {
> >  		wait_event(dmxdev->dvbdev->wait_queue,
> >  				dmxdev->dvbdev->users == 1);
> > -- 
> > 2.35.3
> >