Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220409120901.267526-1-dzm91@hust.edu.cn> <YlQbqnYP/jcYinvz@hovoldconsulting.com>
 <CAD-N9QV=tRxcRH_bD7-3X4sLKYBu4LYDk5tTfUAaX2JDd7nLTg@mail.gmail.com>
In-Reply-To: <CAD-N9QV=tRxcRH_bD7-3X4sLKYBu4LYDk5tTfUAaX2JDd7nLTg@mail.gmail.com>
From:   Dongliang Mu <mudongliangabcd@gmail.com>
Date:   Thu, 14 Apr 2022 22:03:55 +0800
Message-ID: <CAD-N9QXs7+AyGRUgJMe9YGBsUe67QK+y+=Q6OCVABQnwWn+1dA@mail.gmail.com>
Subject: Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free
To:     Johan Hovold <johan@kernel.org>
Cc:     Dongliang Mu <dzm91@hust.edu.cn>,
        Oliver Neukum <oliver@neukum.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        syzbot+eabbf2aaa999cc507108@syzkaller.appspotmail.com,
        USB <linux-usb@vger.kernel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Apr 14, 2022 at 9:58 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> On Mon, Apr 11, 2022 at 8:14 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote:
> > > From: Dongliang Mu <mudongliangabcd@gmail.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().
>
> You mean the following thread interlaving?
>
> ioctl                                unbind
>                                 cdc_ncm_free(ctx);
> dev->data[0]
>                                  dev->data[0] = 0;
>
> It seems this will still trigger UAF. Maybe we need to add mutex to
> prevent this. But I am not sure.

ioctl                                   unbind
                                  cdc_ncm_free(ctx);
                                  dev->data[0] = 0;
dev->data[0]

This will trigger a null pointer dereference if my patch is applied, right?

>
> >
> > Johan
> >
> > > ==================================================================
> > > BUG: KASAN: use-after-free in cdc_ncm_set_dgram_size+0xc91/0xde0
> > > Read of size 8 at addr ffff8880755210b0 by task dhcpcd/3174
> > >
> > > Call Trace:
> > >  <TASK>
> > >  __dump_stack lib/dump_stack.c:88 [inline]
> > >  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> > >  print_address_description.constprop.0.cold+0xeb/0x495 mm/kasan/report.c:313
> > >  print_report mm/kasan/report.c:429 [inline]
> > >  kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491
> > >  cdc_ncm_set_dgram_size+0xc91/0xde0 drivers/net/usb/cdc_ncm.c:608
> > >  cdc_ncm_change_mtu+0x10c/0x140 drivers/net/usb/cdc_ncm.c:798
> > >  __dev_set_mtu net/core/dev.c:8519 [inline]
> > >  dev_set_mtu_ext+0x352/0x5b0 net/core/dev.c:8572
> > >  dev_set_mtu+0x8e/0x120 net/core/dev.c:8596
> > >  dev_ifsioc+0xb87/0x1090 net/core/dev_ioctl.c:332
> > >  dev_ioctl+0x1b9/0xe30 net/core/dev_ioctl.c:586
> > >  sock_do_ioctl+0x15a/0x230 net/socket.c:1136
> > >  sock_ioctl+0x2f1/0x640 net/socket.c:1239
> > >  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/0x80 arch/x86/entry/common.c:80
> > >  entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > RIP: 0033:0x7f00859e70e7
> > > RSP: 002b:00007ffedd503dd8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> > > RAX: ffffffffffffffda RBX: 00007f00858f96c8 RCX: 00007f00859e70e7
> > > RDX: 00007ffedd513fc8 RSI: 0000000000008922 RDI: 0000000000000018
> > > RBP: 00007ffedd524178 R08: 00007ffedd513f88 R09: 00007ffedd513f38
> > > R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> > > R13: 00007ffedd513fc8 R14: 0000000000000028 R15: 0000000000008922
> > >  </TASK>
> >
> > > Reported-by: syzbot+eabbf2aaa999cc507108@syzkaller.appspotmail.com
> > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > ---
> > >  drivers/net/usb/cdc_ncm.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> > > index 15f91d691bba..9fc2df9f0b63 100644
> > > --- a/drivers/net/usb/cdc_ncm.c
> > > +++ b/drivers/net/usb/cdc_ncm.c
> > > @@ -1019,6 +1019,7 @@ void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf)
> > >
> > >       usb_set_intfdata(intf, NULL);
> > >       cdc_ncm_free(ctx);
> > > +     dev->data[0] = 0;
> > >  }
> > >  EXPORT_SYMBOL_GPL(cdc_ncm_unbind);