From nobody Sat Apr 08 15:13:02 2023 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4PtzJG73Hwz44Xc8; Sat, 8 Apr 2023 15:13:02 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4PtzJG5fLSz3k14; Sat, 8 Apr 2023 15:13:02 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1680966782; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=+c+GWqzsQDoE/8TxTb5Qdi9J4z893D/H34hTIEL1zdk=; b=NezSZ5zrdsK+QZY6ssYu2JoRIFv/CgCZP8vVsLeO5Vj9fcYUuKsEPOQSAFW65q+gwiHeje F6RZSrHzZjqc1vef7xUHkMoCdR+TkRRNGl7FNZQvrneuV5KJ1cd0ELvQDB0PRZPF0lrhvH 7wAuiz3LftIeB92YyMuyfTeoQagSWQbe7cAmFNzqFdjD+GHrZPqQM/cGNlOAcvR0OzWcWh /ZHrxvSudGdi6/wP2A2rOnXPVSHgz2+EQ9T/B/wOZIO8v3WvudOXDxDcKP6Xwi4nk7y/Nc hl2r2DPOpDBAfHFRvXHtTycTCUdeyaqlKTeOvnLOK8nqbQ7gAhiZfGmNPcsBag== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1680966782; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=+c+GWqzsQDoE/8TxTb5Qdi9J4z893D/H34hTIEL1zdk=; b=Wy8SM8ZWmWnp/OhzUaGmizWzWx5ph2SR7ktUtidp7HK5IsnskAW1E7/FCY7WC5lnyodJ1E 6OZComGAZyNeQgO171fQ8RiUAkrAGnAmKlJk4r1E7Txunu4FoD39a0HFNIO2jxWwDWdncc sTGIA3P6qaR3yBvkHOz8vMIXbb+eqhsEYRIft2RszelcnDqNng8KgDwZppbThjuZUZCr4P wrHYTTtpTiobnR+cwIVLqILuu6TFG97142lGGNhAyiTRqPCr7QWiZmwc53in0Z7yYU4Qla GkYUQCeqDjT8Tvrb3Hx7seHPduldXw4r4WV5Qz0rggnr5ByJHLBWS9OD+ripdA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1680966782; a=rsa-sha256; cv=none; b=LbYw8HwFcDI7aQw4T6bt6PAV67VXciOScfPTZKt7qdMk7jTvjXfULkQx9lZrWMfGksUPre TFJe62ezWoFaxulQIZdZ8YKvk6G2RTPuZ96bnlql30mYuxhHFJ6JNZUmnrFsotHS8jdouI JEQrfjo+CioyEizeWAx614A57HM2oZdi0BleqtEZarQIbbiEuhoujXMfon1B+CHl/S0oWn BAHB8BIWDV9I4SBLrcrPAf1N8cBEhD1SqzBeheajyAocVB5afUIgRZzF9FCynjyT4H7m3Y V6ys5YHtKz3COp6OplDBeWBFxKAr4jx+dw704MPId2/q6nsLmaaqJX0Vsd39fQ== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4PtzJG4mP4zWxX; Sat, 8 Apr 2023 15:13:02 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 338FD2PM070745; Sat, 8 Apr 2023 15:13:02 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 338FD2M5070744; Sat, 8 Apr 2023 15:13:02 GMT (envelope-from git) Date: Sat, 8 Apr 2023 15:13:02 GMT Message-Id: <202304081513.338FD2M5070744@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Hans Petter Selasky Subject: git: 9b077d72bcc3 - main - usb(4): Separate the fast path and the slow path to avoid races and use-after-free for the USB FS interface. List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: hselasky X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 9b077d72bcc313baea2b9283afc7f568739eaadc Auto-Submitted: auto-generated X-ThisMailContainsUnwantedMimeParts: N The branch main has been updated by hselasky: URL: https://cgit.FreeBSD.org/src/commit/?id=9b077d72bcc313baea2b9283afc7f568739eaadc commit 9b077d72bcc313baea2b9283afc7f568739eaadc Author: Hans Petter Selasky AuthorDate: 2023-03-31 17:14:18 +0000 Commit: Hans Petter Selasky CommitDate: 2023-04-08 15:11:31 +0000 usb(4): Separate the fast path and the slow path to avoid races and use-after-free for the USB FS interface. Bad behaving user-space USB applicatoins may crash the kernel by issuing USB FS related ioctl(2)'s out of their expected order. By default the USB FS ioctl(2) interface is only available to the administrator, root, and driver applications like webcamd(8) needs to be hijacked in order for this to happen. The issue is the fast-path code does not always see updates made by the slow-path code, and may then work on freed memory. This is easily fixed by using an EPOCH(9) type of synchronization mechanism. A SX(9) lock will be used as a substitute for EPOCH(9), due to the need for sleepability. In addition most calls going into the fast-path originate from a single user-space process and the need for multi-thread performance is not present. Differential Revision: https://reviews.freebsd.org/D39373 Reviewed by: markj@ Reported by: C Turt admbugs: 994 MFC after: 1 week Sponsored by: NVIDIA Networking --- sys/dev/usb/usb_dev.c | 4 +- sys/dev/usb/usb_dev.h | 6 ++- sys/dev/usb/usb_device.c | 6 +-- sys/dev/usb/usb_generic.c | 105 ++++++++++++++++++++++++++++++---------------- 4 files changed, 80 insertions(+), 41 deletions(-) diff --git a/sys/dev/usb/usb_dev.c b/sys/dev/usb/usb_dev.c index 84446edc3ecd..5f06b1b9690e 100644 --- a/sys/dev/usb/usb_dev.c +++ b/sys/dev/usb/usb_dev.c @@ -2,7 +2,7 @@ /*- * SPDX-License-Identifier: BSD-2-Clause-FreeBSD * - * Copyright (c) 2006-2008 Hans Petter Selasky. All rights reserved. + * Copyright (c) 2006-2023 Hans Petter Selasky * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -386,6 +386,7 @@ usb_fifo_alloc(struct mtx *mtx) f = malloc(sizeof(*f), M_USBDEV, M_WAITOK | M_ZERO); cv_init(&f->cv_io, "FIFO-IO"); cv_init(&f->cv_drain, "FIFO-DRAIN"); + sx_init(&f->fs_fastpath_lock, "FIFO-FP"); f->priv_mtx = mtx; f->refcount = 1; knlist_init_mtx(&f->selinfo.si_note, mtx); @@ -626,6 +627,7 @@ usb_fifo_free(struct usb_fifo *f) cv_destroy(&f->cv_io); cv_destroy(&f->cv_drain); + sx_destroy(&f->fs_fastpath_lock); knlist_clear(&f->selinfo.si_note, 0); seldrain(&f->selinfo); diff --git a/sys/dev/usb/usb_dev.h b/sys/dev/usb/usb_dev.h index 1bdfa46d064f..8df3af9990cf 100644 --- a/sys/dev/usb/usb_dev.h +++ b/sys/dev/usb/usb_dev.h @@ -2,7 +2,7 @@ /*- * SPDX-License-Identifier: BSD-2-Clause-FreeBSD * - * Copyright (c) 2008 Hans Petter Selasky. All rights reserved. + * Copyright (c) 2008-2023 Hans Petter Selasky * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -110,13 +110,15 @@ struct usb_fifo { struct selinfo selinfo; struct cv cv_io; struct cv cv_drain; + struct sx fs_fastpath_lock; struct usb_fifo_methods *methods; struct usb_symlink *symlink[2];/* our symlinks */ struct proc *async_p; /* process that wants SIGIO */ struct usb_fs_endpoint *fs_ep_ptr; struct usb_device *udev; +#define USB_FS_XFER_MAX 126 struct usb_xfer *xfer[2]; - struct usb_xfer **fs_xfer; + struct usb_xfer *fs_xfer[USB_FS_XFER_MAX]; struct mtx *priv_mtx; /* client data */ /* set if FIFO is opened by a FILE: */ struct usb_cdev_privdata *curr_cpd; diff --git a/sys/dev/usb/usb_device.c b/sys/dev/usb/usb_device.c index 392d969587c0..820cb21c704d 100644 --- a/sys/dev/usb/usb_device.c +++ b/sys/dev/usb/usb_device.c @@ -2,7 +2,7 @@ /*- * SPDX-License-Identifier: BSD-2-Clause-FreeBSD * - * Copyright (c) 2008-2020 Hans Petter Selasky. All rights reserved. + * Copyright (c) 2008-2023 Hans Petter Selasky * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -2829,7 +2829,7 @@ usb_fifo_free_wrap(struct usb_device *udev, continue; } if ((f->dev_ep_index == 0) && - (f->fs_xfer == NULL)) { + (f->fs_ep_max == 0)) { /* no need to free this FIFO */ continue; } @@ -2837,7 +2837,7 @@ usb_fifo_free_wrap(struct usb_device *udev, if ((f->methods == &usb_ugen_methods) && (f->dev_ep_index == 0) && (!(flag & USB_UNCFG_FLAG_FREE_EP0)) && - (f->fs_xfer == NULL)) { + (f->fs_ep_max == 0)) { /* no need to free this FIFO */ continue; } diff --git a/sys/dev/usb/usb_generic.c b/sys/dev/usb/usb_generic.c index 87a1e792c609..682f9c28c956 100644 --- a/sys/dev/usb/usb_generic.c +++ b/sys/dev/usb/usb_generic.c @@ -2,7 +2,7 @@ /*- * SPDX-License-Identifier: BSD-2-Clause-FreeBSD * - * Copyright (c) 2008-2022 Hans Petter Selasky + * Copyright (c) 2008-2023 Hans Petter Selasky * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -968,10 +968,10 @@ ugen_fs_init(struct usb_fifo *f, int error; /* verify input parameters */ - if (fs_ep_ptr == NULL || ep_index_max > 127) + if (fs_ep_ptr == NULL || ep_index_max > USB_FS_XFER_MAX) return (EINVAL); - if (f->fs_xfer != NULL) + if (f->fs_ep_max != 0) return (EBUSY); if (f->dev_ep_index != 0 || ep_index_max == 0) @@ -982,11 +982,11 @@ ugen_fs_init(struct usb_fifo *f, error = usb_fifo_alloc_buffer(f, 1, ep_index_max); if (error == 0) { - f->fs_xfer = malloc(sizeof(f->fs_xfer[0]) * - ep_index_max, M_USB, M_WAITOK | M_ZERO); + mtx_lock(f->priv_mtx); f->fs_ep_max = ep_index_max; f->fs_ep_ptr = fs_ep_ptr; f->fs_ep_sz = fs_ep_sz; + mtx_unlock(f->priv_mtx); } return (error); } @@ -994,15 +994,26 @@ ugen_fs_init(struct usb_fifo *f, int ugen_fs_uninit(struct usb_fifo *f) { - if (f->fs_xfer == NULL) { + if (f->fs_ep_max == 0) return (EINVAL); - } - usbd_transfer_unsetup(f->fs_xfer, f->fs_ep_max); - free(f->fs_xfer, M_USB); - f->fs_xfer = NULL; + + /* + * Prevent calls into the fast-path code, by setting fs_ep_max + * to zero: + */ + sx_xlock(&f->fs_fastpath_lock); + mtx_lock(f->priv_mtx); f->fs_ep_max = 0; + mtx_unlock(f->priv_mtx); + sx_xunlock(&f->fs_fastpath_lock); + + usbd_transfer_unsetup(f->fs_xfer, USB_FS_XFER_MAX); + + mtx_lock(f->priv_mtx); f->fs_ep_ptr = NULL; f->flag_iscomplete = 0; + mtx_unlock(f->priv_mtx); + usb_fifo_free_buffer(f); return (0); } @@ -1109,10 +1120,26 @@ usb_fs_open(struct usb_fifo *f, struct usb_fs_open *popen, static int usb_fs_close(struct usb_fifo *f, struct usb_fs_close *pclose) { + struct usb_xfer *xfer; + if (pclose->ep_index >= f->fs_ep_max) return (EINVAL); - usbd_transfer_unsetup(f->fs_xfer + pclose->ep_index, 1); + /* + * Prevent calls into the fast-path code, by setting the + * fs_xfer[] in question to NULL: + */ + sx_xlock(&f->fs_fastpath_lock); + mtx_lock(f->priv_mtx); + xfer = f->fs_xfer[pclose->ep_index]; + f->fs_xfer[pclose->ep_index] = NULL; + mtx_unlock(f->priv_mtx); + sx_xunlock(&f->fs_fastpath_lock); + + if (xfer == NULL) + return (EINVAL); + + usbd_transfer_unsetup(&xfer, 1); return (0); } @@ -1249,14 +1276,16 @@ ugen_fs_copy_in(struct usb_fifo *f, uint8_t ep_index) int error; uint8_t isread; + mtx_lock(f->priv_mtx); if (ep_index >= f->fs_ep_max) { + mtx_unlock(f->priv_mtx); return (EINVAL); } xfer = f->fs_xfer[ep_index]; if (xfer == NULL) { + mtx_unlock(f->priv_mtx); return (EINVAL); } - mtx_lock(f->priv_mtx); if (usbd_transfer_pending(xfer)) { mtx_unlock(f->priv_mtx); return (EBUSY); /* should not happen */ @@ -1529,14 +1558,16 @@ ugen_fs_copy_out(struct usb_fifo *f, uint8_t ep_index) int error; uint8_t isread; - if (ep_index >= f->fs_ep_max) + mtx_lock(f->priv_mtx); + if (ep_index >= f->fs_ep_max) { + mtx_unlock(f->priv_mtx); return (EINVAL); - + } xfer = f->fs_xfer[ep_index]; - if (xfer == NULL) + if (xfer == NULL) { + mtx_unlock(f->priv_mtx); return (EINVAL); - - mtx_lock(f->priv_mtx); + } if (!xfer->flags_int.transferring && !xfer->flags_int.started) { mtx_unlock(f->priv_mtx); @@ -1675,10 +1706,6 @@ ugen_ioctl(struct usb_fifo *f, u_long cmd, void *addr, int fflags) struct usb_fs_complete *pcomp; struct usb_fs_start *pstart; struct usb_fs_stop *pstop; - struct usb_fs_open *popen; - struct usb_fs_open_stream *popen_stream; - struct usb_fs_close *pclose; - struct usb_fs_clear_stall_sync *pstall; void *addr; } u; struct usb_xfer *xfer; @@ -1691,6 +1718,7 @@ ugen_ioctl(struct usb_fifo *f, u_long cmd, void *addr, int fflags) switch (cmd) { case USB_FS_COMPLETE: + sx_slock(&f->fs_fastpath_lock); mtx_lock(f->priv_mtx); error = ugen_fs_get_complete(f, &ep_index); mtx_unlock(f->priv_mtx); @@ -1701,9 +1729,11 @@ ugen_ioctl(struct usb_fifo *f, u_long cmd, void *addr, int fflags) u.pcomp->ep_index = ep_index; error = ugen_fs_copy_out(f, u.pcomp->ep_index); } + sx_sunlock(&f->fs_fastpath_lock); break; case USB_FS_START: + sx_slock(&f->fs_fastpath_lock); error = ugen_fs_copy_in(f, u.pstart->ep_index); if (error == 0) { mtx_lock(f->priv_mtx); @@ -1711,6 +1741,7 @@ ugen_ioctl(struct usb_fifo *f, u_long cmd, void *addr, int fflags) usbd_transfer_start(xfer); mtx_unlock(f->priv_mtx); } + sx_sunlock(&f->fs_fastpath_lock); break; case USB_FS_STOP: @@ -1739,20 +1770,6 @@ ugen_ioctl(struct usb_fifo *f, u_long cmd, void *addr, int fflags) mtx_unlock(f->priv_mtx); break; - case USB_FS_OPEN: - case USB_FS_OPEN_STREAM: - error = usb_fs_open(f, u.popen, fflags, - (cmd == USB_FS_OPEN_STREAM) ? u.popen_stream->stream_id : 0); - break; - - case USB_FS_CLOSE: - error = usb_fs_close(f, u.pclose); - break; - - case USB_FS_CLEAR_STALL_SYNC: - error = usb_fs_clear_stall_sync(f, u.pstall); - break; - default: error = ENOIOCTL; break; @@ -2212,6 +2229,10 @@ ugen_ioctl_post(struct usb_fifo *f, u_long cmd, void *addr, int fflags) struct usb_fs_init32 *pinit32; #endif struct usb_fs_uninit *puninit; + struct usb_fs_open *popen; + struct usb_fs_open_stream *popen_stream; + struct usb_fs_close *pclose; + struct usb_fs_clear_stall_sync *pstall; struct usb_device_port_path *dpp; uint32_t *ptime; void *addr; @@ -2440,6 +2461,20 @@ ugen_ioctl_post(struct usb_fifo *f, u_long cmd, void *addr, int fflags) error = ugen_fs_uninit(f); break; + case USB_FS_OPEN: + case USB_FS_OPEN_STREAM: + error = usb_fs_open(f, u.popen, fflags, + (cmd == USB_FS_OPEN_STREAM) ? u.popen_stream->stream_id : 0); + break; + + case USB_FS_CLOSE: + error = usb_fs_close(f, u.pclose); + break; + + case USB_FS_CLEAR_STALL_SYNC: + error = usb_fs_clear_stall_sync(f, u.pstall); + break; + default: mtx_lock(f->priv_mtx); error = ugen_iface_ioctl(f, cmd, addr, fflags);