diff mbox series

usb: gadget: configfs: Restrict symlink creation if UDC already binded

Message ID 20230125065740.12504-1-quic_ugoswami@quicinc.com (mailing list archive)
State New, archived
Headers show
Series usb: gadget: configfs: Restrict symlink creation if UDC already binded | expand

Commit Message

Udipto Goswami Jan. 25, 2023, 6:57 a.m. UTC
During enumeration or composition switch, if a userspace process
agnostic of the conventions of configs tries to create function symlink
seven after the UDC is bound to current config which is not correct.
Potentially it can create duplicates within the current config.

Prevent this by adding a check if udc_name already exists then bail
out of cfg_link.

Fixes: 88af8bbe4ef7 ("usb: gadget: the start of the configfs interface")
Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
---
 drivers/usb/gadget/configfs.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Greg Kroah-Hartman Jan. 25, 2023, 2:39 p.m. UTC | #1
On Wed, Jan 25, 2023 at 12:27:40PM +0530, Udipto Goswami wrote:
> During enumeration or composition switch, if a userspace process
> agnostic of the conventions of configs tries to create function symlink
> seven after the UDC is bound to current config which is not correct.
> Potentially it can create duplicates within the current config.

I'm sorry, but I can not parse this paragraph at all.

Exactly what is the problem here?  Is userspace doing something it
shouldn't?  If so, fix userspace, right?

> Prevent this by adding a check if udc_name already exists then bail
> out of cfg_link.

Why?  What will this help prevent if userspace already messed things up?

confused,

greg k-h
Udipto Goswami Jan. 31, 2023, 5:42 a.m. UTC | #2
Hi Greg,

On 1/25/23 8:09 PM, Greg Kroah-Hartman wrote:
> On Wed, Jan 25, 2023 at 12:27:40PM +0530, Udipto Goswami wrote:
>> During enumeration or composition switch, if a userspace process
>> agnostic of the conventions of configs tries to create function symlink
>> seven after the UDC is bound to current config which is not correct.
>> Potentially it can create duplicates within the current config.
> 
> I'm sorry, but I can not parse this paragraph at all.
> 
> Exactly what is the problem here?  Is userspace doing something it
> shouldn't?  If so, fix userspace, right?
> 
>> Prevent this by adding a check if udc_name already exists then bail
>> out of cfg_link.
> 
> Why?  What will this help prevent if userspace already messed things up?
> 
> confused,

Apologies about this, had already pushed a v2 fixing the commit text[1]

It's not particularly any userspace process doing something wrong, but 
even in general usage if a end users is able to execute commands 
directly through command line accessing kernel file system and gets 
access to config/usb_gadget/g1/, one an easily create
a duplicate symlink for an existing function.

Step1:
ln -s X1 ffs.a
-->cfg_link
--> usb_get_function(ffs.a)
	->ffs_alloc
	
	CFG->FUNC_LIST: <ffs.a>
	C->FUNCTION: <empty>
	
Step2: 	
echo udc.name > /config/usb_gadget/g1/UDC
--> UDC_store
	->composite_bind
	->usb_add_function

	CFG->FUNC_LIST: <empty>
	C->FUNCTION: <ffs.a>	
	
Step3:
ln -s Y1 ffs.a
-->cfg_link
-->usb_get_function(ffs.a)
	->ffs_alloc

	CFG->FUNC_LIST: <ffs.a>
	C->FUNCTION: <ffs.a>	

both the lists corresponds to the same function instance ffs.a but the 
usb_function* pointer is different because in step 3 ffs_alloc has 
created a new reference to usb_function* for ffs.a and added it to cfg_list.

Step4:
Now a composition switch involving <ffs.b,ffs.a> is executed.

the composition switch will involve 3 things:
	1. unlinking the previous functions existing
	2. creating new symlinks
	3. writing UDC

However, the composition switch is generally taken care by a userspace 
process which creates the symlinks in its own nomenclature(X*) and 
removes only those. So it won't be able to remove Y1 which user had 
created by own.

Due to this the new symlinks cannot be created for ffs.a since the entry 
already exists in CFG->FUNC_LIST.
The state of the CFG->FUNC_LIST is as follows:
	CFG->FUNC_LIST: <ffs.a>

Since UDC is binded already, creating these dummy/incorrect symlinks, 
that can interfere with successive enumeration attempts can be avoided.

Cfg->func_list points to the list of functions that corresponds to that 
particular configuration. C->function points to the list of functions 
that are already bound to UDC and enumerated successfully.Ideally, when 
a particular configuration is already enumerated and bounded, meddling 
with cfg->func_list can create inconsistencies.

[1]: 
https://lore.kernel.org/all/20230125072138.21925-1-quic_ugoswami@quicinc.com/

Thanks,
-Udipto
diff mbox series

Patch

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 78e7353e397b..434e49d29c50 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -455,6 +455,11 @@  static int config_usb_cfg_link(
 		}
 	}
 
+	if (gi->composite.gadget_driver.udc_name) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	f = usb_get_function(fi);
 	if (IS_ERR(f)) {
 		ret = PTR_ERR(f);