usb: gadget: configfs: Disallow empty function instance name

usb: gadget: configfs: Disallow empty function instance name

[Alan Stern]

On Tue, 12 Dec 2017, Felipe Balbi wrote:

> Hi,
> 
> 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.

Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: gadget: configfs: Disallow empty function instance name

[Krzysztof Opasiak]

On 12/13/2017 10:29 AM, Felipe Balbi wrote:
> 
> 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.
> 

How is it different from disallowing empty name?
It may also cause some "broken" scripts stop working.
Isn't it going to introduce some weird problems like:

mkdir g1/function/ffs._
mkdir g2/function/ffs.
-EBUSY

Best regards,

usb: gadget: configfs: Disallow empty function instance name

[Felipe Balbi]

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.

usb: gadget: configfs: Disallow empty function instance name

[Krzysztof Opasiak]

On 12/12/2017 02:16 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> 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 created PR for this[1]. If anyone here has any objections to this 
please let me now as soon as possible.

If there is no veto or other solution, I merge this in the end of week.

Footnotes:
1 - https://github.com/libusbgx/libusbgx/pull/20

Best regards,

usb: gadget: configfs: Disallow empty function instance name

[Felipe Balbi]

Hi,

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?

usb: gadget: configfs: Disallow empty function instance name

[Krzysztof Opasiak]

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.

Footnotes:
1 - https://github.com/libusbgx/libusbgx/issues/19

usb: gadget: configfs: Disallow empty function instance name

[Felipe Balbi]

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.

usb: gadget: configfs: Disallow empty function instance name

[Krzysztof Opasiak]

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);
+	}
+
 	fi = usb_get_function_instance(func_name);
 	if (IS_ERR(fi))
 		return ERR_CAST(fi);