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 <[email protected]>

drivers/media/usb/uvc/uvc_v4l2.c +3 -3

index a7e12fd..3e7e283 100644
@@ -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;

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 <[email protected]>

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvc_v4l2.c index a7e12fd..3e7e283 100644

drivers/media/usb/uvc/uvc_v4l2.c +4 -4

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

Hi Dan,

Thank you for the patch.

Quoted 2 lines · 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 <[email protected]>
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 <[email protected]>

mm/slab.c +0 -0

index 0b0550ca85b4..a7eb830c6684 100644

Quoted 3 lines

Fixes 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than

duplicating its implementation")

Signed-off-by Dan Carpenter <[email protected]>

Reviewed-by Laurent Pinchart <[email protected]>

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 ?

Quoted 37 lines

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvc_v4l2.c index a7e12fd..3e7e283 100644

drivers/media/usb/uvc/uvc_v4l2.c +4 -4

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

Am 25.11.2016 14:57, schrieb Laurent Pinchart:

Quoted 29 lines · On Friday 25 Nov 2016 13:28:35 Dan Carpenter wrote:

Quoted 2 lines

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 <[email protected]>
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 <[email protected]>

mm/slab.c +0 -0

index 0b0550ca85b4..a7eb830c6684 100644
@@ -3819,6 +3819,8 @@ void kfree(const void *objp)
 	if (unlikely(ZERO_OR_NULL_PTR(objp)))
 		return;
 

mm/slab.c void kfree(const void *objp) · line 3826

Quoted 5 lines

mm/slab.c +2 -0

+	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 ?

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.

Quoted 6 lines · Laurent Pinchart

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.

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 kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 6 lines · Dan Carpenter

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.

Quoted 1 line

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.

Hi Julia and Dan,

Quoted 7 lines · On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote:

Quoted 6 lines · Dan Carpenter

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.

Quoted 5 lines

Quoted 1 line

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.

Quoted 14 lines · Laurent Pinchart

Quoted 7 lines · On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote:

Quoted 6 lines · Dan Carpenter

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.

Quoted 5 lines

Quoted 1 line

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.

Quoted 6 lines

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

From that code only you can't tell whether the jump label is the right one. If

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.

But then you get the following patch (which, apart from being totally made up, probably shows what I've watched yesterday evening).

@@ ... @@

(quoted diff) +4 -0

 		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;

Quoted 1 line

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.

mm/slab.c +0 -0

index 0b0550ca85b4..a7eb830c6684 100644