| Message ID | 4AB43041.6050001@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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.kluin@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 on the next call if it's not NULL. We would also need to handle other errors, I don't see why -ENOMEM should get a special treatment. This might be a bit too complex. It would be simpler just to keep on looping over devices and adding controls even if an error occurs. > INIT_LIST_HEAD(&info->mappings); > list_add_tail(&info->list, &uvc_driver.controls); > uvc_ctrl_add_ctrl() is also called in uvc_ctrl_init_device(). If we're going to handle errors in uvc_ctrl_add_info(), they should be handled in uvc_ctrl_init_device() as well. My opinion is that we should probably not care about uvc_ctrl_add_ctrl() errors, but we certainly need to prevent ctrl->info from being modified if ctrl->data allocation fails. In that case uvc_ctrl_add_ctrl() can still be a void function, all we would need to do would be - ctrl->info = info; ctrl->data = kmalloc(ctrl->info->size * UVC_CTRL_NDATA, GFP_KERNEL); + if (ctrl->data == NULL) + return -ENOMEM; + + ctrl->info = info;
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 24 September 2009 10:50:39 Paulo Assis wrote: > 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. Oops, you're right. My bad. Thanks for catching this.
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);
Produce an error if kmalloc() fails. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- Found with sed: http://kernelnewbies.org/roelkluin Build tested. Please review -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html