| 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 |
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
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 --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);
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(+)