[v1] usb: gadget: uvc: Fix ERR_PTR dereference in uvc_v4l2.c

[PATCH] usb: gadget: uvc: Fix ERR_PTR dereference in uvc_v4l2.c

Posted by Abhishek Tamboli 1 year, 4 months ago

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;
--
2.34.1

Re: [PATCH] usb: gadget: uvc: Fix ERR_PTR dereference in uvc_v4l2.c

Posted by Greg KH 1 year, 4 months ago

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

Re: [PATCH] usb: gadget: uvc: Fix ERR_PTR dereference in uvc_v4l2.c

Posted by Abhishek Tamboli 1 year, 4 months ago

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

Re: [PATCH] usb: gadget: uvc: Fix ERR_PTR dereference in uvc_v4l2.c

Posted by Dan Carpenter 1 year, 4 months ago

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

Re: [PATCH] usb: gadget: uvc: Fix ERR_PTR dereference in uvc_v4l2.c

Posted by Abhishek Tamboli 1 year, 4 months ago

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

Re: [PATCH] usb: gadget: uvc: Fix ERR_PTR dereference in uvc_v4l2.c

Posted by Abhishek Tamboli 1 year, 4 months ago

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

Re: [PATCH] usb: gadget: uvc: Fix ERR_PTR dereference in uvc_v4l2.c

Posted by Laurent Pinchart 1 year, 4 months ago

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] usb: gadget: uvc: Fix ERR_PTR dereference in uvc_v4l2.c

Posted by Michael Grzeschik 1 year, 4 months ago

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

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |