Re: [PATCH v2] watch_queue: Fix note pool publication race in watch_queue_set_size()

[Bradley Morgan]

On 9 October 2026 23:47:15 BST, "Cen Zhang (Microsoft)"
<cenzhang@xxxxxxxxxxxxxxxxxxx> wrote:
>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 post_one_notification() reads them with
>plain loads under a different lock, so nothing orders the four stores
>against the poster's loads: 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 publishing nr_notes last with smp_store_release(), and by
>reading it once with smp_load_acquire() in post_one_notification() before
>the bitmap and the page array are touched.  A poster that observes a
>non-zero nr_notes then also observes the pool behind it; one that observes
>zero drops the notification as before.

Gnarly looking bug.

>
>Fixes: c73be61cede5 ("pipe: Add general notification queue support")
>Reported-by: Dae R. Jeong <threeearcat@xxxxxxxxx>
>Closes: https://lore.kernel.org/all/ZT-S8Q7tyutcvu_q@dragonet/
>Reported-by: AutonomousCodeSecurity@xxxxxxxxxxxxx
>Suggested-by: Dae R. Jeong <threeearcat@xxxxxxxxx>
>Assisted-by: LLM

Nit: Cc: All Applicable 
<stable@xxxxxxxxxxxxxxx>

>Signed-off-by: Cen Zhang (Microsoft) <cenzhang@xxxxxxxxxxxxxxxxxxx>
>---
>v2:
> - Use smp_store_release()/smp_load_acquire() instead of lock (Dae R. Jeong).
>v1:
> - https://lore.kernel.org/all/20261008224233.9675-1-cenzhang@xxxxxxxxxxxxxxxxxxx/
>
> kernel/watch_queue.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
>diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
>index 538520861e8b..932a88ac1f39 100644
>--- a/kernel/watch_queue.c
>+++ b/kernel/watch_queue.c
>@@ -101,7 +101,7 @@ static bool post_one_notification(struct watch_queue *wqueue,
> 	struct pipe_inode_info *pipe = wqueue->pipe;
> 	struct pipe_buffer *buf;
> 	struct page *page;
>-	unsigned int head, tail, note, offset, len;
>+	unsigned int head, tail, note, offset, len, nr_notes;
> 	bool done = false;
> 
> 	spin_lock_irq(&pipe->rd_wait.lock);
>@@ -111,8 +111,10 @@ static bool post_one_notification(struct watch_queue *wqueue,
> 	if (pipe_full(head, tail, pipe->ring_size))
> 		goto lost;
> 
>-	note = find_first_bit(wqueue->notes_bitmap, wqueue->nr_notes);
>-	if (note >= wqueue->nr_notes)
>+	/* Pairs with the smp_store_release() in watch_queue_set_size(). */
>+	nr_notes = smp_load_acquire(&wqueue->nr_notes);
>+	note = find_first_bit(wqueue->notes_bitmap, nr_notes);
>+	if (note >= nr_notes)
> 		goto lost;
> 
> 	page = wqueue->notes[note / WATCH_QUEUE_NOTES_PER_PAGE];
>@@ -297,7 +299,8 @@ long watch_queue_set_size(struct pipe_inode_info *pipe, unsigned int nr_notes)
> 	wqueue->notes = pages;
> 	wqueue->notes_bitmap = bitmap;
> 	wqueue->nr_pages = nr_pages;
>-	wqueue->nr_notes = nr_notes;
>+	/* Pairs with the smp_load_acquire() in post_one_notification(). */
>+	smp_store_release(&wqueue->nr_notes, nr_notes);

Looks okay to me cen, just the nit:

Reviewed-by: Bradley Morgan <brads@xxxxxxxxxxxxxx>


> 	return 0;
> 
> error_p:
>

--- Thanks!
"I'm not a very positive person" - Linus torvalds