usb: gadget: configfs: Disallow empty function instance name

[Felipe Balbi <balbi@kernel.org>]

Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> Krzysztof Opasiak <k.opasiak@samsung.com> writes:
>> > On 12/12/2017 01:31 PM, Felipe Balbi wrote:
>> >> 
>> >> Hi,
>> >> 
>> >> Krzysztof Opasiak <k.opasiak@samsung.com> writes:
>> >>> Every function should have a type and instance name.
>> >>> Unfortunately in most cases instance name was left unused and unchecked.
>> >>> This may lead to situations like FunctionFS device name identified by ""
>> >>> or some misleading debug messages from TCM like:
>> >>>
>> >>> tcm: Activating
>> >>>
>> >>> To avoid this let's add a check that instance name should have at least
>> >>> one character.
>> >>>
>> >>> Reported-by: Stefan Agner <stefan@agner.ch>
>> >>> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
>> >>> ---
>> >>>   drivers/usb/gadget/configfs.c | 5 +++++
>> >>>   1 file changed, 5 insertions(+)
>> >>>
>> >>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> >>> index aeb9f3c40521..bdc9ec597d6a 100644
>> >>> --- a/drivers/usb/gadget/configfs.c
>> >>> +++ b/drivers/usb/gadget/configfs.c
>> >>> @@ -548,6 +548,11 @@ static struct config_group *function_make(
>> >>>   	*instance_name = '\0';
>> >>>   	instance_name++;
>> >>>   
>> >>> +	if (*instance_name == '\0') {
>> >>> +		pr_err("Instance name (after .) should not be empty\n");
>> >>> +		return ERR_PTR(-EINVAL);
>> >>> +	}
>> >> 
>> >> aaaaaand just like that you break potentially existing scripts :-)
>> >> 
>> >> We need to find a better way of enforcing a name which doesn't break
>> >> existing users.
>> >
>> > I'm really open for suggestions how to enforce this without breaking 
>> > those scripts ;)
>> >
>> > The origin of this commit is github issue for libusbgx[1].
>> > So the problem is that library allows to create a function with empty 
>> > name (because I mistakenly assumed that kernel rejects this) but then it 
>> > is unable to reinitialize the ConfigFS state because there is a check 
>> > that disallows this. From my point of view I'd be happy to disallow 
>> > empty names because it causes some problems (f_fs) or weird debug 
>> > messages (f_tcm) so is generally inconvenient and seems to be 
>> > unintentional. But I would like to keep this consistent with kernel policy.
>> 
>> I think we need to first fix libusbgx to prevent empty names.
>> 
>> I don't want to be the one hearing from Linus that "we don't break
>> userspace". It's clear that empty names shouldn't be allowed, but they
>> _are_ allowed as of today, so how can we cause a regression all of a
>> sudden?
>> 
>> Alan, Greg, any suggestions?
>
> You could do some silly name munging, like changing an empty name to
> " " whenever you encounter it.  Or adding an '_' to the end of any name
> that consists of nothing but '_' characters.

Hmm, that could be done. So everytime userspace gives us an empty name,
we would convert to '_'. That still doesn't solve the problems of
mounting functionfs, though. But I guess there's nothing that can be
done in that case.