| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20231206160356.GA3006@vmlxhi-118.adit-jv.com>
Date: Wed, 6 Dec 2023 17:03:56 +0100
From: Hardik Gajjar <hgajjar@...adit-jv.com>
To: John Keeping <john@...ping.me.uk>
CC: Hardik Gajjar <hgajjar@...adit-jv.com>,
<gregkh@...uxfoundation.org>, <stern@...land.harvard.edu>,
<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<erosca@...adit-jv.com>, <jlayton@...nel.org>, <brauner@...nel.org>
Subject: Re: [PATCH v2] usb: gadget: f_fs: Add the missing get_alt callback
On Sat, Dec 02, 2023 at 02:51:15PM +0000, John Keeping wrote:
> On Fri, Dec 01, 2023 at 03:52:34PM +0100, Hardik Gajjar wrote:
> > The Apple CarLife iAP gadget has a descriptor with two alternate
> > settings. The host sends the set_alt request to configure alt_setting
> > 0 or 1, and this is verified by the subsequent get_alt request.
> >
> > This patch implements and sets the get_alt callback. Without the
> > get_alt callback, composite.c abruptly concludes the
> > USB_REQ_GET/SET_INTERFACE request, assuming only one alt setting
> > for the endpoint.
> >
> > Signed-off-by: Hardik Gajjar <hgajjar@...adit-jv.com>
> > ---
> > changes since version 1:
> > - improve commit message to indicate why the get_alt callback
> > is necessary
> > - Link to v1 - https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20231124164435.74727-2D1-2Dhgajjar-40de.adit-2Djv.com_&d=DwICAg&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=SAhjP5GOmrADp1v_EE5jWoSuMlYCIt9gKduw-DCBPLs&m=HVW16RXWO_lhdf8dGUC8lUHmcMmonwJgRYsnIBtUkIvOKiQCwdrs5RXMJKo9Qhh9&s=tbwPiQ25PgliLyyD75FDRJqiFxR-1ExoOMIXOD4WEZI&e=
>
> This doesn't address my questions about v1 - I understand what the
> get_alt callback does, but I don't see how this is sufficient to make it
> work in all circumstances.
>
> To use your example of having settings 0 and 1, what happens if the host
> requests setting 2?
>
> Without this patch, when .get_alt is not set, composite_setup() will
> reject all settings except 0 so there is no need for ffs_func_set_alt()
> to filter its input. But if .get_alt is set, don't we need to filter
> for valid input here?
>
> I also do not see how it is acceptable to change alt setting without
> notifying userspace in the general case. If it works for your specific
> use case, that is one thing, but nothing requires the endpoint setup to
> be the same across alt settings and the userspace component likely needs
> to know if the setup changes.
>
> For examples, look at afunc_set_alt() or tcm_set_alt() in other gadget
> functions. If either of these were to be implemented in userspace then
> simply accepting the alt setting is not enough - there are changes that
> must be made to the functionality.
Indeed, we require a filter for the alt value, and my suggestion is to set the filter to allow alt values up to two.
The set_alt callback function in the f_fs driver is currently resetting the associated endpoints by disabling and enabling them again for positive alt values.
Additionally, it sets ffs_event FUNCTIONFS_ENABLE, which provide userspaces notifications about some change.
It appears that the same event mechanism is used when there is a request to set the alt setting to -1, notifying by FUNCTIONFS_DISABLE.
For example, you can find the link to the Android USB daemon utilizing this FFS event in userspace.
https://gerrit.pixelexperience.org/plugins/gitiles/system_core/+/b36b54cf796d722653ad9bea11284815a3f5eda5/adb/daemon/usb.cpp#331
It's evident that the f_fs driver significantly differs from other gadget drivers (e.g., ncm, uac), where major control resides in userspace.
I believe there's no need for a mechanism to notify userspace for the f_fs driver.
Userspace triggers the data read, eventually called by usb_ep_queue() in f_fs.
As soon as set_alt resets the associated endpoint, the UDC driver should call the endpoint callback with an error, smoothly terminating the userspace system call with the appropriate error.
I couldn't find any userspace notification implementation in tcm_set_alt.
Please let me correct if sounds inappropriate.
Thanks,
Hardik
>
> > ---
> > drivers/usb/gadget/function/f_fs.c | 16 +++++++++++++++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > index efe3e3b85769..37c47c11f57a 100644
> > --- a/drivers/usb/gadget/function/f_fs.c
> > +++ b/drivers/usb/gadget/function/f_fs.c
> > @@ -75,6 +75,7 @@ struct ffs_function {
> > short *interfaces_nums;
> >
> > struct usb_function function;
> > + int cur_alt[MAX_CONFIG_INTERFACES];
> > };
> >
> >
> > @@ -98,6 +99,7 @@ static int __must_check ffs_func_eps_enable(struct ffs_function *func);
> > static int ffs_func_bind(struct usb_configuration *,
> > struct usb_function *);
> > static int ffs_func_set_alt(struct usb_function *, unsigned, unsigned);
> > +static int ffs_func_get_alt(struct usb_function *f, unsigned int intf);
> > static void ffs_func_disable(struct usb_function *);
> > static int ffs_func_setup(struct usb_function *,
> > const struct usb_ctrlrequest *);
> > @@ -3232,6 +3234,15 @@ static void ffs_reset_work(struct work_struct *work)
> > ffs_data_reset(ffs);
> > }
> >
> > +static int ffs_func_get_alt(struct usb_function *f,
> > + unsigned int interface)
> > +{
> > + struct ffs_function *func = ffs_func_from_usb(f);
> > + int intf = ffs_func_revmap_intf(func, interface);
> > +
> > + return (intf < 0) ? intf : func->cur_alt[interface];
> > +}
> > +
> > static int ffs_func_set_alt(struct usb_function *f,
> > unsigned interface, unsigned alt)
> > {
> > @@ -3266,8 +3277,10 @@ static int ffs_func_set_alt(struct usb_function *f,
> >
> > ffs->func = func;
> > ret = ffs_func_eps_enable(func);
> > - if (ret >= 0)
> > + if (ret >= 0) {
> > ffs_event_add(ffs, FUNCTIONFS_ENABLE);
> > + func->cur_alt[interface] = alt;
> > + }
> > return ret;
> > }
> >
> > @@ -3574,6 +3587,7 @@ static struct usb_function *ffs_alloc(struct usb_function_instance *fi)
> > func->function.bind = ffs_func_bind;
> > func->function.unbind = ffs_func_unbind;
> > func->function.set_alt = ffs_func_set_alt;
> > + func->function.get_alt = ffs_func_get_alt;
> > func->function.disable = ffs_func_disable;
> > func->function.setup = ffs_func_setup;
> > func->function.req_match = ffs_func_req_match;
> > --
> > 2.17.1
> >
Powered by blists - more mailing lists