Re: [PATCH v2 2/2] usb: gadget: configfs: keep extended property lengths consistent

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: gadget: uninitialized opts_mutex leaves ext_prop stores unlocked
--

commit df5678128639f7210af0f75c6d367de91a94c6da
Author: Yingjie Wang <x-professor@outlook.com>

usb: gadget: configfs: keep extended property lengths consistent

This patch tracks the raw data length of USB OS Extended Properties
separately from the wire length, validates UTF-8 payloads before storing
them, and updates wire and aggregate descriptor lengths when the property
type or data changes.

Link: https://lore.kernel.org/r/179338899796.2590875.15337207674245295434.b4-review@b4

> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index f5fff213d7952..3d931da3ae6b4 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
[ ... ]
> @@ -1321,37 +1344,34 @@ static ssize_t ext_prop_type_store(struct config_item *item,
>  	if (desc->opts_mutex)
>  		mutex_lock(desc->opts_mutex);
>  
> -	if ((ext_prop->type == USB_EXT_PROP_BINARY ||
> -	    ext_prop->type == USB_EXT_PROP_LE32 ||
> -	    ext_prop->type == USB_EXT_PROP_BE32) &&
> -	    (type == USB_EXT_PROP_UNICODE ||
> -	    type == USB_EXT_PROP_UNICODE_ENV ||
> -	    type == USB_EXT_PROP_UNICODE_LINK))
> -		ext_prop->data_len <<= 1;
> -	else if ((ext_prop->type == USB_EXT_PROP_UNICODE ||
> -		   ext_prop->type == USB_EXT_PROP_UNICODE_ENV ||
> -		   ext_prop->type == USB_EXT_PROP_UNICODE_LINK) &&
> -		   (type == USB_EXT_PROP_BINARY ||
> -		   type == USB_EXT_PROP_LE32 ||
> -		   type == USB_EXT_PROP_BE32))
> -		ext_prop->data_len >>= 1;
> +	if (ext_prop->data) {
> +		int data_len;
> +
> +		data_len = ext_prop_data_wire_len(type, ext_prop->data,
> +						  ext_prop->data_raw_len);
> +		if (data_len < 0) {
> +			ret = data_len;
> +			goto out;
> +		}
> +		desc->ext_prop_len += data_len - ext_prop->data_len;
> +		ext_prop->data_len = data_len;
> +	}
>  	ext_prop->type = type;
> +	ret = len;
>  
> +out:
>  	if (desc->opts_mutex)
>  		mutex_unlock(desc->opts_mutex);
> -	return len;
> +	return ret;
>  }
[ ... ]
> @@ -1370,10 +1391,18 @@ static ssize_t ext_prop_data_store(struct config_item *item,
>  
>  	if (desc->opts_mutex)
>  		mutex_lock(desc->opts_mutex);
> +	data_len = ext_prop_data_wire_len(ext_prop->type, new_data, len);
> +	if (data_len < 0) {
> +		if (desc->opts_mutex)
> +			mutex_unlock(desc->opts_mutex);
> +		kfree(new_data);
> +		return data_len;
> +	}
>  	kfree(ext_prop->data);
>  	ext_prop->data = new_data;
>  	desc->ext_prop_len -= ext_prop->data_len;
> -	ext_prop->data_len = ext_prop_data_wire_len(ext_prop->type, len);
> +	ext_prop->data_raw_len = len;
> +	ext_prop->data_len = data_len;
>  	desc->ext_prop_len += ext_prop->data_len;

[Severity: High]
Can concurrent writes to the type and data configfs attributes trigger a
use-after-free on ext_prop->data or corrupt desc->ext_prop_len and
ext_prop->data_len?

Both ext_prop_type_store() and ext_prop_data_store() rely on
desc->opts_mutex to serialize ext_prop_data_wire_len() and
kfree(ext_prop->data). However, neither of the callers that set up configfs
OS descriptors initializes opts_mutex:

drivers/usb/gadget/function/f_ncm.c:ncm_alloc_inst() {
	...
	opts->ncm_os_desc.ext_compat_id = opts->ncm_ext_compat_id;

	mutex_init(&opts->lock);
	...
	descs[0] = &opts->ncm_os_desc;
}

drivers/usb/gadget/function/f_rndis.c:rndis_alloc_inst() {
	...
	opts->rndis_os_desc.ext_compat_id = opts->rndis_ext_compat_id;

	mutex_init(&opts->lock);
	...
	descs[0] = &opts->rndis_os_desc;
}

Because desc->opts_mutex is always NULL, mutex_lock(desc->opts_mutex) is
never taken. Since configfs_write_iter() only holds the per-open-file
buffer->mutex and a shared read lock on frag->frag_sem, can
ext_prop_data_store() free ext_prop->data while ext_prop_type_store() is
reading it inside ext_prop_data_wire_len(), or interleave type and data
updates to leave ext_prop->data_len and desc->ext_prop_len out of sync?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793470427.git.x-professor@outlook.com?part=2