linux-kernel - Re: 回复: [PATCH] ALSA: seq: KASAN: use-after-free Read in delete_and_unsubscribe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <s5hd0486yq3.wl-tiwai@suse.de>
Date:   Mon, 03 Aug 2020 08:16:20 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     "Zhang, Qiang" <Qiang.Zhang@...driver.com>
Cc:     "perex@...ex.cz" <perex@...ex.cz>,
        "tiwai@...e.com" <tiwai@...e.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: 回复: [PATCH] ALSA: seq: KASAN:
 use-after-free Read in delete_and_unsubscribe_port

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