Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 03 Aug 2020 08:16:20 +0200
Message-ID: <s5hd0486yq3.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     "Zhang, Qiang" <Qiang.Zhang@windriver.com>
Cc:     "perex@perex.cz" <perex@perex.cz>,
        "tiwai@suse.com" <tiwai@suse.com>,
        "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: =?UTF-8?B?5Zue5aSNOg==?= [PATCH] ALSA: seq: KASAN:
 use-after-free Read in delete_and_unsubscribe_port
In-Reply-To: <BYAPR11MB2632746756E574DE66BAC427FF4D0@BYAPR11MB2632.namprd11.prod.outlook.com>
References: <20200801062403.8005-1-qiang.zhang@windriver.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 03 Aug 2020 03:35:05 +0200,
Zhang, Qiang wrote:
> 
> >Thanks for the patch.  But I'm afraid that this change would break the
> >existing behavior and might have a bad side-effect.
> 
> >It's likely the same issue as reported in another syzkaller report
> >("KASAN: invalid-free in snd_seq_port_disconnect"), and Hillf's patch
> >below should covert this as well.  Could you check whether it works?
> 
> yes It's should same issue, add mutex lock in odev_ioctl, ensure serialization.
> however, it should not be necessary to mutually exclusive with open and close.

That's a big-hammer approach indeed, but it should be more reasonable
in this case.  It makes the patch shorter and simpler, while the OSS
sequencer is an ancient interface that wasn't considered much for the
concurrency, and this might also cover the case where the access to
another sequencer object that is being to be closed.

So, it'd be great if you can confirm that the patch actually works.
Then we can brush up and merge it for 5.9-rc1.


thanks,

Takashi

> 
> 
> 
> >thanks,
> 
> >Takashi
> 
> >---
> >--- a/sound/core/seq/oss/seq_oss.c
> >+++ b/sound/core/seq/oss/seq_oss.c
> >@@ -167,11 +167,17 @@ odev_write(struct file *file, const char
>  >static long
>  >odev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  >{
> >+       long rc;
> >      struct seq_oss_devinfo *dp;
> >+
> >+       mutex_lock(&register_mutex);
> >       dp = file->private_data;
> >        if (snd_BUG_ON(!dp))
> >-               return -ENXIO;
> >-       return snd_seq_oss_ioctl(dp, cmd, arg);
> >+               rc = -ENXIO;
> >+       else
> >+               rc = snd_seq_oss_ioctl(dp, cmd, arg);
> >+       mutex_unlock(&register_mutex);
> >+       return rc;
>  >}
> 
>  >#ifdef CONFIG_COMPAT