Hi Ricardo, Thank you for the patch. On Thu, Mar 24, 2022 at 11:30:08PM +0100, Ricardo Ribalda wrote: > If the mapping fails, the name field is not freed on exit. > Take the same approach as with the menu_info and have two different > allocations with two different life cycles. I think this needs an update. > Fixes: 07adedb5c606 ("media: uvcvideo: Use control names from framework") > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 10 ++++++++++ > drivers/media/usb/uvc/uvc_v4l2.c | 8 ++++---- > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index b4f6edf968bc..8b3bd516cb2f 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -2188,11 +2188,21 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > if (map == NULL) > return -ENOMEM; > > + /* For UVCIOC_CTRL_MAP custom controls */ > + if (mapping->name) { > + map->name = kstrdup(mapping->name, GFP_KERNEL); > + if (!map->name) { > + kfree(map); > + return -ENOMEM; > + } > + } > + > INIT_LIST_HEAD(&map->ev_subs); > > size = sizeof(*mapping->menu_info) * mapping->menu_count; > map->menu_info = kmemdup(mapping->menu_info, size, GFP_KERNEL); > if (map->menu_info == NULL) { > + kfree(map->name); > kfree(map); > return -ENOMEM; > } > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 711556d13d03..891c0bbc295c 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -42,12 +42,12 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain, > map->id = xmap->id; > /* Non standard control id. */ > if (v4l2_ctrl_get_name(map->id) == NULL) { > - map->name = kmemdup(xmap->name, sizeof(xmap->name), > - GFP_KERNEL); > - if (!map->name) { > - ret = -ENOMEM; > + if (!xmap->name) { > + ret = -EINVAL; > goto free_map; > } > + map->name = xmap->name; > + map->name[sizeof(xmap->name) - 1] = '\0'; Do you mind writing this as xmap->name[sizeof(xmap->name) - 1] = '\0'; ? It looks less like a possible bug than having map and xmap. Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > } > memcpy(map->entity, xmap->entity, sizeof(map->entity)); > map->selector = xmap->selector; -- Regards, Laurent Pinchart