...
cdns3_gadget_ep_free_request(&priv_ep->endpoint, &priv_req->request);
list_del_init(&priv_req->list);
...
'priv_req' actually free at cdns3_gadget_ep_free_request(). But
list_del_init() use priv_req->list after it.
[ 1542.642868][ T534] BUG: KFENCE: use-after-free read in __list_del_entry_valid+0x10/0xd4
[ 1542.642868][ T534]
[ 1542.653162][ T534] Use-after-free read at 0x000000009ed0ba99 (in kfence-#3):
[ 1542.660311][ T534] __list_del_entry_valid+0x10/0xd4
[ 1542.665375][ T534] cdns3_gadget_ep_disable+0x1f8/0x388 [cdns3]
[ 1542.671571][ T534] usb_ep_disable+0x44/0xe4
[ 1542.675948][ T534] ffs_func_eps_disable+0x64/0xc8
[ 1542.680839][ T534] ffs_func_set_alt+0x74/0x368
[ 1542.685478][ T534] ffs_func_disable+0x18/0x28
Move list_del_init() before cdns3_gadget_ep_free_request() to resolve this
problem.
Cc: <[email protected]>
Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
Signed-off-by: Frank Li <[email protected]>
---
drivers/usb/cdns3/cdns3-gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
index aeca902ab6cc4..d6723d31fc6e2 100644
--- a/drivers/usb/cdns3/cdns3-gadget.c
+++ b/drivers/usb/cdns3/cdns3-gadget.c
@@ -2540,11 +2540,11 @@ static int cdns3_gadget_ep_disable(struct usb_ep *ep)
while (!list_empty(&priv_ep->wa2_descmiss_req_list)) {
priv_req = cdns3_next_priv_request(&priv_ep->wa2_descmiss_req_list);
+ list_del_init(&priv_req->list);
kfree(priv_req->request.buf);
cdns3_gadget_ep_free_request(&priv_ep->endpoint,
&priv_req->request);
- list_del_init(&priv_req->list);
--priv_ep->wa2_counter;
}
--
2.34.1
829 if (request->complete) {
830 spin_unlock(&priv_dev->lock);
831 usb_gadget_giveback_request(&priv_ep->endpoint,
832 request);
833 spin_lock(&priv_dev->lock);
834 }
835
836 if (request->buf == priv_dev->zlp_buf)
837 cdns3_gadget_ep_free_request(&priv_ep->endpoint, request);
Driver append an additional zero package request when queue a package,
which length mod max package size is 0. When transfer complete, run to
line 831, usb_gadget_giveback_request() will free this requestion. 836
condition is true, so cdns3_gadget_ep_free_request() free this request
again.
Log:
[ 1920.140696][ T150] BUG: KFENCE: use-after-free read in cdns3_gadget_giveback+0x134/0x2c0 [cdns3]
[ 1920.140696][ T150]
[ 1920.151837][ T150] Use-after-free read at 0x000000003d1cd10b (in kfence-#36):
[ 1920.159082][ T150] cdns3_gadget_giveback+0x134/0x2c0 [cdns3]
[ 1920.164988][ T150] cdns3_transfer_completed+0x438/0x5f8 [cdns3]
Add check at line 829, skip call usb_gadget_giveback_request() if it is
additional zero length package request. Needn't call
usb_gadget_giveback_request() because it is allocated in this driver.
Cc: <[email protected]>
Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
Signed-off-by: Frank Li <[email protected]>
---
drivers/usb/cdns3/cdns3-gadget.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
index d6723d31fc6e2..fd1beb10bba72 100644
--- a/drivers/usb/cdns3/cdns3-gadget.c
+++ b/drivers/usb/cdns3/cdns3-gadget.c
@@ -828,7 +828,11 @@ void cdns3_gadget_giveback(struct cdns3_endpoint *priv_ep,
return;
}
- if (request->complete) {
+ /*
+ * zlp request is appended by driver, needn't call usb_gadget_giveback_request() to notify
+ * gadget composite driver.
+ */
+ if (request->complete && request->buf != priv_dev->zlp_buf) {
spin_unlock(&priv_dev->lock);
usb_gadget_giveback_request(&priv_ep->endpoint,
request);
--
2.34.1
On 02/02/2024 07:18, Frank Li wrote:
> ...
> cdns3_gadget_ep_free_request(&priv_ep->endpoint, &priv_req->request);
> list_del_init(&priv_req->list);
> ...
>
> 'priv_req' actually free at cdns3_gadget_ep_free_request(). But
> list_del_init() use priv_req->list after it.
>
> [ 1542.642868][ T534] BUG: KFENCE: use-after-free read in __list_del_entry_valid+0x10/0xd4
> [ 1542.642868][ T534]
> [ 1542.653162][ T534] Use-after-free read at 0x000000009ed0ba99 (in kfence-#3):
> [ 1542.660311][ T534] __list_del_entry_valid+0x10/0xd4
> [ 1542.665375][ T534] cdns3_gadget_ep_disable+0x1f8/0x388 [cdns3]
> [ 1542.671571][ T534] usb_ep_disable+0x44/0xe4
> [ 1542.675948][ T534] ffs_func_eps_disable+0x64/0xc8
> [ 1542.680839][ T534] ffs_func_set_alt+0x74/0x368
> [ 1542.685478][ T534] ffs_func_disable+0x18/0x28
>
> Move list_del_init() before cdns3_gadget_ep_free_request() to resolve this
> problem.
>
> Cc: <[email protected]>
> Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
> Signed-off-by: Frank Li <[email protected]>
Reviewed-by: Roger Quadros <[email protected]>
Hello Frank,
On 02/02/2024 07:18, Frank Li wrote:
> 829 if (request->complete) {
> 830 spin_unlock(&priv_dev->lock);
> 831 usb_gadget_giveback_request(&priv_ep->endpoint,
> 832 request);
> 833 spin_lock(&priv_dev->lock);
> 834 }
> 835
> 836 if (request->buf == priv_dev->zlp_buf)
> 837 cdns3_gadget_ep_free_request(&priv_ep->endpoint, request);
>
> Driver append an additional zero package request when queue a package,
> which length mod max package size is 0. When transfer complete, run to
> line 831, usb_gadget_giveback_request() will free this requestion. 836
> condition is true, so cdns3_gadget_ep_free_request() free this request
> again.
Please replace "package" with "packet" in subject and description.
>
> Log:
>
> [ 1920.140696][ T150] BUG: KFENCE: use-after-free read in cdns3_gadget_giveback+0x134/0x2c0 [cdns3]
> [ 1920.140696][ T150]
> [ 1920.151837][ T150] Use-after-free read at 0x000000003d1cd10b (in kfence-#36):
> [ 1920.159082][ T150] cdns3_gadget_giveback+0x134/0x2c0 [cdns3]
> [ 1920.164988][ T150] cdns3_transfer_completed+0x438/0x5f8 [cdns3]
>
> Add check at line 829, skip call usb_gadget_giveback_request() if it is
> additional zero length package request. Needn't call
> usb_gadget_giveback_request() because it is allocated in this driver.
>
> Cc: <[email protected]>
> Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
> Signed-off-by: Frank Li <[email protected]>
> ---
> drivers/usb/cdns3/cdns3-gadget.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
> index d6723d31fc6e2..fd1beb10bba72 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.c
> +++ b/drivers/usb/cdns3/cdns3-gadget.c
> @@ -828,7 +828,11 @@ void cdns3_gadget_giveback(struct cdns3_endpoint *priv_ep,
> return;
> }
>
> - if (request->complete) {
> + /*
> + * zlp request is appended by driver, needn't call usb_gadget_giveback_request() to notify
> + * gadget composite driver.
> + */
> + if (request->complete && request->buf != priv_dev->zlp_buf) {
> spin_unlock(&priv_dev->lock);
> usb_gadget_giveback_request(&priv_ep->endpoint,
> request);
--
cheers,
-roger