Re: [PATCH] uvc: kmalloc failure ignored in uvc_ctrl_add_ctrl()

2009-09-24 Thread Laurent Pinchart 
Hi Roel,

thanks for noticing the problem and providing a patch. Some comments inlined.

On Saturday 19 September 2009 03:13:37 Roel Kluin wrote:
 Produce an error if kmalloc() fails.
 
 Signed-off-by: Roel Kluin roel.kl...@gmail.com
 ---
 Found with sed: http://kernelnewbies.org/roelkluin
 
 Build tested. Please review
 
 diff --git a/drivers/media/video/uvc/uvc_ctrl.c
  b/drivers/media/video/uvc/uvc_ctrl.c index c3225a5..dda80b5 100644
 --- a/drivers/media/video/uvc/uvc_ctrl.c
 +++ b/drivers/media/video/uvc/uvc_ctrl.c
 @@ -1189,7 +1189,7 @@ int uvc_ctrl_resume_device(struct uvc_device *dev)
   * Control and mapping handling
   */
 
 -static void uvc_ctrl_add_ctrl(struct uvc_device *dev,
 +static int uvc_ctrl_add_ctrl(struct uvc_device *dev,
   struct uvc_control_info *info)
  {
   struct uvc_entity *entity;
 @@ -1214,7 +1214,7 @@ static void uvc_ctrl_add_ctrl(struct uvc_device *dev,
   }
 
   if (!found)
 - return;
 + return 0;
 
   if (UVC_ENTITY_TYPE(entity) == UVC_VC_EXTENSION_UNIT) {
   /* Check if the device control information and length match
 @@ -1231,7 +1231,7 @@ static void uvc_ctrl_add_ctrl(struct uvc_device *dev,
   control  UVC_GUID_FORMAT /%u (%d).\n,
   UVC_GUID_ARGS(info-entity), info-selector,
   ret);
 - return;
 + return -EINVAL;

uvc_query_ctrl returns an error code on failure, so

return ret;

might be more appropriate.

   }
 
   if (info-size != le16_to_cpu(size)) {
 @@ -1239,7 +1239,7 @@ static void uvc_ctrl_add_ctrl(struct uvc_device *dev,
   /%u size doesn't match user supplied 
   value.\n, UVC_GUID_ARGS(info-entity),
   info-selector);
 - return;
 + return -EINVAL;
   }
 
   ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl-entity-id,
 @@ -1249,7 +1249,7 @@ static void uvc_ctrl_add_ctrl(struct uvc_device *dev,
   control  UVC_GUID_FORMAT /%u (%d).\n,
   UVC_GUID_ARGS(info-entity), info-selector,
   ret);
 - return;
 + return -EINVAL;
   }

Ditto,

return ret;
 
   flags = info-flags;
 @@ -1259,15 +1259,18 @@ static void uvc_ctrl_add_ctrl(struct uvc_device
  *dev, UVC_GUID_FORMAT /%u flags don't match 
   supported operations.\n,
   UVC_GUID_ARGS(info-entity), info-selector);
 - return;
 + return -EINVAL;
   }
   }
 
   ctrl-info = info;
   ctrl-data = kmalloc(ctrl-info-size * UVC_CTRL_NDATA, GFP_KERNEL);
 + if (ctrl-data == NULL)
 + return -ENOMEM;

That's not enough to prevent a kernel crash. The driver can try to dereference 
ctrl-data if ctrl-info isn't NULL. You should only set ctrl-info if 
allocationg succeeds. Something like

ctrl-data = kmalloc(ctrl-info-size * UVC_CTRL_NDATA, GFP_KERNEL);
if (ctrl-data == NULL)
return -ENOMEM;

ctrl-info = info;

   uvc_trace(UVC_TRACE_CONTROL, Added control  UVC_GUID_FORMAT /%u 
   to device %s entity %u\n, UVC_GUID_ARGS(ctrl-info-entity),
   ctrl-info-selector, dev-udev-devpath, entity-id);
 + return 0;
  }
 
  /*
 @@ -1309,8 +1312,11 @@ int uvc_ctrl_add_info(struct uvc_control_info *info)
   }
   }
 
 - list_for_each_entry(dev, uvc_driver.devices, list)
 - uvc_ctrl_add_ctrl(dev, info);
 + list_for_each_entry(dev, uvc_driver.devices, list) {
 + ret = uvc_ctrl_add_ctrl(dev, info);
 + if (ret == -ENOMEM)
 + goto end;
 + }

This could lead to a memory leak.

Let's suppose you have two connected devices and try to add a control that 
both devices support. Let's also suppose the call to uvc_ctrl_add_ctrl() 
succeeds for the first device, but fails with -ENOMEM for the second. 
UVCIOC_CTRL_ADD will then return with an error without adding the control 
information to the uvc_driver.controls list.

The userspace application receives an -ENOMEM error an retries the call. 
uvc_ctrl_add_ctrl() will then overwrite ctrl-data with newly kmalloc'ed 
memory, leaking the previously allocated instance.

I'm not sure if we should really bail out here. The current situation is 
clearly not optimal, in the sense that UVCIOC_CTRL_ADD will succeed even if 
allocation fails for some control instances. On the other hand, your patch 
introduces a memory leak, which is not good either.

If we decide to bail out with an error we probably need to free ctrl-data 
memory allocated by the UVCIOC_CTRL_ADD call (and reset ctrl-info to NULL), 
or at least make sure we properly kfree ctrl-data

Re: [Linux-uvc-devel] [PATCH] uvc: kmalloc failure ignored in uvc_ctrl_add_ctrl()

2009-09-24 Thread Paulo Assis 
Laurent,


 That's not enough to prevent a kernel crash. The driver can try to dereference
 ctrl-data if ctrl-info isn't NULL. You should only set ctrl-info if
 allocationg succeeds. Something like

        ctrl-data = kmalloc(ctrl-info-size * UVC_CTRL_NDATA, GFP_KERNEL);
        if (ctrl-data == NULL)
                return -ENOMEM;

        ctrl-info = info;

Without reading any code this doesn't seem correct, how can you use
ctrl-info-size if you haven't set ctrl-info yet?

Did you mean something like this:

 ctrl-data = kmalloc(info-size * UVC_CTRL_NDATA, GFP_KERNEL);
 if (ctrl-data == NULL)
 return -ENOMEM;

 ctrl-info = info;


Like I said I haven't read the code but this looks better.

Best regards,
Paulo
--
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] uvc: kmalloc failure ignored in uvc_ctrl_add_ctrl()

2009-09-18 Thread Roel Kluin 
Produce an error if kmalloc() fails.

Signed-off-by: Roel Kluin roel.kl...@gmail.com
---
Found with sed: http://kernelnewbies.org/roelkluin

Build tested. Please review

diff --git a/drivers/media/video/uvc/uvc_ctrl.c 
b/drivers/media/video/uvc/uvc_ctrl.c
index c3225a5..dda80b5 100644
--- a/drivers/media/video/uvc/uvc_ctrl.c
+++ b/drivers/media/video/uvc/uvc_ctrl.c
@@ -1189,7 +1189,7 @@ int uvc_ctrl_resume_device(struct uvc_device *dev)
  * Control and mapping handling
  */
 
-static void uvc_ctrl_add_ctrl(struct uvc_device *dev,
+static int uvc_ctrl_add_ctrl(struct uvc_device *dev,
struct uvc_control_info *info)
 {
struct uvc_entity *entity;
@@ -1214,7 +1214,7 @@ static void uvc_ctrl_add_ctrl(struct uvc_device *dev,
}
 
if (!found)
-   return;
+   return 0;
 
if (UVC_ENTITY_TYPE(entity) == UVC_VC_EXTENSION_UNIT) {
/* Check if the device control information and length match
@@ -1231,7 +1231,7 @@ static void uvc_ctrl_add_ctrl(struct uvc_device *dev,
control  UVC_GUID_FORMAT /%u (%d).\n,
UVC_GUID_ARGS(info-entity), info-selector,
ret);
-   return;
+   return -EINVAL;
}
 
if (info-size != le16_to_cpu(size)) {
@@ -1239,7 +1239,7 @@ static void uvc_ctrl_add_ctrl(struct uvc_device *dev,
/%u size doesn't match user supplied 
value.\n, UVC_GUID_ARGS(info-entity),
info-selector);
-   return;
+   return -EINVAL;
}
 
ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl-entity-id,
@@ -1249,7 +1249,7 @@ static void uvc_ctrl_add_ctrl(struct uvc_device *dev,
control  UVC_GUID_FORMAT /%u (%d).\n,
UVC_GUID_ARGS(info-entity), info-selector,
ret);
-   return;
+   return -EINVAL;
}
 
flags = info-flags;
@@ -1259,15 +1259,18 @@ static void uvc_ctrl_add_ctrl(struct uvc_device *dev,
UVC_GUID_FORMAT /%u flags don't match 
supported operations.\n,
UVC_GUID_ARGS(info-entity), info-selector);
-   return;
+   return -EINVAL;
}
}
 
ctrl-info = info;
ctrl-data = kmalloc(ctrl-info-size * UVC_CTRL_NDATA, GFP_KERNEL);
+   if (ctrl-data == NULL)
+   return -ENOMEM;
uvc_trace(UVC_TRACE_CONTROL, Added control  UVC_GUID_FORMAT /%u 
to device %s entity %u\n, UVC_GUID_ARGS(ctrl-info-entity),
ctrl-info-selector, dev-udev-devpath, entity-id);
+   return 0;
 }
 
 /*
@@ -1309,8 +1312,11 @@ int uvc_ctrl_add_info(struct uvc_control_info *info)
}
}
 
-   list_for_each_entry(dev, uvc_driver.devices, list)
-   uvc_ctrl_add_ctrl(dev, info);
+   list_for_each_entry(dev, uvc_driver.devices, list) {
+   ret = uvc_ctrl_add_ctrl(dev, info);
+   if (ret == -ENOMEM)
+   goto end;
+   }
 
INIT_LIST_HEAD(info-mappings);
list_add_tail(info-list, uvc_driver.controls);
--
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