Merge patch series "usb: gadget: Refactor function drivers to use __free() cleanup"

Kuen-Han Tsai <khtsai@google.com> says:

This patch series refactors the error-handling paths in the bind()
function for f_ncm, f_acm, f_ecm, and f_rndis drivers.

The current, unified goto logic in these drivers is vulnerable to a null
pointer dereference. This is caused by the cleanup logic incorrectly
handling the stale usb_request pointer after a bind/unbind cycle. This
series fixes this issue by converting the drivers to use the modern
__free() scope-based cleanup mechanism.

Patches 1-2 are preparatory, adding the endpoint pointer to struct
usb_request and defining helpers for the __free() cleanup. The remaining
four patches use this new plumbing to refactor each driver.

Future work
-----------
1. Refactor usb_ep_free_request(), usb_ep_queue(), and usb_ep_dequeue()
   functions as the ep parameter becomes redudant.
2. Convert the remaining gadget function drivers to use the new __free()
   cleanup mechanism.

Link: https://lore.kernel.org/r/20250916-ready-v1-0-4997bf277548@google.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
This commit is contained in:
Greg Kroah-Hartman
2025-09-17 12:44:55 +02:00
6 changed files with 135 additions and 146 deletions
+19 -23
View File
@@ -11,12 +11,15 @@
/* #define VERBOSE_DEBUG */
#include <linux/cleanup.h>
#include <linux/slab.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/device.h>
#include <linux/err.h>
#include <linux/usb/gadget.h>
#include "u_serial.h"
@@ -613,6 +616,7 @@ acm_bind(struct usb_configuration *c, struct usb_function *f)
struct usb_string *us;
int status;
struct usb_ep *ep;
struct usb_request *request __free(free_usb_request) = NULL;
/* REVISIT might want instance-specific strings to help
* distinguish instances ...
@@ -630,7 +634,7 @@ acm_bind(struct usb_configuration *c, struct usb_function *f)
/* allocate instance-specific interface IDs, and patch descriptors */
status = usb_interface_id(c, f);
if (status < 0)
goto fail;
return status;
acm->ctrl_id = status;
acm_iad_descriptor.bFirstInterface = status;
@@ -639,43 +643,41 @@ acm_bind(struct usb_configuration *c, struct usb_function *f)
status = usb_interface_id(c, f);
if (status < 0)
goto fail;
return status;
acm->data_id = status;
acm_data_interface_desc.bInterfaceNumber = status;
acm_union_desc.bSlaveInterface0 = status;
acm_call_mgmt_descriptor.bDataInterface = status;
status = -ENODEV;
/* allocate instance-specific endpoints */
ep = usb_ep_autoconfig(cdev->gadget, &acm_fs_in_desc);
if (!ep)
goto fail;
return -ENODEV;
acm->port.in = ep;
ep = usb_ep_autoconfig(cdev->gadget, &acm_fs_out_desc);
if (!ep)
goto fail;
return -ENODEV;
acm->port.out = ep;
ep = usb_ep_autoconfig(cdev->gadget, &acm_fs_notify_desc);
if (!ep)
goto fail;
return -ENODEV;
acm->notify = ep;
acm_iad_descriptor.bFunctionProtocol = acm->bInterfaceProtocol;
acm_control_interface_desc.bInterfaceProtocol = acm->bInterfaceProtocol;
/* allocate notification */
acm->notify_req = gs_alloc_req(ep,
sizeof(struct usb_cdc_notification) + 2,
GFP_KERNEL);
if (!acm->notify_req)
goto fail;
request = gs_alloc_req(ep,
sizeof(struct usb_cdc_notification) + 2,
GFP_KERNEL);
if (!request)
return -ENODEV;
acm->notify_req->complete = acm_cdc_notify_complete;
acm->notify_req->context = acm;
request->complete = acm_cdc_notify_complete;
request->context = acm;
/* support all relevant hardware speeds... we expect that when
* hardware is dual speed, all bulk-capable endpoints work at
@@ -692,7 +694,9 @@ acm_bind(struct usb_configuration *c, struct usb_function *f)
status = usb_assign_descriptors(f, acm_fs_function, acm_hs_function,
acm_ss_function, acm_ss_function);
if (status)
goto fail;
return status;
acm->notify_req = no_free_ptr(request);
dev_dbg(&cdev->gadget->dev,
"acm ttyGS%d: IN/%s OUT/%s NOTIFY/%s\n",
@@ -700,14 +704,6 @@ acm_bind(struct usb_configuration *c, struct usb_function *f)
acm->port.in->name, acm->port.out->name,
acm->notify->name);
return 0;
fail:
if (acm->notify_req)
gs_free_req(acm->notify, acm->notify_req);
ERROR(cdev, "%s/%p: can't bind, err %d\n", f->name, f, status);
return status;
}
static void acm_unbind(struct usb_configuration *c, struct usb_function *f)
+20 -28
View File
@@ -8,6 +8,7 @@
/* #define VERBOSE_DEBUG */
#include <linux/cleanup.h>
#include <linux/slab.h>
#include <linux/kernel.h>
#include <linux/module.h>
@@ -15,6 +16,8 @@
#include <linux/etherdevice.h>
#include <linux/string_choices.h>
#include <linux/usb/gadget.h>
#include "u_ether.h"
#include "u_ether_configfs.h"
#include "u_ecm.h"
@@ -678,6 +681,7 @@ ecm_bind(struct usb_configuration *c, struct usb_function *f)
struct usb_ep *ep;
struct f_ecm_opts *ecm_opts;
struct usb_request *request __free(free_usb_request) = NULL;
if (!can_support_ecm(cdev->gadget))
return -EINVAL;
@@ -711,7 +715,7 @@ ecm_bind(struct usb_configuration *c, struct usb_function *f)
/* allocate instance-specific interface IDs */
status = usb_interface_id(c, f);
if (status < 0)
goto fail;
return status;
ecm->ctrl_id = status;
ecm_iad_descriptor.bFirstInterface = status;
@@ -720,24 +724,22 @@ ecm_bind(struct usb_configuration *c, struct usb_function *f)
status = usb_interface_id(c, f);
if (status < 0)
goto fail;
return status;
ecm->data_id = status;
ecm_data_nop_intf.bInterfaceNumber = status;
ecm_data_intf.bInterfaceNumber = status;
ecm_union_desc.bSlaveInterface0 = status;
status = -ENODEV;
/* allocate instance-specific endpoints */
ep = usb_ep_autoconfig(cdev->gadget, &fs_ecm_in_desc);
if (!ep)
goto fail;
return -ENODEV;
ecm->port.in_ep = ep;
ep = usb_ep_autoconfig(cdev->gadget, &fs_ecm_out_desc);
if (!ep)
goto fail;
return -ENODEV;
ecm->port.out_ep = ep;
/* NOTE: a status/notification endpoint is *OPTIONAL* but we
@@ -746,20 +748,18 @@ ecm_bind(struct usb_configuration *c, struct usb_function *f)
*/
ep = usb_ep_autoconfig(cdev->gadget, &fs_ecm_notify_desc);
if (!ep)
goto fail;
return -ENODEV;
ecm->notify = ep;
status = -ENOMEM;
/* allocate notification request and buffer */
ecm->notify_req = usb_ep_alloc_request(ep, GFP_KERNEL);
if (!ecm->notify_req)
goto fail;
ecm->notify_req->buf = kmalloc(ECM_STATUS_BYTECOUNT, GFP_KERNEL);
if (!ecm->notify_req->buf)
goto fail;
ecm->notify_req->context = ecm;
ecm->notify_req->complete = ecm_notify_complete;
request = usb_ep_alloc_request(ep, GFP_KERNEL);
if (!request)
return -ENOMEM;
request->buf = kmalloc(ECM_STATUS_BYTECOUNT, GFP_KERNEL);
if (!request->buf)
return -ENOMEM;
request->context = ecm;
request->complete = ecm_notify_complete;
/* support all relevant hardware speeds... we expect that when
* hardware is dual speed, all bulk-capable endpoints work at
@@ -778,7 +778,7 @@ ecm_bind(struct usb_configuration *c, struct usb_function *f)
status = usb_assign_descriptors(f, ecm_fs_function, ecm_hs_function,
ecm_ss_function, ecm_ss_function);
if (status)
goto fail;
return status;
/* NOTE: all that is done without knowing or caring about
* the network link ... which is unavailable to this code
@@ -788,20 +788,12 @@ ecm_bind(struct usb_configuration *c, struct usb_function *f)
ecm->port.open = ecm_open;
ecm->port.close = ecm_close;
ecm->notify_req = no_free_ptr(request);
DBG(cdev, "CDC Ethernet: IN/%s OUT/%s NOTIFY/%s\n",
ecm->port.in_ep->name, ecm->port.out_ep->name,
ecm->notify->name);
return 0;
fail:
if (ecm->notify_req) {
kfree(ecm->notify_req->buf);
usb_ep_free_request(ecm->notify, ecm->notify_req);
}
ERROR(cdev, "%s: can't bind, err %d\n", f->name, status);
return status;
}
static inline struct f_ecm_opts *to_f_ecm_opts(struct config_item *item)
+33 -45
View File
@@ -11,6 +11,7 @@
* Copyright (C) 2008 Nokia Corporation
*/
#include <linux/cleanup.h>
#include <linux/kernel.h>
#include <linux/interrupt.h>
#include <linux/module.h>
@@ -20,6 +21,7 @@
#include <linux/string_choices.h>
#include <linux/usb/cdc.h>
#include <linux/usb/gadget.h>
#include "u_ether.h"
#include "u_ether_configfs.h"
@@ -1436,18 +1438,18 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
struct usb_ep *ep;
struct f_ncm_opts *ncm_opts;
struct usb_os_desc_table *os_desc_table __free(kfree) = NULL;
struct usb_request *request __free(free_usb_request) = NULL;
if (!can_support_ecm(cdev->gadget))
return -EINVAL;
ncm_opts = container_of(f->fi, struct f_ncm_opts, func_inst);
if (cdev->use_os_string) {
f->os_desc_table = kzalloc(sizeof(*f->os_desc_table),
GFP_KERNEL);
if (!f->os_desc_table)
os_desc_table = kzalloc(sizeof(*os_desc_table), GFP_KERNEL);
if (!os_desc_table)
return -ENOMEM;
f->os_desc_n = 1;
f->os_desc_table[0].os_desc = &ncm_opts->ncm_os_desc;
}
mutex_lock(&ncm_opts->lock);
@@ -1459,7 +1461,7 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
mutex_unlock(&ncm_opts->lock);
if (status)
goto fail;
return status;
ncm_opts->bound = true;
@@ -1467,10 +1469,9 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
us = usb_gstrings_attach(cdev, ncm_strings,
ARRAY_SIZE(ncm_string_defs));
if (IS_ERR(us)) {
status = PTR_ERR(us);
goto fail;
}
if (IS_ERR(us))
return PTR_ERR(us);
ncm_control_intf.iInterface = us[STRING_CTRL_IDX].id;
ncm_data_nop_intf.iInterface = us[STRING_DATA_IDX].id;
ncm_data_intf.iInterface = us[STRING_DATA_IDX].id;
@@ -1480,20 +1481,16 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
/* allocate instance-specific interface IDs */
status = usb_interface_id(c, f);
if (status < 0)
goto fail;
return status;
ncm->ctrl_id = status;
ncm_iad_desc.bFirstInterface = status;
ncm_control_intf.bInterfaceNumber = status;
ncm_union_desc.bMasterInterface0 = status;
if (cdev->use_os_string)
f->os_desc_table[0].if_id =
ncm_iad_desc.bFirstInterface;
status = usb_interface_id(c, f);
if (status < 0)
goto fail;
return status;
ncm->data_id = status;
ncm_data_nop_intf.bInterfaceNumber = status;
@@ -1502,35 +1499,31 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
ecm_desc.wMaxSegmentSize = cpu_to_le16(ncm_opts->max_segment_size);
status = -ENODEV;
/* allocate instance-specific endpoints */
ep = usb_ep_autoconfig(cdev->gadget, &fs_ncm_in_desc);
if (!ep)
goto fail;
return -ENODEV;
ncm->port.in_ep = ep;
ep = usb_ep_autoconfig(cdev->gadget, &fs_ncm_out_desc);
if (!ep)
goto fail;
return -ENODEV;
ncm->port.out_ep = ep;
ep = usb_ep_autoconfig(cdev->gadget, &fs_ncm_notify_desc);
if (!ep)
goto fail;
return -ENODEV;
ncm->notify = ep;
status = -ENOMEM;
/* allocate notification request and buffer */
ncm->notify_req = usb_ep_alloc_request(ep, GFP_KERNEL);
if (!ncm->notify_req)
goto fail;
ncm->notify_req->buf = kmalloc(NCM_STATUS_BYTECOUNT, GFP_KERNEL);
if (!ncm->notify_req->buf)
goto fail;
ncm->notify_req->context = ncm;
ncm->notify_req->complete = ncm_notify_complete;
request = usb_ep_alloc_request(ep, GFP_KERNEL);
if (!request)
return -ENOMEM;
request->buf = kmalloc(NCM_STATUS_BYTECOUNT, GFP_KERNEL);
if (!request->buf)
return -ENOMEM;
request->context = ncm;
request->complete = ncm_notify_complete;
/*
* support all relevant hardware speeds... we expect that when
@@ -1550,7 +1543,7 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
status = usb_assign_descriptors(f, ncm_fs_function, ncm_hs_function,
ncm_ss_function, ncm_ss_function);
if (status)
goto fail;
return status;
/*
* NOTE: all that is done without knowing or caring about
@@ -1563,23 +1556,18 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
hrtimer_setup(&ncm->task_timer, ncm_tx_timeout, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
if (cdev->use_os_string) {
os_desc_table[0].os_desc = &ncm_opts->ncm_os_desc;
os_desc_table[0].if_id = ncm_iad_desc.bFirstInterface;
f->os_desc_table = no_free_ptr(os_desc_table);
f->os_desc_n = 1;
}
ncm->notify_req = no_free_ptr(request);
DBG(cdev, "CDC Network: IN/%s OUT/%s NOTIFY/%s\n",
ncm->port.in_ep->name, ncm->port.out_ep->name,
ncm->notify->name);
return 0;
fail:
kfree(f->os_desc_table);
f->os_desc_n = 0;
if (ncm->notify_req) {
kfree(ncm->notify_req->buf);
usb_ep_free_request(ncm->notify, ncm->notify_req);
}
ERROR(cdev, "%s: can't bind, err %d\n", f->name, status);
return status;
}
static inline struct f_ncm_opts *to_f_ncm_opts(struct config_item *item)
+35 -50
View File
@@ -19,6 +19,8 @@
#include <linux/atomic.h>
#include <linux/usb/gadget.h>
#include "u_ether.h"
#include "u_ether_configfs.h"
#include "u_rndis.h"
@@ -662,6 +664,8 @@ rndis_bind(struct usb_configuration *c, struct usb_function *f)
struct usb_ep *ep;
struct f_rndis_opts *rndis_opts;
struct usb_os_desc_table *os_desc_table __free(kfree) = NULL;
struct usb_request *request __free(free_usb_request) = NULL;
if (!can_support_rndis(c))
return -EINVAL;
@@ -669,12 +673,9 @@ rndis_bind(struct usb_configuration *c, struct usb_function *f)
rndis_opts = container_of(f->fi, struct f_rndis_opts, func_inst);
if (cdev->use_os_string) {
f->os_desc_table = kzalloc(sizeof(*f->os_desc_table),
GFP_KERNEL);
if (!f->os_desc_table)
os_desc_table = kzalloc(sizeof(*os_desc_table), GFP_KERNEL);
if (!os_desc_table)
return -ENOMEM;
f->os_desc_n = 1;
f->os_desc_table[0].os_desc = &rndis_opts->rndis_os_desc;
}
rndis_iad_descriptor.bFunctionClass = rndis_opts->class;
@@ -692,16 +693,14 @@ rndis_bind(struct usb_configuration *c, struct usb_function *f)
gether_set_gadget(rndis_opts->net, cdev->gadget);
status = gether_register_netdev(rndis_opts->net);
if (status)
goto fail;
return status;
rndis_opts->bound = true;
}
us = usb_gstrings_attach(cdev, rndis_strings,
ARRAY_SIZE(rndis_string_defs));
if (IS_ERR(us)) {
status = PTR_ERR(us);
goto fail;
}
if (IS_ERR(us))
return PTR_ERR(us);
rndis_control_intf.iInterface = us[0].id;
rndis_data_intf.iInterface = us[1].id;
rndis_iad_descriptor.iFunction = us[2].id;
@@ -709,36 +708,30 @@ rndis_bind(struct usb_configuration *c, struct usb_function *f)
/* allocate instance-specific interface IDs */
status = usb_interface_id(c, f);
if (status < 0)
goto fail;
return status;
rndis->ctrl_id = status;
rndis_iad_descriptor.bFirstInterface = status;
rndis_control_intf.bInterfaceNumber = status;
rndis_union_desc.bMasterInterface0 = status;
if (cdev->use_os_string)
f->os_desc_table[0].if_id =
rndis_iad_descriptor.bFirstInterface;
status = usb_interface_id(c, f);
if (status < 0)
goto fail;
return status;
rndis->data_id = status;
rndis_data_intf.bInterfaceNumber = status;
rndis_union_desc.bSlaveInterface0 = status;
status = -ENODEV;
/* allocate instance-specific endpoints */
ep = usb_ep_autoconfig(cdev->gadget, &fs_in_desc);
if (!ep)
goto fail;
return -ENODEV;
rndis->port.in_ep = ep;
ep = usb_ep_autoconfig(cdev->gadget, &fs_out_desc);
if (!ep)
goto fail;
return -ENODEV;
rndis->port.out_ep = ep;
/* NOTE: a status/notification endpoint is, strictly speaking,
@@ -747,21 +740,19 @@ rndis_bind(struct usb_configuration *c, struct usb_function *f)
*/
ep = usb_ep_autoconfig(cdev->gadget, &fs_notify_desc);
if (!ep)
goto fail;
return -ENODEV;
rndis->notify = ep;
status = -ENOMEM;
/* allocate notification request and buffer */
rndis->notify_req = usb_ep_alloc_request(ep, GFP_KERNEL);
if (!rndis->notify_req)
goto fail;
rndis->notify_req->buf = kmalloc(STATUS_BYTECOUNT, GFP_KERNEL);
if (!rndis->notify_req->buf)
goto fail;
rndis->notify_req->length = STATUS_BYTECOUNT;
rndis->notify_req->context = rndis;
rndis->notify_req->complete = rndis_response_complete;
request = usb_ep_alloc_request(ep, GFP_KERNEL);
if (!request)
return -ENOMEM;
request->buf = kmalloc(STATUS_BYTECOUNT, GFP_KERNEL);
if (!request->buf)
return -ENOMEM;
request->length = STATUS_BYTECOUNT;
request->context = rndis;
request->complete = rndis_response_complete;
/* support all relevant hardware speeds... we expect that when
* hardware is dual speed, all bulk-capable endpoints work at
@@ -778,7 +769,7 @@ rndis_bind(struct usb_configuration *c, struct usb_function *f)
status = usb_assign_descriptors(f, eth_fs_function, eth_hs_function,
eth_ss_function, eth_ss_function);
if (status)
goto fail;
return status;
rndis->port.open = rndis_open;
rndis->port.close = rndis_close;
@@ -789,10 +780,19 @@ rndis_bind(struct usb_configuration *c, struct usb_function *f)
if (rndis->manufacturer && rndis->vendorID &&
rndis_set_param_vendor(rndis->params, rndis->vendorID,
rndis->manufacturer)) {
status = -EINVAL;
goto fail_free_descs;
usb_free_all_descriptors(f);
return -EINVAL;
}
if (cdev->use_os_string) {
os_desc_table[0].os_desc = &rndis_opts->rndis_os_desc;
os_desc_table[0].if_id = rndis_iad_descriptor.bFirstInterface;
f->os_desc_table = no_free_ptr(os_desc_table);
f->os_desc_n = 1;
}
rndis->notify_req = no_free_ptr(request);
/* NOTE: all that is done without knowing or caring about
* the network link ... which is unavailable to this code
* until we're activated via set_alt().
@@ -802,21 +802,6 @@ rndis_bind(struct usb_configuration *c, struct usb_function *f)
rndis->port.in_ep->name, rndis->port.out_ep->name,
rndis->notify->name);
return 0;
fail_free_descs:
usb_free_all_descriptors(f);
fail:
kfree(f->os_desc_table);
f->os_desc_n = 0;
if (rndis->notify_req) {
kfree(rndis->notify_req->buf);
usb_ep_free_request(rndis->notify, rndis->notify_req);
}
ERROR(cdev, "%s: can't bind, err %d\n", f->name, status);
return status;
}
void rndis_borrow_net(struct usb_function_instance *f, struct net_device *net)
+3
View File
@@ -194,6 +194,9 @@ struct usb_request *usb_ep_alloc_request(struct usb_ep *ep,
req = ep->ops->alloc_request(ep, gfp_flags);
if (req)
req->ep = ep;
trace_usb_ep_alloc_request(ep, req, req ? 0 : -ENOMEM);
return req;
+25
View File
@@ -15,6 +15,7 @@
#ifndef __LINUX_USB_GADGET_H
#define __LINUX_USB_GADGET_H
#include <linux/cleanup.h>
#include <linux/configfs.h>
#include <linux/device.h>
#include <linux/errno.h>
@@ -32,6 +33,7 @@ struct usb_ep;
/**
* struct usb_request - describes one i/o request
* @ep: The associated endpoint set by usb_ep_alloc_request().
* @buf: Buffer used for data. Always provide this; some controllers
* only use PIO, or don't use DMA for some endpoints.
* @dma: DMA address corresponding to 'buf'. If you don't set this
@@ -98,6 +100,7 @@ struct usb_ep;
*/
struct usb_request {
struct usb_ep *ep;
void *buf;
unsigned length;
dma_addr_t dma;
@@ -291,6 +294,28 @@ static inline void usb_ep_fifo_flush(struct usb_ep *ep)
/*-------------------------------------------------------------------------*/
/**
* free_usb_request - frees a usb_request object and its buffer
* @req: the request being freed
*
* This helper function frees both the request's buffer and the request object
* itself by calling usb_ep_free_request(). Its signature is designed to be used
* with DEFINE_FREE() to enable automatic, scope-based cleanup for usb_request
* pointers.
*/
static inline void free_usb_request(struct usb_request *req)
{
if (!req)
return;
kfree(req->buf);
usb_ep_free_request(req->ep, req);
}
DEFINE_FREE(free_usb_request, struct usb_request *, free_usb_request(_T))
/*-------------------------------------------------------------------------*/
struct usb_dcd_config_params {
__u8 bU1devExitLat; /* U1 Device exit Latency */
#define USB_DEFAULT_U1_DEV_EXIT_LAT 0x01 /* Less then 1 microsec */