In gpioc_kqread(), kn->kn_data is computed via number_of_events(), which reads evidx_head, evidx_tail, and numevents without synchronization. For example: static size_t number_of_events(struct gpioc_cdevpriv *priv) { if (priv->evidx_head >= priv->evidx_tail) return (priv->evidx_head - priv->evidx_tail); else return (priv->numevents + priv->evidx_head - priv->evidx_tail); } Because head/tail may change between the check and the use, the “head >= tail” test can fail, and the subtraction may overflow. Impact • Undefined behavior: signed overflow. • Wrong interface semantics: EVFILT_READ kn_data may become a very large value, leading to bogus copyout values and faulty user decisions (e.g., self-DoS). Suggested fix Snapshot head, tail, and numevents once into local variables and compute from that single snapshot, instead of repeatedly reading shared fields.
Updated description: In gpioc_kqread(), kn->kn_data is computed via number_of_events(), which reads evidx_head, evidx_tail, and numevents without synchronization. For example: static size_t number_of_events(struct gpioc_cdevpriv *priv) { if (priv->evidx_head >= priv->evidx_tail) return (priv->evidx_head - priv->evidx_tail); else return (priv->numevents + priv->evidx_head - priv->evidx_tail); } Because head/tail may change between the check and the use, the “head >= tail” test can fail, and the subtraction may be negative, when converted to an unsigned integer, it wraps to a very large value. Impact • Integer overflow • Wrong interface semantics: EVFILT_READ kn_data may become a very large value, leading to bogus copyout values and faulty user decisions (e.g., self-DoS). Suggested fix Snapshot head, tail, and numevents once into local variables and compute from that single snapshot, instead of repeatedly reading shared fields.
Ahmad, would you be able to take a look at this?
I can. Thank you for bringing it to my attention. Qiu-ji: kqueue(9) should be locking priv->mtx before calling gpioc_kqread, so this shouldn't be a problem. Did you run into such a race? If so, do you have a way of reproducing it?
(In reply to Ahmad Khalifa from comment #3) Hi Ahmad, Thanks for looking into this. I believe there might be a misunderstanding regarding the lock. The priv->mtx lock is part of the gpioc_cdevpriv struct, which is defined within /source/sys/dev/gpio/gpioc.c. The generic kqueue framework in /sys/kern/kern_event.c calls gpioc_kqread() via a function pointer but has no knowledge of this driver-private structure or its lock, and therefore cannot hold it. This creates a potential race condition, as the unprotected reads can run concurrently with functions that modify the event queue, such as gpioc_read() and gpioc_interrupt_handler(). We found this issue using a custom static analysis tool that has identified numerous confirmed bugs in both Linux and FreeBSD. While we have built PoCs for many of the Linux bugs, we are still getting familiar with the FreeBSD environment but will attempt to create a PoC for this one. Thanks, Qiu-ji Chen
(In reply to Ahmad Khalifa from comment #3) Hi Ahmad, While re-auditing /sys/dev/gpio/gpioc.c, we noticed another potential issue in gpioc_ioctl() under the GPIOCONFIGEVENTS case. There appears to be a Time-of-Check to Time-of-Use (TOCTOU) race condition. The code relies on the !SLIST_EMPTY(&priv->pins) check to prevent races. The assumption seems to be that if the pin list is empty, there can be no concurrent access, since gpioc_read() would return ENXIO and gpioc_interrupt_handler() cannot be invoked if no pins are configured for interrupts. However, since the ioctl interface allows for concurrency, one thread could pass the !SLIST_EMPTY check, while another concurrent ioctl call using GPIOSETCONFIG enables an interrupt pin. This would create a race between the first thread and a now-possible gpioc_interrupt_handler() or gpioc_read(), potentially leading to a Use-After-Free. We suggest protecting the critical section in this ioctl case by holding the priv->mtx lock. Best regards, Qiu-ji Chen
The priv->mtx lock is given to kqueue through knlist_init_mtx (which is called in gpioc_open). It gets locked by knlist_mtx_lock (which is a generic implementation, since we never give one to kqueue) before it calls the knote's f_event (which in our case is gpioc_kqread). As for the ioctl race, yes, that does look like an oversight. I will try to get that fixed ASAP. Thank you for the report. :)
(In reply to Ahmad Khalifa from comment #6) Hi Ahmad, Thank you for confirming the ioctl race and for the detailed explanation of the kqueue locking mechanism. You are absolutely right about knlist_init_mtx; our static analysis and manual audit completely missed that initialization path. We appreciate the clarification and will work on improving our tool to correctly handle this pattern. Based on your explanation, the expectation is that filterops->f_event will always be called with the knlist lock held. With this understanding, we reviewed the call sites in /sys/kern/kern_event.c. We found that while 5 of the 6 calls to f_event do hold the lock, one in knote_fork() appears to be missing it. In that function, the knlist lock is released at line 573 but not re-acquired until line 610, leaving the call to kn->kn_fop->f_event(kn, NOTE_FORK) at line 608 unprotected. Perhaps the lock acquisition at line 610 should be moved before this call? On a related note, the comment for struct knote in /sys/sys/event.h mentions that fields like kn_sfflags are protected by the knlist lock, but it doesn't state that the kn_fop callbacks are also expected to be called under the lock. It might be helpful to update this comment to make the locking requirement explicit. So far, I have only audited the call sites for f_event, but it seems possible that other function pointers in filterops might also have call sites that violate this locking assumption. I will continue to review them, but wanted to bring this potential pattern to your attention. In Linux, a tool like lockdep is very effective for catching these locking violations. I am not familiar with the equivalent in FreeBSD, but perhaps adding assertions could help verify the rules. Thanks again for your time and help. Best regards, Qiu-ji Chen
(In reply to Qiu-ji Chen from comment #7) Hmm, that does seem odd at first glance, especially since the man page implies that only f_detach is called without the lock held. Mark: Is that intentional? Thanks for your in depth review of this.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=d000adfe41e6f2fe8f3dbe92d8fc2d34ae882086 commit d000adfe41e6f2fe8f3dbe92d8fc2d34ae882086 Author: Ahmad Khalifa <vexeduxr@FreeBSD.org> AuthorDate: 2025-09-30 11:09:50 +0000 Commit: Ahmad Khalifa <vexeduxr@FreeBSD.org> CommitDate: 2025-09-30 11:20:25 +0000 gpioc: fix race in ioctl(GPIOCONFIGEVENTS) A race can occur in gpioc_ioctl when it is called with GPIOCONFIGEVENTS closely followed by GPIOSETCONFIG. GPIOSETCONFIG can alter the priv->pins list, making it no longer empty and opening the door for access to priv->events while we are reallocating it. Fix this by holding priv->mtx while handling GPIOCONFIGEVENTS. Reported by: Qiu-ji Chen PR: 289120 Reviewed by: mmel MFC after: 1 day Differential Revision: https://reviews.freebsd.org/D52783 sys/dev/gpio/gpioc.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-)
(In reply to Ahmad Khalifa from comment #8) Hi Ahmad, Thanks for the follow-up. Two things further suggest this is an oversight. First, an earlier f_event call in the same function (around line 559) correctly holds the lock, which confirms the intended pattern. Second, the locking sequence around line 609 seems awkward and inefficient. After the unlocked f_event call, KNOTE_ACTIVATE(kn, 0) briefly locks and unlocks kq_lock, only for the code to re-acquire both the knlist lock and kq_lock immediately after on lines 610-611. This reinforces the idea that a cleaner fix would be to acquire both locks before the f_event call at line 608. This would not only fix the race but also allow changing the macro to KNOTE_ACTIVATE(kn, 1), correcting the inefficient locking sequence. Best regards, Qiu-ji Chen
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=4472ecb362b068400f26abfce2db2a2b10a88d95 commit 4472ecb362b068400f26abfce2db2a2b10a88d95 Author: Ahmad Khalifa <vexeduxr@FreeBSD.org> AuthorDate: 2025-09-30 11:09:50 +0000 Commit: Ahmad Khalifa <vexeduxr@FreeBSD.org> CommitDate: 2025-10-01 08:52:21 +0000 gpioc: fix race in ioctl(GPIOCONFIGEVENTS) A race can occur in gpioc_ioctl when it is called with GPIOCONFIGEVENTS closely followed by GPIOSETCONFIG. GPIOSETCONFIG can alter the priv->pins list, making it no longer empty and opening the door for access to priv->events while we are reallocating it. Fix this by holding priv->mtx while handling GPIOCONFIGEVENTS. Reported by: Qiu-ji Chen PR: 289120 Reviewed by: mmel MFC after: 1 day Differential Revision: https://reviews.freebsd.org/D52783 (cherry picked from commit d000adfe41e6f2fe8f3dbe92d8fc2d34ae882086) sys/dev/gpio/gpioc.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=6163ced966b4e37a987e514aa2d2a61781c98fa5 commit 6163ced966b4e37a987e514aa2d2a61781c98fa5 Author: Ahmad Khalifa <vexeduxr@FreeBSD.org> AuthorDate: 2025-09-30 11:09:50 +0000 Commit: Ahmad Khalifa <vexeduxr@FreeBSD.org> CommitDate: 2025-10-01 09:08:55 +0000 gpioc: fix race in ioctl(GPIOCONFIGEVENTS) A race can occur in gpioc_ioctl when it is called with GPIOCONFIGEVENTS closely followed by GPIOSETCONFIG. GPIOSETCONFIG can alter the priv->pins list, making it no longer empty and opening the door for access to priv->events while we are reallocating it. Fix this by holding priv->mtx while handling GPIOCONFIGEVENTS. Reported by: Qiu-ji Chen PR: 289120 Reviewed by: mmel MFC after: 1 day Differential Revision: https://reviews.freebsd.org/D52783 (cherry picked from commit d000adfe41e6f2fe8f3dbe92d8fc2d34ae882086) sys/dev/gpio/gpioc.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-)
A commit in branch stable/15 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=8fef0ee894979cde55a5d2b7571ff71c393fda7d commit 8fef0ee894979cde55a5d2b7571ff71c393fda7d Author: Ahmad Khalifa <vexeduxr@FreeBSD.org> AuthorDate: 2025-09-30 11:09:50 +0000 Commit: Ahmad Khalifa <vexeduxr@FreeBSD.org> CommitDate: 2025-10-01 09:37:36 +0000 gpioc: fix race in ioctl(GPIOCONFIGEVENTS) A race can occur in gpioc_ioctl when it is called with GPIOCONFIGEVENTS closely followed by GPIOSETCONFIG. GPIOSETCONFIG can alter the priv->pins list, making it no longer empty and opening the door for access to priv->events while we are reallocating it. Fix this by holding priv->mtx while handling GPIOCONFIGEVENTS. Reported by: Qiu-ji Chen PR: 289120 Reviewed by: mmel MFC after: 1 day Differential Revision: https://reviews.freebsd.org/D52783 (cherry picked from commit d000adfe41e6f2fe8f3dbe92d8fc2d34ae882086) sys/dev/gpio/gpioc.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-)