* [PATCH 0/2] Fix two NULL pointer dereferences when role-switching
@ 2020-01-09 13:17 Bryan O'Donoghue
2020-01-09 13:17 ` [PATCH 1/2] usb: gadget: f_ncm: Use atomic_t to track in-flight request Bryan O'Donoghue
2020-01-09 13:17 ` [PATCH 2/2] usb: gadget: f_ecm: " Bryan O'Donoghue
0 siblings, 2 replies; 3+ messages in thread
From: Bryan O'Donoghue @ 2020-01-09 13:17 UTC (permalink / raw)
To: balbi, gregkh, linux-usb, linux-kernel; +Cc: Bryan O'Donoghue
Both the f_ncm and f_ecm drivers use setting of [ne]cm->notify_req to NULL
to indicate a request is in-flight. This however can lead to a NULL pointer
dereference in the ubind() path of both drivers.
These two patches fix the error by using an atomic_t as a flag like is done
in the f_rndis driver.
# Setup
mount -t configfs none /sys/kernel/config/
cd /sys/kernel/config
cd usb_gadget/
mkdir g1
cd g1/
echo 0x1209 > idVendor
echo 0x0001 > idProduct
mkdir strings/0x409
echo 0123456789 > strings/0x409/serialnumber
echo B0D > strings/0x409/manufacturer
echo B0D-device > strings/0x409/product
mkdir functions/ncm.usb0
mkdir configs/c.1
mkdir configs/c.1/strings/0x409
echo CDC NCM > configs/c.1/strings/0x409/configuration
ln -s functions/ncm.usb0 configs/c.1
echo 7580000.dwc3 > UDC
# Give both ends an IP
device:
ifconfig usb0 192.168.8.2
host:
ifconfig usbX 192.168.8.1
ping 192.168.8.2
# Pull out device cable
# Attach a device to the target triggering a role switch and an unbind()
[ 115.776303] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[ 115.776326] Mem abort info:
[ 115.784159] ESR = 0x96000006
[ 115.786653] EC = 0x25: DABT (current EL), IL = 32 bits
[ 115.789798] SET = 0, FnV = 0
[ 115.795255] EA = 0, S1PTW = 0
[ 115.798121] Data abort info:
[ 115.801157] ISV = 0, ISS = 0x00000006
[ 115.804286] CM = 0, WnR = 0
[ 115.807845] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000083919000
[ 115.810962] [0000000000000000] pgd=000000008391f003, pud=0000000083920003, pmd=0000000000000000
[ 115.817406] Internal error: Oops: 96000006 [#1] PREEMPT SMP
[ 115.825892] Modules linked in:
[ 115.831447] CPU: 0 PID: 34 Comm: kworker/0:1 Not tainted 5.5.0-rc3-00075-gb6e9f933acdf #161
[ 115.834577] Hardware name: Qualcomm Technologies, Inc. QCS404 EVB 4000 (DT)
[ 115.842830] Workqueue: events_freezable __dwc3_set_mode
[ 115.849764] pstate: 80000005 (Nzcv daif -PAN -UAO)
[ 115.854973] pc : ncm_unbind+0x58/0x78
[ 115.859832] lr : ncm_unbind+0x54/0x78
[ 115.863562] sp : ffff80001014bc70
[ 115.867208] x29: ffff80001014bc70 x28: 0000000000000000
[ 115.870510] x27: ffffce10ed07a510 x26: ffffce10ecf959b8
[ 115.875891] x25: ffff00003daa9000 x24: ffff00003daa9490
[ 115.881186] x23: ffff00003da8f200 x22: ffff00003da8f168
[ 115.886482] x21: ffff00003da8f0b8 x20: ffff00003da8f120
[ 115.891776] x19: ffff00003d05fa00 x18: ffffffffffffffff
[ 115.897072] x17: 0000000000000000 x16: 0000000000000000
[ 115.902366] x15: ffffce10ed6ae000 x14: 00000000fffffff0
[ 115.907662] x13: ffffce10ed883fb0 x12: ffffce10ed6ae000
[ 115.912957] x11: 0000000000000000 x10: 0000000000000000
[ 115.918252] x9 : 0000000000000007 x8 : 0000000040000000
[ 115.923546] x7 : 0000000000000000 x6 : 000000008010000d
[ 115.928842] x5 : ffffce10ec793638 x4 : ffff000039772600
[ 115.934137] x3 : 000000008010000d x2 : fffffe0000c5dca0
[ 115.939433] x1 : 4625c8679f119400 x0 : 0000000000000000
[ 115.944729] Call trace:
[ 115.950021] ncm_unbind+0x58/0x78
[ 115.952193] purge_configs_funcs+0x130/0x138
[ 115.955666] configfs_composite_unbind+0x58/0x98
[ 115.960007] usb_gadget_remove_driver+0x54/0x88
[ 115.964607] usb_del_gadget_udc+0x8c/0xf8
[ 115.968860] dwc3_gadget_exit+0x18/0x68
[ 115.973026] __dwc3_set_mode+0x138/0x258
[ 115.976675] process_one_work+0x1e0/0x358
[ 115.980839] worker_thread+0x40/0x488
[ 115.984746] kthread+0x118/0x120
[ 115.988391] ret_from_fork+0x10/0x18
[ 115.991691] Code: aa1303e0 391e603f 97ffb836 f940a260 (f9400000)
[ 115.995253] ---[ end trace ab25b53d409d0cf3 ]---
Bryan O'Donoghue (2):
usb: gadget: f_ncm: Use atomic_t to track in-flight request
usb: gadget: f_ecm: Use atomic_t to track in-flight request
drivers/usb/gadget/function/f_ecm.c | 16 ++++++++++++----
drivers/usb/gadget/function/f_ncm.c | 17 +++++++++++++----
2 files changed, 25 insertions(+), 8 deletions(-)
--
2.24.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 1/2] usb: gadget: f_ncm: Use atomic_t to track in-flight request
2020-01-09 13:17 [PATCH 0/2] Fix two NULL pointer dereferences when role-switching Bryan O'Donoghue
@ 2020-01-09 13:17 ` Bryan O'Donoghue
2020-01-09 13:17 ` [PATCH 2/2] usb: gadget: f_ecm: " Bryan O'Donoghue
1 sibling, 0 replies; 3+ messages in thread
From: Bryan O'Donoghue @ 2020-01-09 13:17 UTC (permalink / raw)
To: balbi, gregkh, linux-usb, linux-kernel; +Cc: Bryan O'Donoghue
Currently ncm->notify_req is used to flag when a request is in-flight.
ncm->notify_req is set to NULL and when a request completes it is
subsequently reset.
This is fundamentally buggy in that the unbind logic of the NCM driver will
unconditionally free ncm->notify_req leading to a NULL pointer dereference.
Fixes: 40d133d7f5426 ("usb: gadget: f_ncm: convert to new function interface
with backward compatibility")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
drivers/usb/gadget/function/f_ncm.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index 2d6e76e4cffa..1d900081b1f0 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -53,6 +53,7 @@ struct f_ncm {
struct usb_ep *notify;
struct usb_request *notify_req;
u8 notify_state;
+ atomic_t notify_count;
bool is_open;
const struct ndp_parser_opts *parser_opts;
@@ -547,7 +548,7 @@ static void ncm_do_notify(struct f_ncm *ncm)
int status;
/* notification already in flight? */
- if (!req)
+ if (atomic_read(&ncm->notify_count))
return;
event = req->buf;
@@ -587,7 +588,8 @@ static void ncm_do_notify(struct f_ncm *ncm)
event->bmRequestType = 0xA1;
event->wIndex = cpu_to_le16(ncm->ctrl_id);
- ncm->notify_req = NULL;
+ atomic_inc(&ncm->notify_count);
+
/*
* In double buffering if there is a space in FIFO,
* completion callback can be called right after the call,
@@ -597,7 +599,7 @@ static void ncm_do_notify(struct f_ncm *ncm)
status = usb_ep_queue(ncm->notify, req, GFP_ATOMIC);
spin_lock(&ncm->lock);
if (status < 0) {
- ncm->notify_req = req;
+ atomic_dec(&ncm->notify_count);
DBG(cdev, "notify --> %d\n", status);
}
}
@@ -632,17 +634,19 @@ static void ncm_notify_complete(struct usb_ep *ep, struct usb_request *req)
case 0:
VDBG(cdev, "Notification %02x sent\n",
event->bNotificationType);
+ atomic_dec(&ncm->notify_count);
break;
case -ECONNRESET:
case -ESHUTDOWN:
+ atomic_set(&ncm->notify_count, 0);
ncm->notify_state = NCM_NOTIFY_NONE;
break;
default:
DBG(cdev, "event %02x --> %d\n",
event->bNotificationType, req->status);
+ atomic_dec(&ncm->notify_count);
break;
}
- ncm->notify_req = req;
ncm_do_notify(ncm);
spin_unlock(&ncm->lock);
}
@@ -1649,6 +1653,11 @@ static void ncm_unbind(struct usb_configuration *c, struct usb_function *f)
ncm_string_defs[0].id = 0;
usb_free_all_descriptors(f);
+ if (atomic_read(&ncm->notify_count)) {
+ usb_ep_dequeue(ncm->notify, ncm->notify_req);
+ atomic_set(&ncm->notify_count, 0);
+ }
+
kfree(ncm->notify_req->buf);
usb_ep_free_request(ncm->notify, ncm->notify_req);
}
--
2.24.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH 2/2] usb: gadget: f_ecm: Use atomic_t to track in-flight request
2020-01-09 13:17 [PATCH 0/2] Fix two NULL pointer dereferences when role-switching Bryan O'Donoghue
2020-01-09 13:17 ` [PATCH 1/2] usb: gadget: f_ncm: Use atomic_t to track in-flight request Bryan O'Donoghue
@ 2020-01-09 13:17 ` Bryan O'Donoghue
1 sibling, 0 replies; 3+ messages in thread
From: Bryan O'Donoghue @ 2020-01-09 13:17 UTC (permalink / raw)
To: balbi, gregkh, linux-usb, linux-kernel; +Cc: Bryan O'Donoghue
Currently ecm->notify_req is used to flag when a request is in-flight.
ecm->notify_req is set to NULL and when a request completes it is
subsequently reset.
This is fundamentally buggy in that the unbind logic of the ECM driver will
unconditionally free ecm->notify_req leading to a NULL pointer dereference.
Fixes: da741b8c56d61 ("usb ethernet gadget: split CDC Ethernet function")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
drivers/usb/gadget/function/f_ecm.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
index 460d5d7c984f..7f5cf488b2b1 100644
--- a/drivers/usb/gadget/function/f_ecm.c
+++ b/drivers/usb/gadget/function/f_ecm.c
@@ -52,6 +52,7 @@ struct f_ecm {
struct usb_ep *notify;
struct usb_request *notify_req;
u8 notify_state;
+ atomic_t notify_count;
bool is_open;
/* FIXME is_open needs some irq-ish locking
@@ -380,7 +381,7 @@ static void ecm_do_notify(struct f_ecm *ecm)
int status;
/* notification already in flight? */
- if (!req)
+ if (atomic_read(&ecm->notify_count))
return;
event = req->buf;
@@ -420,10 +421,10 @@ static void ecm_do_notify(struct f_ecm *ecm)
event->bmRequestType = 0xA1;
event->wIndex = cpu_to_le16(ecm->ctrl_id);
- ecm->notify_req = NULL;
+ atomic_inc(&ecm->notify_count);
status = usb_ep_queue(ecm->notify, req, GFP_ATOMIC);
if (status < 0) {
- ecm->notify_req = req;
+ atomic_dec(&ecm->notify_count);
DBG(cdev, "notify --> %d\n", status);
}
}
@@ -448,17 +449,19 @@ static void ecm_notify_complete(struct usb_ep *ep, struct usb_request *req)
switch (req->status) {
case 0:
/* no fault */
+ atomic_dec(&ecm->notify_count);
break;
case -ECONNRESET:
case -ESHUTDOWN:
+ atomic_set(&ecm->notify_count, 0);
ecm->notify_state = ECM_NOTIFY_NONE;
break;
default:
DBG(cdev, "event %02x --> %d\n",
event->bNotificationType, req->status);
+ atomic_dec(&ecm->notify_count);
break;
}
- ecm->notify_req = req;
ecm_do_notify(ecm);
}
@@ -907,6 +910,11 @@ static void ecm_unbind(struct usb_configuration *c, struct usb_function *f)
usb_free_all_descriptors(f);
+ if (atomic_read(&ecm->notify_count)) {
+ usb_ep_dequeue(ecm->notify, ecm->notify_req);
+ atomic_set(&ecm->notify_count, 0);
+ }
+
kfree(ecm->notify_req->buf);
usb_ep_free_request(ecm->notify, ecm->notify_req);
}
--
2.24.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-01-09 13:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-09 13:17 [PATCH 0/2] Fix two NULL pointer dereferences when role-switching Bryan O'Donoghue
2020-01-09 13:17 ` [PATCH 1/2] usb: gadget: f_ncm: Use atomic_t to track in-flight request Bryan O'Donoghue
2020-01-09 13:17 ` [PATCH 2/2] usb: gadget: f_ecm: " Bryan O'Donoghue
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).