| Message ID | 20210721194307.12155-1-paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] media: em28xx: add missing em28xx_close_extension | expand |
On 21/07/2021 21:43, Pavel Skripkin wrote: > If em28xx dev has ->dev_next pointer, we need to delete dev_next list node > from em28xx_extension_devlist on disconnect to avoid UAF bugs and > corrupted list bugs, since driver frees this pointer on disconnect. > > Fixes: 1a23f81b7dc3 ("V4L/DVB (9979): em28xx: move usb probe code to a proper place") > Reported-and-tested-by: syzbot+a6969ef522a36d3344c9@syzkaller.appspotmail.com > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > --- > > Changes in v2: > Previous patch was completely broken. I've done some debugging > again and found true root case of the reported bug. > > --- > drivers/media/usb/em28xx/em28xx-cards.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c > index c1e0dccb7408..d56b040e1bd7 100644 > --- a/drivers/media/usb/em28xx/em28xx-cards.c > +++ b/drivers/media/usb/em28xx/em28xx-cards.c > @@ -4139,8 +4139,10 @@ static void em28xx_usb_disconnect(struct usb_interface *intf) > > em28xx_close_extension(dev); > > - if (dev->dev_next) > + if (dev->dev_next) { > em28xx_release_resources(dev->dev_next); > + em28xx_close_extension(dev->dev_next); Wouldn't it be better to swap these two? That order is also used for em28xx_close_extension(dev) and em28xx_release_resources(dev). You do need to store dev->dev_next in a temp variable, though. Regards, Hans > + } > em28xx_release_resources(dev); > > if (dev->dev_next) { >
On Thu, 29 Jul 2021 11:45:19 +0200 Hans Verkuil <hverkuil@xs4all.nl> wrote: > On 21/07/2021 21:43, Pavel Skripkin wrote: > > If em28xx dev has ->dev_next pointer, we need to delete dev_next > > list node from em28xx_extension_devlist on disconnect to avoid UAF > > bugs and corrupted list bugs, since driver frees this pointer on > > disconnect. > > > > Fixes: 1a23f81b7dc3 ("V4L/DVB (9979): em28xx: move usb probe code > > to a proper place") Reported-and-tested-by: > > syzbot+a6969ef522a36d3344c9@syzkaller.appspotmail.com > > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- > > > > Changes in v2: > > Previous patch was completely broken. I've done some > > debugging again and found true root case of the reported bug. > > > > --- > > drivers/media/usb/em28xx/em28xx-cards.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/em28xx/em28xx-cards.c > > b/drivers/media/usb/em28xx/em28xx-cards.c index > > c1e0dccb7408..d56b040e1bd7 100644 --- > > a/drivers/media/usb/em28xx/em28xx-cards.c +++ > > b/drivers/media/usb/em28xx/em28xx-cards.c @@ -4139,8 +4139,10 @@ > > static void em28xx_usb_disconnect(struct usb_interface *intf) > > em28xx_close_extension(dev); > > > > - if (dev->dev_next) > > + if (dev->dev_next) { > > em28xx_release_resources(dev->dev_next); > > + em28xx_close_extension(dev->dev_next); > > Wouldn't it be better to swap these two? > > That order is also used for em28xx_close_extension(dev) and > em28xx_release_resources(dev). > > You do need to store dev->dev_next in a temp variable, though. > Hi, Hans! I don't understand why I need to store dev->dev_next in a temp variable. I don't see code in em28xx_release_resources() or em28xx_close_extension() that zeroes this pointer. With regards, Pavel Skripkin
On 29/07/2021 14:45, Pavel Skripkin wrote: > On Thu, 29 Jul 2021 11:45:19 +0200 > Hans Verkuil <hverkuil@xs4all.nl> wrote: > >> On 21/07/2021 21:43, Pavel Skripkin wrote: >>> If em28xx dev has ->dev_next pointer, we need to delete dev_next >>> list node from em28xx_extension_devlist on disconnect to avoid UAF >>> bugs and corrupted list bugs, since driver frees this pointer on >>> disconnect. >>> >>> Fixes: 1a23f81b7dc3 ("V4L/DVB (9979): em28xx: move usb probe code >>> to a proper place") Reported-and-tested-by: >>> syzbot+a6969ef522a36d3344c9@syzkaller.appspotmail.com >>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- >>> >>> Changes in v2: >>> Previous patch was completely broken. I've done some >>> debugging again and found true root case of the reported bug. >>> >>> --- >>> drivers/media/usb/em28xx/em28xx-cards.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c >>> b/drivers/media/usb/em28xx/em28xx-cards.c index >>> c1e0dccb7408..d56b040e1bd7 100644 --- >>> a/drivers/media/usb/em28xx/em28xx-cards.c +++ >>> b/drivers/media/usb/em28xx/em28xx-cards.c @@ -4139,8 +4139,10 @@ >>> static void em28xx_usb_disconnect(struct usb_interface *intf) >>> em28xx_close_extension(dev); >>> >>> - if (dev->dev_next) >>> + if (dev->dev_next) { >>> em28xx_release_resources(dev->dev_next); >>> + em28xx_close_extension(dev->dev_next); >> >> Wouldn't it be better to swap these two? >> >> That order is also used for em28xx_close_extension(dev) and >> em28xx_release_resources(dev). >> >> You do need to store dev->dev_next in a temp variable, though. >> > > Hi, Hans! > > I don't understand why I need to store dev->dev_next in a temp > variable. I don't see code in em28xx_release_resources() or > em28xx_close_extension() that zeroes this pointer. Apologies for the confusion, just ignore that bit. I misunderstood what dev_next was for. So check if swapping these two lines passes syzbot, and then that's the final version. Regards, Hans > > > > With regards, > Pavel Skripkin > >
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c index c1e0dccb7408..d56b040e1bd7 100644 --- a/drivers/media/usb/em28xx/em28xx-cards.c +++ b/drivers/media/usb/em28xx/em28xx-cards.c @@ -4139,8 +4139,10 @@ static void em28xx_usb_disconnect(struct usb_interface *intf) em28xx_close_extension(dev); - if (dev->dev_next) + if (dev->dev_next) { em28xx_release_resources(dev->dev_next); + em28xx_close_extension(dev->dev_next); + } em28xx_release_resources(dev); if (dev->dev_next) {
If em28xx dev has ->dev_next pointer, we need to delete dev_next list node from em28xx_extension_devlist on disconnect to avoid UAF bugs and corrupted list bugs, since driver frees this pointer on disconnect. Fixes: 1a23f81b7dc3 ("V4L/DVB (9979): em28xx: move usb probe code to a proper place") Reported-and-tested-by: syzbot+a6969ef522a36d3344c9@syzkaller.appspotmail.com Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- Changes in v2: Previous patch was completely broken. I've done some debugging again and found true root case of the reported bug. --- drivers/media/usb/em28xx/em28xx-cards.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)