| Message ID | 20240802180247.519273-1-abhishektamboli9@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: uvc: Fix ERR_PTR dereference in uvc_v4l2.c | expand |
Hi Abhishek, (CC'ing Michael Grzeschik) Thank you for the patch. On Fri, Aug 02, 2024 at 11:32:47PM +0530, Abhishek Tamboli wrote: > Fix potential dereferencing of ERR_PTR() in find_format_by_pix() > and uvc_v4l2_enum_format(). > > Fix the following smatch errors: > > drivers/usb/gadget/function/uvc_v4l2.c:124 find_format_by_pix() > error: 'fmtdesc' dereferencing possible ERR_PTR() > drivers/usb/gadget/function/uvc_v4l2.c:392 uvc_v4l2_enum_format() > error: 'fmtdesc' dereferencing possible ERR_PTR() > > Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com> > --- > drivers/usb/gadget/function/uvc_v4l2.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c > index a024aecb76dc..9dd602a742c4 100644 > --- a/drivers/usb/gadget/function/uvc_v4l2.c > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > @@ -121,6 +121,9 @@ static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc, > list_for_each_entry(format, &uvc->header->formats, entry) { > const struct uvc_format_desc *fmtdesc = to_uvc_format(format->fmt); > > + if (IS_ERR(fmtdesc)) > + continue; > + > if (fmtdesc->fcc == pixelformat) { > uformat = format->fmt; > break; > @@ -389,6 +392,9 @@ uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f) > return -EINVAL; > > fmtdesc = to_uvc_format(uformat); > + if (IS_ERR(fmtdesc)) > + return -EINVAL; > + > f->pixelformat = fmtdesc->fcc; > > return 0; Michael, you authored this, I'll let you review the patch and decide if this is a false positive.
On Fri, Aug 02, 2024 at 11:32:47PM +0530, Abhishek Tamboli wrote: > Fix potential dereferencing of ERR_PTR() in find_format_by_pix() > and uvc_v4l2_enum_format(). > > Fix the following smatch errors: > > drivers/usb/gadget/function/uvc_v4l2.c:124 find_format_by_pix() > error: 'fmtdesc' dereferencing possible ERR_PTR() > drivers/usb/gadget/function/uvc_v4l2.c:392 uvc_v4l2_enum_format() > error: 'fmtdesc' dereferencing possible ERR_PTR() > > Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com> When I reviewed these warnings in 2022, I assumed that the error checking was left out deliberately because it couldn't fail so I didn't report these warnings. Almost all old Smatch warnings are false positives. That doesn't mean Smatch is bad, it's just how it's going to be when you fix all the real bugs. In this case, I just decided it was a false positive. It's possible I was wrong. Other times, I report the bug and the maintainers say that it's a false positive. There are some old bugs which are real. Sometimes I report a bug but the maintainer doesn't see the email because they go on vacation or something. Or someone sends a patch but it doesn't get merged. Another thing is that if a bug is over five years old and minor then I might not bother reporting it. These days kernel developers are very good at fixing static checker bugs and these kinds of things are pretty rare. I don't review old warnings in a systematic way. If I fix a bug in a file, then I'll re-review all the old warnings. If we decide to merge this code, it needs a Fixes tag. regards, dan carpenter
On Fri, Aug 02, 2024 at 01:40:48PM -0500, Dan Carpenter wrote: > On Fri, Aug 02, 2024 at 11:32:47PM +0530, Abhishek Tamboli wrote: > > Fix potential dereferencing of ERR_PTR() in find_format_by_pix() > > and uvc_v4l2_enum_format(). > > > > Fix the following smatch errors: > > > > drivers/usb/gadget/function/uvc_v4l2.c:124 find_format_by_pix() > > error: 'fmtdesc' dereferencing possible ERR_PTR() > > drivers/usb/gadget/function/uvc_v4l2.c:392 uvc_v4l2_enum_format() > > error: 'fmtdesc' dereferencing possible ERR_PTR() > > > > Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com> > > When I reviewed these warnings in 2022, I assumed that the error > checking was left out deliberately because it couldn't fail so I didn't > report these warnings. > > Almost all old Smatch warnings are false positives. That doesn't mean > Smatch is bad, it's just how it's going to be when you fix all the real > bugs. In this case, I just decided it was a false positive. It's > possible I was wrong. Other times, I report the bug and the maintainers > say that it's a false positive. > > There are some old bugs which are real. Sometimes I report a bug but > the maintainer doesn't see the email because they go on vacation or > something. Or someone sends a patch but it doesn't get merged. Another > thing is that if a bug is over five years old and minor then I might not > bother reporting it. These days kernel developers are very good at > fixing static checker bugs and these kinds of things are pretty rare. > > I don't review old warnings in a systematic way. If I fix a bug in a > file, then I'll re-review all the old warnings. > > If we decide to merge this code, it needs a Fixes tag. > Hi, I wanted to follow up on the patch I submitted to address a Smatch warning. While I understand that this warning might be a false positive, as mentioned in your reviews, I would greatly appreciate your guidance on whether this patch should be merged or if any further adjustments are needed. If we determine that the patch resolves a real issue, I am prepared to include the Fixes tag. Regards, Abhishek
On Fri, Aug 02, 2024 at 01:40:48PM -0500, Dan Carpenter wrote: > On Fri, Aug 02, 2024 at 11:32:47PM +0530, Abhishek Tamboli wrote: > > Fix potential dereferencing of ERR_PTR() in find_format_by_pix() > > and uvc_v4l2_enum_format(). > > > > Fix the following smatch errors: > > > > drivers/usb/gadget/function/uvc_v4l2.c:124 find_format_by_pix() > > error: 'fmtdesc' dereferencing possible ERR_PTR() > > drivers/usb/gadget/function/uvc_v4l2.c:392 uvc_v4l2_enum_format() > > error: 'fmtdesc' dereferencing possible ERR_PTR() > > > > Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com> > > When I reviewed these warnings in 2022, I assumed that the error > checking was left out deliberately because it couldn't fail so I didn't > report these warnings. > > Almost all old Smatch warnings are false positives. That doesn't mean > Smatch is bad, it's just how it's going to be when you fix all the real > bugs. In this case, I just decided it was a false positive. It's > possible I was wrong. Other times, I report the bug and the maintainers > say that it's a false positive. > > There are some old bugs which are real. Sometimes I report a bug but > the maintainer doesn't see the email because they go on vacation or > something. Or someone sends a patch but it doesn't get merged. Another > thing is that if a bug is over five years old and minor then I might not > bother reporting it. These days kernel developers are very good at > fixing static checker bugs and these kinds of things are pretty rare. > > I don't review old warnings in a systematic way. If I fix a bug in a > file, then I'll re-review all the old warnings. > > If we decide to merge this code, it needs a Fixes tag. > Hi, I wanted to follow up on the patch I submitted to address a Smatch warning. While I understand that this warning might be a false positive, as mentioned in your reviews, I would greatly appreciate your guidance on whether this patch should be merged or if any further adjustments are needed. If we determine that the patch resolves a real issue, I am prepared to include the Fixes tag. Regards, Abhishek
Hi Laurent, On Fri, Aug 02, 2024 at 09:18:41PM +0300, Laurent Pinchart wrote: >Hi Abhishek, > >(CC'ing Michael Grzeschik) > >Thank you for the patch. > >On Fri, Aug 02, 2024 at 11:32:47PM +0530, Abhishek Tamboli wrote: >> Fix potential dereferencing of ERR_PTR() in find_format_by_pix() >> and uvc_v4l2_enum_format(). >> >> Fix the following smatch errors: >> >> drivers/usb/gadget/function/uvc_v4l2.c:124 find_format_by_pix() >> error: 'fmtdesc' dereferencing possible ERR_PTR() >> drivers/usb/gadget/function/uvc_v4l2.c:392 uvc_v4l2_enum_format() >> error: 'fmtdesc' dereferencing possible ERR_PTR() >> >> Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com> >> --- >> drivers/usb/gadget/function/uvc_v4l2.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c >> index a024aecb76dc..9dd602a742c4 100644 >> --- a/drivers/usb/gadget/function/uvc_v4l2.c >> +++ b/drivers/usb/gadget/function/uvc_v4l2.c >> @@ -121,6 +121,9 @@ static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc, >> list_for_each_entry(format, &uvc->header->formats, entry) { >> const struct uvc_format_desc *fmtdesc = to_uvc_format(format->fmt); >> >> + if (IS_ERR(fmtdesc)) >> + continue; >> + >> if (fmtdesc->fcc == pixelformat) { >> uformat = format->fmt; >> break; >> @@ -389,6 +392,9 @@ uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f) >> return -EINVAL; >> >> fmtdesc = to_uvc_format(uformat); >> + if (IS_ERR(fmtdesc)) >> + return -EINVAL; >> + >> f->pixelformat = fmtdesc->fcc; >> >> return 0; > >Michael, you authored this, I'll let you review the patch and decide if >this is a false positive. Since the following patch was applied, https://lore.kernel.org/all/20240221-uvc-gadget-configfs-guid-v1-1-f0678ca62ebb@pengutronix.de/ the issue is technically impossible to happen. However the patch I mentioned was only applied recently and in all older kernels someone could add a format into configfs that is not part of uvc_format_desc from drivers/media/common/uvc.c and therefor can run into the issue. As this will also not hurt the current kernel I would like the patch to be applied with the Tag: Fixes: 588b9e8560 (usb: gadget: uvc: add v4l2 enumeration api calls) Thanks, Michael
On Fri, Aug 02, 2024 at 11:32:47PM +0530, Abhishek Tamboli wrote: > Fix potential dereferencing of ERR_PTR() in find_format_by_pix() > and uvc_v4l2_enum_format(). > > Fix the following smatch errors: > > drivers/usb/gadget/function/uvc_v4l2.c:124 find_format_by_pix() > error: 'fmtdesc' dereferencing possible ERR_PTR() > drivers/usb/gadget/function/uvc_v4l2.c:392 uvc_v4l2_enum_format() > error: 'fmtdesc' dereferencing possible ERR_PTR() > > Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com> > --- > drivers/usb/gadget/function/uvc_v4l2.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c > index a024aecb76dc..9dd602a742c4 100644 > --- a/drivers/usb/gadget/function/uvc_v4l2.c > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > @@ -121,6 +121,9 @@ static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc, > list_for_each_entry(format, &uvc->header->formats, entry) { > const struct uvc_format_desc *fmtdesc = to_uvc_format(format->fmt); > > + if (IS_ERR(fmtdesc)) > + continue; > + > if (fmtdesc->fcc == pixelformat) { > uformat = format->fmt; > break; > @@ -389,6 +392,9 @@ uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f) > return -EINVAL; > > fmtdesc = to_uvc_format(uformat); > + if (IS_ERR(fmtdesc)) > + return -EINVAL; > + > f->pixelformat = fmtdesc->fcc; You are now only checking 2 of the responses here, not all of them, which feels odd. Either fix all calls to this function, or none of them :) thanks, greg k-h
Hi Greg, Thank you for the feedback. On Tue, Aug 13, 2024 at 10:11:48AM +0200, Greg KH wrote: > On Fri, Aug 02, 2024 at 11:32:47PM +0530, Abhishek Tamboli wrote: > > Fix potential dereferencing of ERR_PTR() in find_format_by_pix() > > and uvc_v4l2_enum_format(). > > > > Fix the following smatch errors: > > > > drivers/usb/gadget/function/uvc_v4l2.c:124 find_format_by_pix() > > error: 'fmtdesc' dereferencing possible ERR_PTR() > > drivers/usb/gadget/function/uvc_v4l2.c:392 uvc_v4l2_enum_format() > > error: 'fmtdesc' dereferencing possible ERR_PTR() > > > > Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com> > > --- > > drivers/usb/gadget/function/uvc_v4l2.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c > > index a024aecb76dc..9dd602a742c4 100644 > > --- a/drivers/usb/gadget/function/uvc_v4l2.c > > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > > @@ -121,6 +121,9 @@ static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc, > > list_for_each_entry(format, &uvc->header->formats, entry) { > > const struct uvc_format_desc *fmtdesc = to_uvc_format(format->fmt); > > > > + if (IS_ERR(fmtdesc)) > > + continue; > > + > > if (fmtdesc->fcc == pixelformat) { > > uformat = format->fmt; > > break; > > @@ -389,6 +392,9 @@ uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f) > > return -EINVAL; > > > > fmtdesc = to_uvc_format(uformat); > > + if (IS_ERR(fmtdesc)) > > + return -EINVAL; > > + > > f->pixelformat = fmtdesc->fcc; > >You are now only checking 2 of the responses here, not all of them, I addressed only the errors reported by Smatch. > which feels odd. > > Either fix all calls to this function, or none of them :) As you suggested, I'll review the remaining calls to this function and submit an updated patch that covers all cases. Regards, Abhishek
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c index a024aecb76dc..9dd602a742c4 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -121,6 +121,9 @@ static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc, list_for_each_entry(format, &uvc->header->formats, entry) { const struct uvc_format_desc *fmtdesc = to_uvc_format(format->fmt); + if (IS_ERR(fmtdesc)) + continue; + if (fmtdesc->fcc == pixelformat) { uformat = format->fmt; break; @@ -389,6 +392,9 @@ uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f) return -EINVAL; fmtdesc = to_uvc_format(uformat); + if (IS_ERR(fmtdesc)) + return -EINVAL; + f->pixelformat = fmtdesc->fcc; return 0;
Fix potential dereferencing of ERR_PTR() in find_format_by_pix() and uvc_v4l2_enum_format(). Fix the following smatch errors: drivers/usb/gadget/function/uvc_v4l2.c:124 find_format_by_pix() error: 'fmtdesc' dereferencing possible ERR_PTR() drivers/usb/gadget/function/uvc_v4l2.c:392 uvc_v4l2_enum_format() error: 'fmtdesc' dereferencing possible ERR_PTR() Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com> --- drivers/usb/gadget/function/uvc_v4l2.c | 6 ++++++ 1 file changed, 6 insertions(+) -- 2.34.1