On Tue, 12 Dec 2017, Felipe Balbi wrote:
> Hi,
>
> Krzysztof Opasiak <k.opasiak@xxxxxxxxxxx> writes:
> > On 12/12/2017 01:31 PM, Felipe Balbi wrote:
> >>
> >> Hi,
> >>
> >> Krzysztof Opasiak <k.opasiak@xxxxxxxxxxx> 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@xxxxxxxx>
> >>> Signed-off-by: Krzysztof Opasiak <k.opasiak@xxxxxxxxxxx>
> >>> ---
> >>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
