Date: Sun, 30 Apr 2023 06:57:57 GMT From: Hans Petter Selasky <hselasky@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: 53e253c7e57f - stable/13 - usb(4): Separate the fast path and the slow path to avoid races and use-after-free for the USB FS interface. Message-ID: <202304300657.33U6vval071618@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch stable/13 has been updated by hselasky: URL: https://cgit.FreeBSD.org/src/commit/?id=53e253c7e57f4a0a0632fca933277e509e04543c commit 53e253c7e57f4a0a0632fca933277e509e04543c Author: Hans Petter Selasky <hselasky@FreeBSD.org> AuthorDate: 2023-03-31 17:14:18 +0000 Commit: Hans Petter Selasky <hselasky@FreeBSD.org> CommitDate: 2023-04-30 06:56:17 +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 <ecturt@gmail.com> admbugs: 994 Sponsored by: NVIDIA Networking (cherry picked from commit 9b077d72bcc313baea2b9283afc7f568739eaadc) --- 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 ad2f23eb9bfc..640f9fdb7ad6 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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202304300657.33U6vval071618>