Date: Tue, 30 May 2017 20:27:13 +0000 From: bugzilla-noreply@freebsd.org To: freebsd-bugs@FreeBSD.org Subject: [Bug 219525] Multiple bugs in mpr ioctl handler Message-ID: <bug-219525-8-v7EyxmqF5f@https.bugs.freebsd.org/bugzilla/> In-Reply-To: <bug-219525-8@https.bugs.freebsd.org/bugzilla/> References: <bug-219525-8@https.bugs.freebsd.org/bugzilla/>
next in thread | previous in thread | raw e-mail | index | archive | help
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=219525 --- Comment #2 from CTurt <ecturt@gmail.com> --- Thanks for taking a look. To clarify the point you asked about: `recorded_events` is a fixed size, it looks like this in `struct mpr_softc`: mpr_event_entry_t recorded_events[MPR_EVENT_QUEUE_SIZE]; The kernel should copy out only `sizeof(sc->recored_events)` bytes at most from here, or else it will copy out of bounds memory to userland. Going back to the code: size = data->Size; if ((size >= sizeof(sc->recorded_events)) && (status == 0)) { mpr_unlock(sc); if (copyout((void *)sc->recorded_events, PTRIN(data->PtrEvents), size) != 0) status = EFAULT; mpr_lock(sc); } Notice that it passes `size` to `copyout`, which can be greater than `sizeof(sc->recorded_events)` (see the check). Instead, the `copyout` call should be passed `sizeof(sc->recorded_events)` so that it won't read more than the array contains. So, you are correct when you say that it "is just checking if the user supplied size is enough to hold sizeof(sc->recored_events)". But this check won't prevent copying out of bounds memory to userland. -- You are receiving this mail because: You are the assignee for the bug.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?bug-219525-8-v7EyxmqF5f>