Re: Handling custom device control requests in userspace

[Jerry Zhang]

Thanks for the responses everyone

> IMHO, considering the amount of Android users, we can merge this into
> composite.c itself. Just make the code depend on CONFIG_ANDROID. Or
> something along the lines of

I suspect that once you see what this entails you won't be so willing :)

The code for handling accessory control requests is heavily tied into
f_accessory itself.
See acc_ctrlrequest in
https://android.googlesource.com/kernel/common/+/android-4.14/drivers/usb/gadget/function/f_accessory.c
We have configfs handle the requests in android_setup
https://android.googlesource.com/kernel/common/+/android-4.14/drivers/usb/gadget/configfs.c
The three requests we care about are ACCESSORY_START, which sends a uevent
signal, ACCESSORY_SEND_STRING, which stores strings that an ioctl later
retrieves, and ACCESSORY_GET_PROTOCOL, where the device must respond with
its protocol version. We'd probably want to change these communication
methods into configfs attrs under accessory, which means taking all the
other pieces of f_accessory as well. This is kind of weird since the
function itself minus control requests is quite easy to change to
functionfs. It may take more effort to tidy this up than to implement it
generically.

> If I understand correctly the purpose of creating a dedicated
configuration
> is to allow linking the functions to it and this way marking these
functions
> "special" (not assigned to a configuration proper).

yep

> While I understand that engineers should discuss facts rather than
feelings,
> the idea of creating a special configuration doesn't feel right to me,
> because this way you are creating a configuration which is not meant to be
> ever selected by the host. I can be convinced with arguments, though.

The configuration is not just never meant to be selected, the host doesn't
even see it in descriptors since it is not in cdev. Using a configuration
is convenient because the config group already exists and bind() already
takes in a configuration. Otherwise I might store a list of the functions
directly in gadget_info instead.

> I would like to draw your attention to one fact: a functionfs instance
> directory in configfs is intentionally empty, since the functionfs
delegates
> actual implementation, including descriptors and strings, to userspace.
> I would advise to consider (or at least prove wrong) the idea of adding
some
> attribute (file) to functionfs's configfs directory and using it for
marking
> this functionfs instance as special. This would require implementing some
> exclusion mechanism (if this ffs instance is linked to a configuration,
> it is impossible to mark it as special and vice versa).

I think the reason functionfs config attrs are empty is because we already
have several ways to pass in arguments to functionfs (mount args,
descriptor flags) that are already being used.

I'm not sure that it's feasible (without changing an interface somewhere)
to enable this functionality directly within functionfs. The issue is in
order to handle setups a function has to be alloced and bound to a
configuration with an underlying cdev. It's theoretically possible to just
bind to a cdev, but then you'd need to create new methods in struct
usb_function. Also, I think having a list of functions could be useful -- a
vendor could have a kernel function handling some control requests, and a
functionfs handling all the rest. In that case, they'd just link the
functions to control config in order.

> Let me just mention that if we had a generic interface we could cover
> not only android requirements but also do a great job for people who are
> fuzzing USB hosts. Such an interface would allow to easily craft some
> random requests without all this noise related to using GadgetFS...

My main intention is to handle vendor specific requests (compliant but not
standard). This is kind of interesting though and it would not be that hard
to allow -- have a bool flag attr control_config_priority and if that is
set, give setups to functionfs *before* composite.

I can provide patches in either case. I've already started experimenting

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

Re: Handling custom device control requests in userspace

[Krzysztof Opasiak]

On 06.04.2018 15:03, Felipe Balbi wrote:
[...]

okay, but we don't... Do you wanna send some patches?


another thing to keep in mind is that we're trying to be a USB-compliant
stack. Gadget Framework was never meant to be a testing ground for the
Host Stack. There are VERY flexible tools designed specifically for that
in market.


True but we still have GadgetFS and FunctionFS and no one (apart from 
the vendor who can sign his platform image) can guarantee that userspace 
that uses those interfaces is USB compliant...




Maybe, companies who are investing in Fuzzying, should just invest in
one of those tools instead of rellying on a stack that WANTS to send
compliant USB-packets at all times?



Hmmm but apart from companies there are tons of hobbyist or students 
(like mine;)) who are trying to build useful tools using cheap and 
popular hardware. Take USBProxy[1] or GadgetFS backend for umap2[2] as 
an example.


Footnotes:
1 - https://github.com/dominicgs/USBProxy
2 - https://github.com/nccgroup/umap2

Best regards
--
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Handling custom device control requests in userspace

[Felipe Balbi]

Hi again,

Felipe Balbi  writes:
> Krzysztof Opasiak  writes:
 A few advantages over a couple options I've considered are that this 
 mostly
 reuses existing functionalities and won't affect users that haven't 
 enabled
 it. Please let me know of any feedback on the design or any possible
 implementation issues.
>>>
>>> IMHO, considering the amount of Android users, we can merge this into
>>> composite.c itself. Just make the code depend on CONFIG_ANDROID. Or
>>> something along the lines of
>>>
>>> if (IS_ENABLED(CONFIG_ANDROID))
>>> android_audio_accessory_init();
>>>
>>> should get the job done.
>>
>> Huh and what with people that are not android but need to support
>> Android accessory in their products?
>
> Do those really exist?
>
> Well, if they _really_ exist, we can add a "Enable Support for Android
> Audio Accessory" specific to the gadget framework. Then test for:
>
> if (IS_ENABLED(CONFIG_USB_GADGET_ANDROID_AUDIO_ACCESSORY))
>
> or whatever. I'm not certain such devices exist though, need
> confirmation.
>

 Yes, they exist. Most of Tizen phones support android accessory.
>>> 
>>> Fair enough. A specific Kconfig for that is okay.
>>> 
>>
>> Let me just mention that if we had a generic interface we could cover 
>> not only android requirements but also do a great job for people who are 
>> fuzzing USB hosts. Such an interface would allow to easily craft some 
>> random requests without all this noise related to using GadgetFS...
>
> okay, but we don't... Do you wanna send some patches?

another thing to keep in mind is that we're trying to be a USB-compliant
stack. Gadget Framework was never meant to be a testing ground for the
Host Stack. There are VERY flexible tools designed specifically for that
in market.

Maybe, companies who are investing in Fuzzying, should just invest in
one of those tools instead of rellying on a stack that WANTS to send
compliant USB-packets at all times?

-- 
balbi


signature.asc
Description: PGP signature

Re: Handling custom device control requests in userspace

[Felipe Balbi]

Hi,

Krzysztof Opasiak  writes:
>>> A few advantages over a couple options I've considered are that this 
>>> mostly
>>> reuses existing functionalities and won't affect users that haven't 
>>> enabled
>>> it. Please let me know of any feedback on the design or any possible
>>> implementation issues.
>>
>> IMHO, considering the amount of Android users, we can merge this into
>> composite.c itself. Just make the code depend on CONFIG_ANDROID. Or
>> something along the lines of
>>
>> if (IS_ENABLED(CONFIG_ANDROID))
>>  android_audio_accessory_init();
>>
>> should get the job done.
>
> Huh and what with people that are not android but need to support
> Android accessory in their products?

 Do those really exist?

 Well, if they _really_ exist, we can add a "Enable Support for Android
 Audio Accessory" specific to the gadget framework. Then test for:

 if (IS_ENABLED(CONFIG_USB_GADGET_ANDROID_AUDIO_ACCESSORY))

 or whatever. I'm not certain such devices exist though, need
 confirmation.

>>>
>>> Yes, they exist. Most of Tizen phones support android accessory.
>> 
>> Fair enough. A specific Kconfig for that is okay.
>> 
>
> Let me just mention that if we had a generic interface we could cover 
> not only android requirements but also do a great job for people who are 
> fuzzing USB hosts. Such an interface would allow to easily craft some 
> random requests without all this noise related to using GadgetFS...

okay, but we don't... Do you wanna send some patches?

-- 
balbi


signature.asc
Description: PGP signature

Re: Handling custom device control requests in userspace

[Krzysztof Opasiak]

On 06.04.2018 14:35, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  writes:

On 06.04.2018 12:18, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  writes:




A few advantages over a couple options I've considered are that this mostly
reuses existing functionalities and won't affect users that haven't enabled
it. Please let me know of any feedback on the design or any possible
implementation issues.


IMHO, considering the amount of Android users, we can merge this into
composite.c itself. Just make the code depend on CONFIG_ANDROID. Or
something along the lines of

if (IS_ENABLED(CONFIG_ANDROID))
android_audio_accessory_init();

should get the job done.


Huh and what with people that are not android but need to support
Android accessory in their products?


Do those really exist?

Well, if they _really_ exist, we can add a "Enable Support for Android
Audio Accessory" specific to the gadget framework. Then test for:

if (IS_ENABLED(CONFIG_USB_GADGET_ANDROID_AUDIO_ACCESSORY))

or whatever. I'm not certain such devices exist though, need
confirmation.



Yes, they exist. Most of Tizen phones support android accessory.


Fair enough. A specific Kconfig for that is okay.



Let me just mention that if we had a generic interface we could cover 
not only android requirements but also do a great job for people who are 
fuzzing USB hosts. Such an interface would allow to easily craft some 
random requests without all this noise related to using GadgetFS...


Best regards,
--
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Handling custom device control requests in userspace

[Felipe Balbi]

Hi,

Krzysztof Opasiak  writes:
> On 06.04.2018 12:18, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Krzysztof Opasiak  writes:
>> 
>> 
>> 
> A few advantages over a couple options I've considered are that this 
> mostly
> reuses existing functionalities and won't affect users that haven't 
> enabled
> it. Please let me know of any feedback on the design or any possible
> implementation issues.

 IMHO, considering the amount of Android users, we can merge this into
 composite.c itself. Just make the code depend on CONFIG_ANDROID. Or
 something along the lines of

 if (IS_ENABLED(CONFIG_ANDROID))
android_audio_accessory_init();

 should get the job done.
>>>
>>> Huh and what with people that are not android but need to support
>>> Android accessory in their products?
>> 
>> Do those really exist?
>> 
>> Well, if they _really_ exist, we can add a "Enable Support for Android
>> Audio Accessory" specific to the gadget framework. Then test for:
>> 
>> if (IS_ENABLED(CONFIG_USB_GADGET_ANDROID_AUDIO_ACCESSORY))
>> 
>> or whatever. I'm not certain such devices exist though, need
>> confirmation.
>> 
>
> Yes, they exist. Most of Tizen phones support android accessory.

Fair enough. A specific Kconfig for that is okay.

Jerry, do you wanna write the patch for this or would you prefer giving
me some requirements?

-- 
balbi


signature.asc
Description: PGP signature

Re: Handling custom device control requests in userspace

[Krzysztof Opasiak]

On 06.04.2018 12:18, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  writes:




A few advantages over a couple options I've considered are that this mostly
reuses existing functionalities and won't affect users that haven't enabled
it. Please let me know of any feedback on the design or any possible
implementation issues.


IMHO, considering the amount of Android users, we can merge this into
composite.c itself. Just make the code depend on CONFIG_ANDROID. Or
something along the lines of

if (IS_ENABLED(CONFIG_ANDROID))
android_audio_accessory_init();

should get the job done.


Huh and what with people that are not android but need to support
Android accessory in their products?


Do those really exist?

Well, if they _really_ exist, we can add a "Enable Support for Android
Audio Accessory" specific to the gadget framework. Then test for:

if (IS_ENABLED(CONFIG_USB_GADGET_ANDROID_AUDIO_ACCESSORY))

or whatever. I'm not certain such devices exist though, need
confirmation.



Yes, they exist. Most of Tizen phones support android accessory.

Best regards,
--
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Handling custom device control requests in userspace

[Felipe Balbi]

Hi,

Krzysztof Opasiak  writes:



>>> A few advantages over a couple options I've considered are that this mostly
>>> reuses existing functionalities and won't affect users that haven't enabled
>>> it. Please let me know of any feedback on the design or any possible
>>> implementation issues.
>> 
>> IMHO, considering the amount of Android users, we can merge this into
>> composite.c itself. Just make the code depend on CONFIG_ANDROID. Or
>> something along the lines of
>> 
>> if (IS_ENABLED(CONFIG_ANDROID))
>>  android_audio_accessory_init();
>> 
>> should get the job done.
>
> Huh and what with people that are not android but need to support 
> Android accessory in their products?

Do those really exist?

Well, if they _really_ exist, we can add a "Enable Support for Android
Audio Accessory" specific to the gadget framework. Then test for:

if (IS_ENABLED(CONFIG_USB_GADGET_ANDROID_AUDIO_ACCESSORY))

or whatever. I'm not certain such devices exist though, need
confirmation.

-- 
balbi


signature.asc
Description: PGP signature

Re: Handling custom device control requests in userspace

[Krzysztof Opasiak]

On 06.04.2018 12:12, Felipe Balbi wrote:


Hi Jerry,

Jerry Zhang  writes:

Hi all,

I've been looking for a way to handle custom device targeted control
requests from userspace. This would allow us to move away from needing
kernel patches to implement
https://source.android.com/devices/accessories/aoa. It seems like we are
close to being able to do this. With the flag FUNCTIONFS_ALL_CTRL_RECIP, an
f_fs function can handle all requests (device, interface, etc.) that are
not otherwise claimed, and functionfs already implements a pretty good
interface for userspace control requests through ep0. The issue is that
currently the function must be in the config in order to handle requests.
However, at any point in time we don't know which functionfs function is in
the config, or even that the config contains any ffs functions at all.

Here are my (early) thoughts on how to implement this feature. The goal is
to allow a function not in a configuration to handle control requests.

- Each configfs gadget contains a special config_desc called
"control_config". This acts as a normal config_desc except it is not added
back to cdev and instead is a new field in struct gadget_info.

- Functions can be linked into control_config. This is the same as a normal
usb_cfg_link. Functions linked this way can't be used in a normal config
since each function only has one list item.

- On configfs_bind, all functions in control_config are also bound. On
configfs_unbind, all functions in control_config are also unbound. Because
they aren't in cdev they don't appear in descriptors.

- Configfs will now have configfs_setup, which first attempts
composite_setup. If that fails, it will go through functions in
control_config and do the normal req_match / setup procedure. Since each
function here is bound and has a reference to cdev, it can successfully
match and handle control requests.

Thus a user that wants to handle all control requests can make a functionfs
instance "ctrlf" with dummy descriptors, and include the flag
ALL_CTRL_RECIP. Then they can link functions/ffs.ctrlf g1/control_config/f1
and handle control requests at /dev/usb-ffs/ctrlf/ep0.

A few advantages over a couple options I've considered are that this mostly
reuses existing functionalities and won't affect users that haven't enabled
it. Please let me know of any feedback on the design or any possible
implementation issues.


IMHO, considering the amount of Android users, we can merge this into
composite.c itself. Just make the code depend on CONFIG_ANDROID. Or
something along the lines of

if (IS_ENABLED(CONFIG_ANDROID))
android_audio_accessory_init();

should get the job done.


Huh and what with people that are not android but need to support 
Android accessory in their products?


Best regards,
--
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Handling custom device control requests in userspace

[Felipe Balbi]

Hi Jerry,

Jerry Zhang  writes:
> Hi all,
>
> I've been looking for a way to handle custom device targeted control
> requests from userspace. This would allow us to move away from needing
> kernel patches to implement
> https://source.android.com/devices/accessories/aoa. It seems like we are
> close to being able to do this. With the flag FUNCTIONFS_ALL_CTRL_RECIP, an
> f_fs function can handle all requests (device, interface, etc.) that are
> not otherwise claimed, and functionfs already implements a pretty good
> interface for userspace control requests through ep0. The issue is that
> currently the function must be in the config in order to handle requests.
> However, at any point in time we don't know which functionfs function is in
> the config, or even that the config contains any ffs functions at all.
>
> Here are my (early) thoughts on how to implement this feature. The goal is
> to allow a function not in a configuration to handle control requests.
>
> - Each configfs gadget contains a special config_desc called
> "control_config". This acts as a normal config_desc except it is not added
> back to cdev and instead is a new field in struct gadget_info.
>
> - Functions can be linked into control_config. This is the same as a normal
> usb_cfg_link. Functions linked this way can't be used in a normal config
> since each function only has one list item.
>
> - On configfs_bind, all functions in control_config are also bound. On
> configfs_unbind, all functions in control_config are also unbound. Because
> they aren't in cdev they don't appear in descriptors.
>
> - Configfs will now have configfs_setup, which first attempts
> composite_setup. If that fails, it will go through functions in
> control_config and do the normal req_match / setup procedure. Since each
> function here is bound and has a reference to cdev, it can successfully
> match and handle control requests.
>
> Thus a user that wants to handle all control requests can make a functionfs
> instance "ctrlf" with dummy descriptors, and include the flag
> ALL_CTRL_RECIP. Then they can link functions/ffs.ctrlf g1/control_config/f1
> and handle control requests at /dev/usb-ffs/ctrlf/ep0.
>
> A few advantages over a couple options I've considered are that this mostly
> reuses existing functionalities and won't affect users that haven't enabled
> it. Please let me know of any feedback on the design or any possible
> implementation issues.

IMHO, considering the amount of Android users, we can merge this into
composite.c itself. Just make the code depend on CONFIG_ANDROID. Or
something along the lines of

if (IS_ENABLED(CONFIG_ANDROID))
android_audio_accessory_init();

should get the job done.

-- 
balbi


signature.asc
Description: PGP signature

Re: Handling custom device control requests in userspace

[Andrzej Pietrasiewicz]

Hi Jerry,


W dniu 04.04.2018 o 02:04, Jerry Zhang pisze:

Hi all,

I've been looking for a way to handle custom device targeted control
requests from userspace. This would allow us to move away from needing
kernel patches to implement
https://source.android.com/devices/accessories/aoa. It seems like we are
close to being able to do this. With the flag FUNCTIONFS_ALL_CTRL_RECIP, an
f_fs function can handle all requests (device, interface, etc.) that are
not otherwise claimed, and functionfs already implements a pretty good
interface for userspace control requests through ep0. The issue is that
currently the function must be in the config in order to handle requests.
However, at any point in time we don't know which functionfs function is in
the config, or even that the config contains any ffs functions at all.

Here are my (early) thoughts on how to implement this feature. The goal is
to allow a function not in a configuration to handle control requests.

- Each configfs gadget contains a special config_desc called
"control_config". This acts as a normal config_desc except it is not added
back to cdev and instead is a new field in struct gadget_info.



- Functions can be linked into control_config. This is the same as a normal
usb_cfg_link. Functions linked this way can't be used in a normal config
since each function only has one list item.


If I understand correctly the purpose of creating a dedicated configuration
is to allow linking the functions to it and this way marking these functions
"special" (not assigned to a configuration proper).



- On configfs_bind, all functions in control_config are also bound. On
configfs_unbind, all functions in control_config are also unbound. Because
they aren't in cdev they don't appear in descriptors.

- Configfs will now have configfs_setup, which first attempts
composite_setup. If that fails, it will go through functions in
control_config and do the normal req_match / setup procedure. Since each
function here is bound and has a reference to cdev, it can successfully
match and handle control requests.

Thus a user that wants to handle all control requests can make a functionfs
instance "ctrlf" with dummy descriptors, and include the flag
ALL_CTRL_RECIP. Then they can link functions/ffs.ctrlf g1/control_config/f1
and handle control requests at /dev/usb-ffs/ctrlf/ep0.

A few advantages over a couple options I've considered are that this mostly
reuses existing functionalities and won't affect users that haven't enabled
it. Please let me know of any feedback on the design or any possible
implementation issues.



While I understand that engineers should discuss facts rather than feelings,
the idea of creating a special configuration doesn't feel right to me,
because this way you are creating a configuration which is not meant to be
ever selected by the host. I can be convinced with arguments, though.

I would like to draw your attention to one fact: a functionfs instance
directory in configfs is intentionally empty, since the functionfs delegates
actual implementation, including descriptors and strings, to userspace.
I would advise to consider (or at least prove wrong) the idea of adding some
attribute (file) to functionfs's configfs directory and using it for marking
this functionfs instance as special. This would require implementing some
exclusion mechanism (if this ffs instance is linked to a configuration,
it is impossible to mark it as special and vice versa).

Regards,

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