watch_queue: Fix missing locking in watch_queue_set_size()
PATCH 1/1 watch_queue: Fix missing locking in watch_queue_set_size()
There is a race between watch_queue_set_size() and post_one_notification(). watch_queue_set_size() publishes wqueue->notes, notes_bitmap, nr_pages and nr_notes with plain stores and does not take wqueue->lock, so the four stores are not atomic as a group and, depending on the build, notes can be stored last (gcc 14 on x86-64 does this). Since keyctl_watch_key() lets a watch attach to a queue before it is sized, post_one_notification() can already be running on another CPU at that moment: it reads nr_notes > 0, passes its bounds check, and then reads notes while it is still NULL, dereferencing notes[0].
An unprivileged user triggers this reliably by watching one of its own keys through a notification pipe and racing ioctl(IOC_WATCH_QUEUE_SET_SIZE) on that pipe against keyctl(KEYCTL_SETPERM) on the key from another CPU, triggering:
BUG: kernel NULL pointer dereference, address: 0000000000000000 RIP: 0010:post_one_notification.isra.0+0xa2/0x1e0 Call Trace: __post_watch_notification+0x148/0x180 keyctl_setperm_key+0x119/0x130 do_syscall_64+0x108/0x4c0 entry_SYSCALL_64_after_hwframe+0x77/0x7f Kernel panic - not syncing: Fatal exception in interrupt
Fix this by putting the four publication stores under wqueue->lock. post_one_notification() already runs with that lock held, so a poster now sees either the whole pool or none of it.
--- kernel/watch_queue.c | 3 +++ 1 file changed, 3 insertions(+)
kernel/watch_queue.c +3 -0
@@ -294,10 +294,13 @@ long watch_queue_set_size(struct pipe_inode_info *pipe, unsigned int nr_notes) goto error_p;bitmap_fill (bitmap, nr_notes); +/* Use lock since watches may already be attached. */ +spin_lock_bh (&wqueue->lock); wqueue->notes = pages; wqueue->notes_bitmap = bitmap; wqueue->nr_pages = nr_pages; wqueue->nr_notes = nr_notes; +spin_unlock_bh (&wqueue->lock);return 0 ; error_p:
Signature · 1 line
2.55.0
kernel/watch_queue.c long watch_queue_set_size(struct pipe_inode_info *pipe, unsigned int nr_notes) · line 303
Quoted 7 lines
kernel/watch_queue.c +3
-0+/* Use lock since watches may already be attached. */ +spin_lock_bh (&wqueue->lock); wqueue->notes = pages; wqueue->notes_bitmap = bitmap; wqueue->nr_pages = nr_pages; wqueue->nr_notes = nr_notes; +spin_unlock_bh (&wqueue->lock);
I'll send a v2 tomorrow, replacing the lock with smp_store_release()/ smp_load_acquire() for wqueue->nr_notes.
Greg KH
maintainerQuoted 34 lines · Cen Zhang (Microsoft)
There is a race between
watch_queue_set_size()andpost_one_notification().watch_queue_set_size()publisheswqueue->notes,notes_bitmap,nr_pagesandnr_noteswith plain stores and does not takewqueue->lock, so the four stores are not atomic as a group and, depending on the build, notes can be stored last (gcc 14 on x86-64 does this). Sincekeyctl_watch_key()lets a watch attach to a queue before it is sized,post_one_notification()can already be running on another CPU at that moment: it readsnr_notes> 0, passes its bounds check, and then reads notes while it is still NULL, dereferencing notes[0].An unprivileged user triggers this reliably by watching one of its own keys through a notification pipe and racing
ioctl(IOC_WATCH_QUEUE_SET_SIZE)on that pipe againstkeyctl(KEYCTL_SETPERM)on the key from another CPU, triggering:BUG: kernel NULL pointer dereference, address: 0000000000000000 RIP: 0010:post_one_notification.isra.0+0xa2/0x1e0 Call Trace: __post_watch_notification+0x148/0x180 keyctl_setperm_key+0x119/0x130 do_syscall_64+0x108/0x4c0 entry_SYSCALL_64_after_hwframe+0x77/0x7f Kernel panic - not syncing: Fatal exception in interruptFix this by putting the four publication stores under
wqueue->lock.post_one_notification()already runs with that lock held, so a poster now sees either the whole pool or none of it.Fixes c73be61cede5 ("pipe: Add general notification queue support")Reported-by Dae R. Jeong <[email protected]>Reported-by [email protected]Assisted-by LLMSigned-off-by Cen Zhang (Microsoft) <[email protected]>
Quoted 3 lines
--- kernel/watch_queue.c | 3 +++ 1 file changed, 3 insertions(+)
No need to cc: [email protected] for public patches like this, thanks.
greg k-h