On Mon, 2022-04-25 at 15:49 +0200, Oliver Neukum wrote:
>
>
> On 25.04.22 15:23, Bastien Nocera wrote:
> > struct usb_memory {
> > @@ -237,6 +238,9 @@ static int usbdev_mmap(struct file *file,
> > struct vm_area_struct *vma)
> > dma_addr_t dma_handle;
> > int ret;
> >
> > + if (!connected(ps) || ps->revoked)
> > + return -ENODEV;
> > +
> This lacks locking.
Added locking.
> >
> > +static int usbdev_revoke(struct usb_dev_state *ps)
> > +{
> > + struct usb_device *dev = ps->dev;
> > + unsigned int ifnum;
> > + struct async *as;
> > +
> > + if (ps->revoked)
> > + return -ENODEV;
> > + ps->revoked = true;
> > +
> > + usb_lock_device(dev);
> And here you lock the device a second time. That is a bad idea.
I've removed the locking in this function.
> > + for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps-
> > >ifclaimed);
> > + ifnum++) {
> > + if (test_bit(ifnum, &ps->ifclaimed))
> > + releaseintf(ps, ifnum);
> > + }
> > + destroy_all_async(ps);
> > + usb_unlock_device(dev);
> > +
> > + as = async_getcompleted(ps);
> > + while (as) {
> > + free_async(as);
> > + as = async_getcompleted(ps);
> > + }
> > +
> > + return 0;
> > +}
> Getting your file descriptor revoked should wake you up
> from poll(), shouldn't it?
Added a wakeup.
>
> > +
> > /*
> > * NOTE: All requests here that have interface numbers as
> > parameters
> > * are assuming that somehow the configuration has been prevented
> > from
> > @@ -2619,7 +2660,7 @@ static long usbdev_do_ioctl(struct file
> > *file, unsigned int cmd,
> > #endif
> > }
> >
> > - if (!connected(ps)) {
> > + if (!connected(ps) || ps->revoked) {
> > usb_unlock_device(dev);
> > return -ENODEV;
> > }
> > @@ -2779,6 +2820,11 @@ static long usbdev_do_ioctl(struct file
> > *file, unsigned int cmd,
> > case USBDEVFS_WAIT_FOR_RESUME:
> > ret = proc_wait_for_resume(ps);
> > break;
> > + case USBDEVFS_REVOKE:
> You are still in usb_lock_device()
Noted.
>
> Regards
> Oliver
>
I'll post those changes in v2, thanks.
