Re: [patch] [media] uvcvideo: freeing an error pointer
On Wed, Nov 30, 2016 at 03:53:03PM +0200, Laurent Pinchart wrote:
> But then you get the following patch (which, apart from being totally made
> up,
> probably shows what I've watched yesterday evening).
>
> @@ ... @@
> return -ENOMEM;
> }
>
> + ret = check_time_vortex();
> + if (ret < 0)
> + goto power_off_tardis;
> +
> matt_smith = alloc_regeneration();
> if (math_smith->status != OK) {
> ret = -E_NEEDS_FISH_FINGERS_AND_CUSTARD;
>
I don't get it. Did we power on the tardis on the lines before? That's
all the state that you need to keep in your head is just the most
recently allocated thing.
> >From that code only you can't tell whether the jump label is the right one.
> >If
> a single jump label is used with an unwinding code block that supports non-
> allocated resources, you don't have to ask yourself any question.
>
You absolutely do have to ask that question, you just can't answer it
without jumping back and forth. Doing everything at once is logically
more complicated than doing them one thing at a time, and empirically
just from looking at which code has the most bugs, then single exit
labels are the most buggy.
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
Hi Dan,
On Wednesday 30 Nov 2016 15:33:26 Dan Carpenter wrote:
> On Mon, Nov 28, 2016 at 04:49:36PM +0200, Laurent Pinchart wrote:
> > On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote:
> >> On Mon, 28 Nov 2016, Dan Carpenter wrote:
> >>> I understand the comparison, but I just think it's better if people
> >>> always keep track of what has been allocated and what has not. I
> >>> tried so hard to get Markus to stop sending those hundreds of patches
> >>> where he's like "this function has a sanity check so we can pass
> >>> pointers that weren't allocated"... It's garbage code.
> >>>
> >>> But I understand that other people don't agree.
> >>
> >> In my opinion, it is good for code understanding to only do what is
> >> useful to do. It's not a hard and fast rule, but I think it is
> >> something to take into account.
> >
> > On the other hand it complicates the error handling code by increasing the
> > number of goto labels, and it then becomes pretty easy when reworking code
> > to goto the wrong label. This is even more of an issue when the rework
> > doesn't touch the error handling code, in which case the reviewers can
> > easily miss the issue if they don't open the original source file to
> > check the goto labels.
>
> It's really not. I've looked at a lot of error handling in the past
> five years and sent hundreds of fixes for error paths, more than any
> other kernel developer during that time. Although it seems obvious in
> retrospect, it took me years to realize this but the canonical way of
> doing error handling is the least error prone.
>
> Counting the labels is the wrong way to measure complexity. That's like
> counting the number of functions. Code with lots of functions is not
> necessarily more complicated than if it's just one big function.
>
> Part of the key to unwinding correctly is using good label names. It
> should say what the label does. Some people use come-from labels names
> like "goto kmalloc_failed". Those are totally useless. It's like
> naming your functions "called_from_foo()". If there is only one goto
> then come-from label names are useless and if there are more than one
> goto which go to the same label then it's useless *and* misleading.
Yes, label naming is (or at least should be) largely agreed upon, they should
name the unwinding action, not the cause of the failure.
> Functions should be written so you can read them from top to bottom
> without scrolling back and forth.
>
> a = alloc();
> if (!a)
> return -ENOMEM;
>
> b = alloc();
> if (!b) {
> ret = -ENOMEM;
> goto free_a;
> }
But then you get the following patch (which, apart from being totally made up,
probably shows what I've watched yesterday evening).
@@ ... @@
return -ENOMEM;
}
+ ret = check_time_vortex();
+ if (ret < 0)
+ goto power_off_tardis;
+
matt_smith = alloc_regeneration();
if (math_smith->status != OK) {
ret = -E_NEEDS_FISH_FINGERS_AND_CUSTARD;
>From that code only you can't tell whether the jump label is the right one. If
a single jump label is used with an unwinding code block that supports non-
allocated resources, you don't have to ask yourself any question.
> That code tells a complete story without any scrolling. It's future
> proof too. You can tell the goto is correct just from the name. But
> when it's:
>
> a = alloc();
> if (!a)
> goto out;
> b = alloc();
> goto out;
>
> That code doesn't have enough information to be understandable on it's
> own. It's way more bug prone than the first sample.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
On Mon, Nov 28, 2016 at 04:49:36PM +0200, Laurent Pinchart wrote:
> Hi Julia and Dan,
>
> On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote:
> > On Mon, 28 Nov 2016, Dan Carpenter wrote:
> > > I understand the comparison, but I just think it's better if people
> > > always keep track of what has been allocated and what has not. I tried
> > > so hard to get Markus to stop sending those hundreds of patches where
> > > he's like "this function has a sanity check so we can pass pointers
> > > that weren't allocated"... It's garbage code.
> > >
> > > But I understand that other people don't agree.
> >
> > In my opinion, it is good for code understanding to only do what is useful
> > to do. It's not a hard and fast rule, but I think it is something to take
> > into account.
>
> On the other hand it complicates the error handling code by increasing the
> number of goto labels, and it then becomes pretty easy when reworking code to
> goto the wrong label. This is even more of an issue when the rework doesn't
> touch the error handling code, in which case the reviewers can easily miss
> the
> issue if they don't open the original source file to check the goto labels.
>
It's really not. I've looked at a lot of error handling in the past
five years and sent hundreds of fixes for error paths, more than any
other kernel developer during that time. Although it seems obvious in
retrospect, it took me years to realize this but the canonical way of
doing error handling is the least error prone.
Counting the labels is the wrong way to measure complexity. That's like
counting the number of functions. Code with lots of functions is not
necessarily more complicated than if it's just one big function.
Part of the key to unwinding correctly is using good label names. It
should say what the label does. Some people use come-from labels names
like "goto kmalloc_failed". Those are totally useless. It's like
naming your functions "called_from_foo()". If there is only one goto
then come-from label names are useless and if there are more than one
goto which go to the same label then it's useless *and* misleading.
Functions should be written so you can read them from top to bottom
without scrolling back and forth.
a = alloc();
if (!a)
return -ENOMEM;
b = alloc();
if (!b) {
ret = -ENOMEM;
goto free_a;
}
That code tells a complete story without any scrolling. It's future
proof too. You can tell the goto is correct just from the name. But
when it's:
a = alloc();
if (!a)
goto out;
b = alloc();
goto out;
That code doesn't have enough information to be understandable on it's
own. It's way more bug prone than the first sample.
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
To put it another way, tools can figure out what is missing if they have access to good exmples of what should be there. To be clear, I can see both points of view. julia -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
Hi Julia and Dan, On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote: > On Mon, 28 Nov 2016, Dan Carpenter wrote: > > I understand the comparison, but I just think it's better if people > > always keep track of what has been allocated and what has not. I tried > > so hard to get Markus to stop sending those hundreds of patches where > > he's like "this function has a sanity check so we can pass pointers > > that weren't allocated"... It's garbage code. > > > > But I understand that other people don't agree. > > In my opinion, it is good for code understanding to only do what is useful > to do. It's not a hard and fast rule, but I think it is something to take > into account. On the other hand it complicates the error handling code by increasing the number of goto labels, and it then becomes pretty easy when reworking code to goto the wrong label. This is even more of an issue when the rework doesn't touch the error handling code, in which case the reviewers can easily miss the issue if they don't open the original source file to check the goto labels. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
On Mon, 28 Nov 2016, Dan Carpenter wrote: > I understand the comparison, but I just think it's better if people > always keep track of what has been allocated and what has not. I tried > so hard to get Markus to stop sending those hundreds of patches where > he's like "this function has a sanity check so we can pass pointers > that weren't allocated"... It's garbage code. > > But I understand that other people don't agree. In my opinion, it is good for code understanding to only do what is useful to do. It's not a hard and fast rule, but I think it is something to take into account. julia > > regards, > dan carpenter > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
I understand the comparison, but I just think it's better if people always keep track of what has been allocated and what has not. I tried so hard to get Markus to stop sending those hundreds of patches where he's like "this function has a sanity check so we can pass pointers that weren't allocated"... It's garbage code. But I understand that other people don't agree. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
Hi Dan, On Fri, Nov 25, 2016 at 10:20:24PM +0300, Dan Carpenter wrote: > On Fri, Nov 25, 2016 at 06:02:45PM +0200, Laurent Pinchart wrote: > > Sakari Ailus (CC'ed) has expressed the opinion that we might want to go one > > step further and treat error pointers the same way we treat NULL or ZERO > > pointers today, by just returning without logging anything. The reasoning > > is > > that accepting a NULL pointer in kfree() was decided before we made > > extensive > > use of allocation APIs returning error pointers, so it could be time to > > update > > kfree() based on the current allocation usage patterns. > > Just don't free things that haven't been allocated. That honestly seems > like a simple rule to me, whenever I touch error handling code it feels > better and simpler after I fix the bugs. Error handling doesn't have to > be complicated if you just follow the rules. kfree() explicitly allows passing a NULL pointer to it; drivers often call kfree() on objects possibly allocated using kmalloc() and friends. This makes error handling easier in drivers which in turn decreases the probability of bugs, the other side of which we've already seen in form of the bug this patch fixes. Previously interfaces that allocated memory tended to either allocate that memory or in failing to do so, returned error in form of a NULL pointer. memdup_user() breaks that assumption by returning a negative error value as a pointer instead. I suppose one of the motivations of memdup_user() has been to reduce complexity of driver code as well as framework code dealing with implementing IOCTLs but at least in this case the end result was an introduction of a bug. This would not have happened in the first place if the API of functions dealing with releasing memory had been updated as well. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
On Fri, Nov 25, 2016 at 06:02:45PM +0200, Laurent Pinchart wrote: > Sakari Ailus (CC'ed) has expressed the opinion that we might want to go one > step further and treat error pointers the same way we treat NULL or ZERO > pointers today, by just returning without logging anything. The reasoning is > that accepting a NULL pointer in kfree() was decided before we made extensive > use of allocation APIs returning error pointers, so it could be time to > update > kfree() based on the current allocation usage patterns. Just don't free things that haven't been allocated. That honestly seems like a simple rule to me, whenever I touch error handling code it feels better and simpler after I fix the bugs. Error handling doesn't have to be complicated if you just follow the rules. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
On Fri, Nov 25, 2016 at 03:57:51PM +0200, Laurent Pinchart wrote: > diff --git a/mm/slab.c b/mm/slab.c > index 0b0550ca85b4..a7eb830c6684 100644 > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -3819,6 +3819,8 @@ void kfree(const void *objp) > > if (unlikely(ZERO_OR_NULL_PTR(objp))) > return; > + if (WARN_ON(IS_ERR(objp))) > + return; > local_irq_save(flags); > kfree_debugcheck(objp); > c = virt_to_cache(objp); > A bunch of people have proposed that. You're the first person to actually write up a patch. Feel free to send it. ;) regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
Hi Walter,
On Friday 25 Nov 2016 15:47:49 walter harms wrote:
> Am 25.11.2016 14:57, schrieb Laurent Pinchart:
> > On Friday 25 Nov 2016 13:28:35 Dan Carpenter wrote:
> >> A recent cleanup introduced a potential dereference of -EFAULT when we
> >> call kfree(map->menu_info).
> >
> > I should have caught that, my apologies :-(
> >
> > Thinking a bit more about this class of problems, would the following
> > patch make sense ?
> >
> > commit 034b71306510643f9f059249a0c14418099eb436
> > Author: Laurent Pinchart
> > Date: Fri Nov 25 15:54:22 2016 +0200
> >
> > mm/slab: WARN_ON error pointers passed to kfree()
> >
> > Passing an error pointer to kfree() is invalid and can lead to crashes
> > or memory corruption. Reject those pointers and WARN in order to catch
> > the problems and fix them in the callers.
> >
> > Signed-off-by: Laurent Pinchart
> >
> > diff --git a/mm/slab.c b/mm/slab.c
> > index 0b0550ca85b4..a7eb830c6684 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -3819,6 +3819,8 @@ void kfree(const void *objp)
> > if (unlikely(ZERO_OR_NULL_PTR(objp)))
> > return;
> >
> > + if (WARN_ON(IS_ERR(objp)))
> > + return;
> > local_irq_save(flags);
> > kfree_debugcheck(objp);
> > c = virt_to_cache(objp);
>
> I this is better in kfree_debugcheck().
> 1. it has the right name
> 2. is contains already a check
Sakari Ailus (CC'ed) has expressed the opinion that we might want to go one
step further and treat error pointers the same way we treat NULL or ZERO
pointers today, by just returning without logging anything. The reasoning is
that accepting a NULL pointer in kfree() was decided before we made extensive
use of allocation APIs returning error pointers, so it could be time to update
kfree() based on the current allocation usage patterns.
> static void kfree_debugcheck(const void *objp)
> {
> if (!virt_addr_valid(objp)) {
> pr_err("kfree_debugcheck: out of range ptr %lxh\n",
> (unsigned long)objp);
> BUG();
> }
> }
>
> btw: should this read %p ?
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
Am 25.11.2016 14:57, schrieb Laurent Pinchart:
> Hi Dan,
>
> Thank you for the patch.
>
> On Friday 25 Nov 2016 13:28:35 Dan Carpenter wrote:
>> A recent cleanup introduced a potential dereference of -EFAULT when we
>> call kfree(map->menu_info).
>
> I should have caught that, my apologies :-(
>
> Thinking a bit more about this class of problems, would the following patch
> make sense ?
>
> commit 034b71306510643f9f059249a0c14418099eb436
> Author: Laurent Pinchart
> Date: Fri Nov 25 15:54:22 2016 +0200
>
> mm/slab: WARN_ON error pointers passed to kfree()
>
> Passing an error pointer to kfree() is invalid and can lead to crashes
> or memory corruption. Reject those pointers and WARN in order to catch
> the problems and fix them in the callers.
>
> Signed-off-by: Laurent Pinchart
>
> diff --git a/mm/slab.c b/mm/slab.c
> index 0b0550ca85b4..a7eb830c6684 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3819,6 +3819,8 @@ void kfree(const void *objp)
>
> if (unlikely(ZERO_OR_NULL_PTR(objp)))
> return;
> + if (WARN_ON(IS_ERR(objp)))
> + return;
> local_irq_save(flags);
> kfree_debugcheck(objp);
> c = virt_to_cache(objp);
I this is better in kfree_debugcheck().
1. it has the right name
2. is contains already a check
static void kfree_debugcheck(const void *objp)
{
if (!virt_addr_valid(objp)) {
pr_err("kfree_debugcheck: out of range ptr %lxh\n",
(unsigned long)objp);
BUG();
}
}
btw: should this read %p ?
re,
wh
>> Fixes: 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than
>> duplicating its implementation")
>> Signed-off-by: Dan Carpenter
>
> Reviewed-by: Laurent Pinchart
>
> Mauro, the bug is present in your branch only at the moment and queued for
> v4.10. Could you please pick this patch up as well ?
>
>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
>> b/drivers/media/usb/uvc/uvc_v4l2.c index a7e12fd..3e7e283 100644
>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> @@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
>> *chain, if (xmap->menu_count == 0 ||
>> xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
>> ret = -EINVAL;
>> -goto done;
>> +goto free_map;
>> }
>>
>> size = xmap->menu_count * sizeof(*map->menu_info);
>> map->menu_info = memdup_user(xmap->menu_info, size);
>> if (IS_ERR(map->menu_info)) {
>> ret = PTR_ERR(map->menu_info);
>> -goto done;
>> +goto free_map;
>> }
>>
>> map->menu_count = xmap->menu_count;
>> @@ -83,13 +83,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
>> *chain, uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
>>"%u.\n", xmap->v4l2_type);
>> ret = -ENOTTY;
>> -goto done;
>> +goto free_map;
>> }
>>
>> ret = uvc_ctrl_add_mapping(chain, map);
>>
>> -done:
>> kfree(map->menu_info);
>> +free_map:
>> kfree(map);
>>
>> return ret;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
Hi Dan,
Thank you for the patch.
On Friday 25 Nov 2016 13:28:35 Dan Carpenter wrote:
> A recent cleanup introduced a potential dereference of -EFAULT when we
> call kfree(map->menu_info).
I should have caught that, my apologies :-(
Thinking a bit more about this class of problems, would the following patch
make sense ?
commit 034b71306510643f9f059249a0c14418099eb436
Author: Laurent Pinchart
Date: Fri Nov 25 15:54:22 2016 +0200
mm/slab: WARN_ON error pointers passed to kfree()
Passing an error pointer to kfree() is invalid and can lead to crashes
or memory corruption. Reject those pointers and WARN in order to catch
the problems and fix them in the callers.
Signed-off-by: Laurent Pinchart
diff --git a/mm/slab.c b/mm/slab.c
index 0b0550ca85b4..a7eb830c6684 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3819,6 +3819,8 @@ void kfree(const void *objp)
if (unlikely(ZERO_OR_NULL_PTR(objp)))
return;
+ if (WARN_ON(IS_ERR(objp)))
+ return;
local_irq_save(flags);
kfree_debugcheck(objp);
c = virt_to_cache(objp);
> Fixes: 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than
> duplicating its implementation")
> Signed-off-by: Dan Carpenter
Reviewed-by: Laurent Pinchart
Mauro, the bug is present in your branch only at the moment and queued for
v4.10. Could you please pick this patch up as well ?
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index a7e12fd..3e7e283 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
> *chain, if (xmap->menu_count == 0 ||
> xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
> ret = -EINVAL;
> - goto done;
> + goto free_map;
> }
>
> size = xmap->menu_count * sizeof(*map->menu_info);
> map->menu_info = memdup_user(xmap->menu_info, size);
> if (IS_ERR(map->menu_info)) {
> ret = PTR_ERR(map->menu_info);
> - goto done;
> + goto free_map;
> }
>
> map->menu_count = xmap->menu_count;
> @@ -83,13 +83,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
> *chain, uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
> "%u.\n", xmap->v4l2_type);
> ret = -ENOTTY;
> - goto done;
> + goto free_map;
> }
>
> ret = uvc_ctrl_add_mapping(chain, map);
>
> -done:
> kfree(map->menu_info);
> +free_map:
> kfree(map);
>
> return ret;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
> A recent cleanup introduced a potential dereference of -EFAULT when we
> call kfree(map->menu_info).
>
> Fixes: 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than
> duplicating its implementation")
> Signed-off-by: Dan Carpenter
Thanks for your information.
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c
> index a7e12fd..3e7e283 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
> *chain,
> if (xmap->menu_count == 0 ||
> xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
> ret = -EINVAL;
> - goto done;
> + goto free_map;
> }
>
> size = xmap->menu_count * sizeof(*map->menu_info);
> map->menu_info = memdup_user(xmap->menu_info, size);
> if (IS_ERR(map->menu_info)) {
> ret = PTR_ERR(map->menu_info);
> - goto done;
> + goto free_map;
> }
>
> map->menu_count = xmap->menu_count;
> @@ -83,13 +83,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
> *chain,
> uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
> "%u.\n", xmap->v4l2_type);
> ret = -ENOTTY;
> - goto done;
> + goto free_map;
> }
>
> ret = uvc_ctrl_add_mapping(chain, map);
>
> -done:
> kfree(map->menu_info);
> +free_map:
> kfree(map);
>
> return ret;
>
Did your update suggestion become also relevant just because the corresponding
update step “[PATCH 2/2] uvc_v4l2: One function call less in
uvc_ioctl_ctrl_map()
after error detection” which I offered as another change possibility on
2016-08-19
was rejected on 2016-11-22?
https://patchwork.linuxtv.org/patch/36528/
https://patchwork.kernel.org/patch/9289897/
https://lkml.kernel.org/r/<8f89ec37-1556-4c09-f0b7-df87b4169...@users.sourceforge.net>
Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] [media] uvcvideo: freeing an error pointer
A recent cleanup introduced a potential dereference of -EFAULT when we
call kfree(map->menu_info).
Fixes: 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than
duplicating its implementation")
Signed-off-by: Dan Carpenter
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index a7e12fd..3e7e283 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
if (xmap->menu_count == 0 ||
xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
ret = -EINVAL;
- goto done;
+ goto free_map;
}
size = xmap->menu_count * sizeof(*map->menu_info);
map->menu_info = memdup_user(xmap->menu_info, size);
if (IS_ERR(map->menu_info)) {
ret = PTR_ERR(map->menu_info);
- goto done;
+ goto free_map;
}
map->menu_count = xmap->menu_count;
@@ -83,13 +83,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
"%u.\n", xmap->v4l2_type);
ret = -ENOTTY;
- goto done;
+ goto free_map;
}
ret = uvc_ctrl_add_mapping(chain, map);
-done:
kfree(map->menu_info);
+free_map:
kfree(map);
return ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
