Re: [PATCH v2] usb: gadget: uvc: Fix deadlocks in uvcg_configfs attribute callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] usb: gadget: uvc: missing mutex_unlock() in uvcg_extension_ba_source_id_store()
- [High] usb: gadget: uvc: unlocked hierarchy traversal in uvc_func_allow_link()
- [Medium] usb: gadget: uvc: unsynchronized refcnt check in uvcg_color_matching store
- [Medium] usb: gadget: uvc: unlocked fmt->linked check in uvcg_frame_b_frame_index_show()
--

commit 79cf2a23edc28ab90194ff7fd9c348a2fe889c55
Author: Saad Varshani <varshanisaad@gmail.com>

usb: gadget: uvc: Fix deadlocks in uvcg_configfs attribute callbacks

This commit removes su_mutex locking from configfs callbacks in
drivers/usb/gadget/function/uvc_configfs.c to avoid a lock ordering
deadlock between configfs frag_sem and su_mutex.

> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 70a1415ea4014..6a4f17b925e75 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c

[ ... ]

> @@ -1027,24 +939,18 @@ static ssize_t uvcg_extension_ba_source_id_show(struct config_item *item,
>  	}
>  	mutex_unlock(&opts->lock);
>  
> -	mutex_unlock(su_mutex);
> -
>  	return ret;
>  }
>  
>  static ssize_t uvcg_extension_ba_source_id_store(struct config_item *item,
>  						 const char *page, size_t len)
>  {
> -	struct config_group *group = to_config_group(item->ci_parent);
> -	struct mutex *su_mutex = &group->cg_subsys->su_mutex;
>  	struct uvcg_extension *xu = to_uvcg_extension(item);
>  	struct config_item *opts_item;
>  	struct f_uvc_opts *opts;
>  	u8 *source_ids, *iter;
>  	int ret, n = 0;
>  
> -	mutex_lock(su_mutex);
> -
>  	opts_item = item->ci_parent->ci_parent->ci_parent;
>  	opts = to_f_uvc_opts(opts_item);
>  
> @@ -1077,8 +983,6 @@ static ssize_t uvcg_extension_ba_source_id_store(struct config_item *item,
>  	ret = len;
>  
>  unlock:
> -	mutex_unlock(&opts->lock);
> -	mutex_unlock(su_mutex);
>  	return ret;
>  }
>  UVC_ATTR(uvcg_extension_, ba_source_id, baSourceID);

[Severity: High]
Was mutex_unlock(&opts->lock) removed by mistake at the unlock label in
uvcg_extension_ba_source_id_store()?

The function still acquires mutex_lock(&opts->lock) after resolving opts,
and both the normal path and all error paths go through the unlock label.
Writing to the baSourceID configfs attribute via configfs_write_iter() ->
flush_write_buffer() -> uvcg_extension_ba_source_id_store() will now return
to userspace with opts->lock still held, permanently deadlocking subsequent
operations on the UVC function instance.

[ ... ]

> @@ -1998,11 +1851,8 @@ static ssize_t uvcg_frame_b_frame_index_show(struct config_item *item,
>  	struct f_uvc_opts *opts;
>  	struct config_item *opts_item;
>  	struct config_item *fmt_item;
> -	struct mutex *su_mutex = &f->item.ci_group->cg_subsys->su_mutex;
>  	int result;
>  
> -	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> -
>  	fmt_item = f->item.ci_parent;
>  	fmt = to_uvcg_format(fmt_item);
>  
> @@ -2019,7 +1869,6 @@ static ssize_t uvcg_frame_b_frame_index_show(struct config_item *item,
>  	mutex_unlock(&opts->lock);
>  
>  out:
> -	mutex_unlock(su_mutex);
>  	return result;
>  }

[Severity: Medium]
With su_mutex removed from uvcg_frame_b_frame_index_show(), should the
check on fmt->linked be moved inside mutex_lock(&opts->lock)?

In uvcg_frame_b_frame_index_show():

	fmt_item = f->item.ci_parent;
	fmt = to_uvcg_format(fmt_item);

	if (!fmt->linked) {
		result = -EBUSY;
		goto out;
	}

	opts_item = fmt_item->ci_parent->ci_parent->ci_parent;
	opts = to_f_uvc_opts(opts_item);

	mutex_lock(&opts->lock);
	result = sprintf(page, "%u\n", f->frame.b_frame_index);
	mutex_unlock(&opts->lock);

Both fmt->linked and f->frame.b_frame_index (via uvcg_format_set_indices())
are updated under su_mutex and opts->lock in
uvcg_streaming_header_allow_link() and uvcg_streaming_header_drop_link().
Checking fmt->linked before acquiring opts->lock leaves a race window when
reading bFrameIndex concurrently with linking or unlinking a format.

[ ... ]

> @@ -3000,7 +2780,6 @@ static ssize_t uvcg_color_matching_##cname##_store(			\
>  	struct config_item *item, const char *page, size_t len)		\
>  {									\
>  	struct config_group *group = to_config_group(item);		\
> -	struct mutex *su_mutex = &group->cg_subsys->su_mutex;		\
>  	struct uvcg_color_matching *color_match =			\
>  		to_uvcg_color_matching(group);				\
>  	struct f_uvc_opts *opts;					\
> @@ -3012,8 +2791,6 @@ static ssize_t uvcg_color_matching_##cname##_store(			\
>  	if (ret)							\
>  		return ret;						\
>  									\
> -	mutex_lock(su_mutex); /* for navigating configfs hierarchy */	\
> -									\
>  	if (color_match->refcnt) {					\
>  		ret = -EBUSY;						\
>  		goto unlock_su;						\
> @@ -3029,8 +2806,6 @@ static ssize_t uvcg_color_matching_##cname##_store(			\
>  									\
>  	mutex_unlock(&opts->lock);					\
>  unlock_su:								\
> -	mutex_unlock(su_mutex);						\
> -									\
>  	return ret;							\
>  }									\
>  UVC_ATTR(uvcg_color_matching_, cname, aname)

[Severity: Medium]
What protects color_match->refcnt in uvcg_color_matching_##cname##_store()
now that su_mutex is no longer held here?

color_match->refcnt is checked before mutex_lock(&opts->lock) is acquired,
and the callers that modify color_match->refcnt (uvcg_format_allow_link(),
uvcg_format_drop_link(), uvcg_uncompressed_make(), uvcg_mjpeg_make(), and
uvcg_framebased_make()) update it under su_mutex without taking opts->lock:

	color_matching_desc->refcnt--;

	color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
	fmt->color_matching = color_matching_desc;
	color_matching_desc->refcnt++;

Without synchronization between uvcg_color_matching_##cname##_store() and
those refcnt updates, can a concurrent mkdir, symlink, or unlink race with
attribute writes and allow modifying an active color_matching descriptor?

[ ... ]

> @@ -3642,14 +3411,11 @@ static void uvc_func_item_release(struct config_item *item)
>  
>  static int uvc_func_allow_link(struct config_item *src, struct config_item *tgt)
>  {
> -	struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
>  	struct gadget_string *string;
>  	struct config_item *strings;
>  	struct f_uvc_opts *opts;
>  	int ret = 0;
>  
> -	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> -
>  	/* Validate that the target is an entry in strings/<langid> */
>  	strings = config_group_find_item(to_config_group(src->ci_parent->ci_parent),
>  					 "strings");
> @@ -3676,7 +3442,6 @@ static int uvc_func_allow_link(struct config_item *src, struct config_item *tgt)
>  
>  put_strings:
>  	config_item_put(strings);
> -	mutex_unlock(su_mutex);
>  
>  	return ret;
>  }

[Severity: High]
Is it safe to remove su_mutex from uvc_func_allow_link()?

Unlike attribute show and store callbacks, uvc_func_allow_link() is an
allow_link callback invoked from configfs_symlink(), which does not hold
frag_sem or su_mutex:

	if (!ret)
		ret = type->ct_item_ops->allow_link(parent_item, target_item);
	if (!ret) {
		mutex_lock(&configfs_symlink_mutex);
		ret = create_link(parent_item, target_item, target_sd, dentry);
		mutex_unlock(&configfs_symlink_mutex);

uvc_func_allow_link() calls config_group_find_item(), which walks
group->cg_children and requires group->cg_subsys->su_mutex to be held
against concurrent configfs_mkdir() and configfs_rmdir() modifications.

In addition, before create_link() increments target_sd->s_links, a
concurrent configfs_rmdir() on tgt can run unlink_obj() under su_mutex:

	group = item->ci_group;
	if (group) {
		list_del_init(&item->ci_entry);

		item->ci_group = NULL;
		item->ci_parent = NULL;

		/* Drop the reference for ci_entry */
		config_item_put(item);

		/* Drop the reference for ci_parent */
		config_group_put(group);
	}

Without su_mutex, couldn't evaluating tgt->ci_parent->ci_parent != strings
in uvc_func_allow_link() race with unlink_obj(tgt) and dereference a NULL
or freed ci_parent?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106020727.6055-1-varshanisaad@gmail.com?part=1