Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Andy Shevchenko]

On Mon, Apr 11, 2022 at 9:33 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote:
> > From: Dongliang Mu <mudongliangabcd@xxxxxxxxx>
> >
> > 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