netdev - Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm

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: <CAHp75VeTqmdLhavZ+VbBYSFMDHr0FG4iKFGdbzE-wo5MCNikAA@mail.gmail.com>
Date:   Thu, 14 Apr 2022 18:01:57 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Johan Hovold <johan@...nel.org>,
        Oleksij Rempel <linux@...pel-privat.de>
Cc:     Dongliang Mu <dzm91@...t.edu.cn>,
        Oliver Neukum <oliver@...kum.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Dongliang Mu <mudongliangabcd@...il.com>,
        syzbot+eabbf2aaa999cc507108@...kaller.appspotmail.com,
        USB <linux-usb@...r.kernel.org>, netdev <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

On Mon, Apr 11, 2022 at 9:33 PM Johan Hovold <johan@...nel.org> wrote:
> On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote:
> > From: Dongliang Mu <mudongliangabcd@...il.com>
> >
> > cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0]
> > with ctx. However, in the unbind function - cdc_ncm_unbind,
> > it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as
> > a dangling pointer. The following ioctl operation will trigger
> > the UAF in the function cdc_ncm_set_dgram_size.
> >
> > Fix this by setting dev->data[0] as zero.
>
> This sounds like a poor band-aid. Please explain how this prevent the
> ioctl() from racing with unbind().

Good question. Isn't it the commit 2c9d6c2b871d ("usbnet: run unbind()
before unregister_netdev()") which changed the ordering of the
interface shutdown and basically makes this race happen? I don't see
how we can guarantee that IOCTL won't be called until we quiescence
the network device — my understanding that on device surprise removal
we have to first shutdown what it created and then unbind the device.
If I understand the original issue correctly then the problem is in
usbnet->unbind and it should actually be split to two hooks, otherwise
it seems every possible IOCTL callback must have some kind of
reference counting and keep an eye on the surprise removal.

Johan, can you correct me if my understanding is wrong?

-- 
With Best Regards,
Andy Shevchenko