linux-kernel - Re: [PATCH v5 2/3] usb: gadget: udc: core: Invoke usb_gadget

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <618e4f17-2799-4838-a21c-184c9303bef6@rowland.harvard.edu>
Date:   Wed, 31 May 2023 13:55:18 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     Badhri Jagan Sridharan <badhri@...gle.com>
Cc:     gregkh@...uxfoundation.org, colin.i.king@...il.com,
        xuetao09@...wei.com, quic_eserrao@...cinc.com,
        water.zhangjiantao@...wei.com, francesco@...cini.it,
        alistair@...stair23.me, stephan@...hold.net, bagasdotme@...il.com,
        luca@...tu.xyz, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v5 2/3] usb: gadget: udc: core: Invoke usb_gadget_connect
 only when started

On Wed, May 31, 2023 at 04:02:02AM +0000, Badhri Jagan Sridharan wrote:
> usb_udc_connect_control() does not check to see if the udc has already
> been started. This causes gadget->ops->pullup to be called through
> usb_gadget_connect() when invoked from usb_udc_vbus_handler() even
> before usb_gadget_udc_start() is called. Guard this by checking for
> udc->started in usb_udc_connect_control() before invoking
> usb_gadget_connect().

After a merged version of patches 1/3 and 3/3 have been applied, it 
seems like most of this will not be needed any more.  Maybe not any of 
it.

usb_udc_connect_control() gets called from only two places.  One of them 
is in gadget_bind_driver(), where we know that the UDC has been started 
and connecting is allowed.  The other place is the vbus work routine 
queued by usb_udc_vbus_handler().  If that place checks the new 
allow_connect flag before calling usb_gadget_connect(), nothing more 
will be needed.  You just have to make sure that the allow_connect flag 
is set in gadget_bind_driver between the start and connect_control 
calls, and it is cleared in gadget_unbind_driver before the 
cancel_work_sync call.

It's possible that a new mutex will be needed to synchronize accesses to 
the allow_connect flag.  That's something you will have to study and 
decide on.  But if you can avoid adding one, that would be best.

> Guarding udc->vbus, udc->started, gadget->connect, gadget->deactivate
> related functions with connect_lock. usb_gadget_connect_locked(),
> usb_gadget_disconnect_locked(), usb_udc_connect_control_locked(),
> usb_gadget_udc_start_locked(), usb_gadget_udc_stop_locked() are called
> with this lock held as they can be simulataneously invoked from
> different code paths.

It's a general principle of kernel programming that locks protect data, 
not code.  So if this patch were to be accepted, you would have to 
change this description to say that connect_lock guards various flags, 
not various function calls.

Alan Stern