* [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-07-28 15:51 ` Siddh Raman Pant via Linux-kernel-mentees 0 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant @ 2022-07-28 15:51 UTC (permalink / raw) To: David Howells, Christophe JAILLET, Eric Dumazet, Fabio M. De Francesco Cc: linux-security-modules, linux-kernel, linux-kernel-mentees, syzbot+c70d87ac1d001f29a058 If not done, a reference to a freed pipe remains in the watch_queue, as this function is called before freeing a pipe in free_pipe_info() (see line 834 of fs/pipe.c). This causes a UAF when post_one_notification() tries to access the pipe on a key update, which is reported by syzbot. We also need to use READ_ONCE() in post_one_notification() to prevent the compiler from optimising and loading a non-NULL value from wqueue->pipe. Bug report: https://syzkaller.appspot.com/bug?id=1870dd7791ba05f2ea7f47f7cbdde701173973fc Reported-and-tested-by: syzbot+c70d87ac1d001f29a058@syzkaller.appspotmail.com Signed-off-by: Siddh Raman Pant <code@siddh.me> --- Changes in v3: - Restore the original unlock order, and clear before unlock. - Use READ_ONCE() in post path. This was explained by David Howells <dhowells@redhat.com> in reply to v1. Not added Suggested-by since he didn't reply yet. Changes in v2: - Removed the superfluous ifdef guard. kernel/watch_queue.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c index bb9962b33f95..617425e34252 100644 --- a/kernel/watch_queue.c +++ b/kernel/watch_queue.c @@ -99,7 +99,7 @@ static bool post_one_notification(struct watch_queue *wqueue, struct watch_notification *n) { void *p; - struct pipe_inode_info *pipe = wqueue->pipe; + struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe); struct pipe_buffer *buf; struct page *page; unsigned int head, tail, mask, note, offset, len; @@ -637,6 +637,12 @@ void watch_queue_clear(struct watch_queue *wqueue) spin_lock_bh(&wqueue->lock); } + /* Clearing the watch queue, so we should clean the associated pipe. */ + if (wqueue->pipe) { + wqueue->pipe->watch_queue = NULL; + wqueue->pipe = NULL; + } + spin_unlock_bh(&wqueue->lock); rcu_read_unlock(); } -- 2.35.1 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-07-28 15:51 ` Siddh Raman Pant via Linux-kernel-mentees 0 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-07-28 15:51 UTC (permalink / raw) To: David Howells, Christophe JAILLET, Eric Dumazet, Fabio M. De Francesco Cc: linux-security-modules, linux-kernel-mentees, linux-kernel, syzbot+c70d87ac1d001f29a058 If not done, a reference to a freed pipe remains in the watch_queue, as this function is called before freeing a pipe in free_pipe_info() (see line 834 of fs/pipe.c). This causes a UAF when post_one_notification() tries to access the pipe on a key update, which is reported by syzbot. We also need to use READ_ONCE() in post_one_notification() to prevent the compiler from optimising and loading a non-NULL value from wqueue->pipe. Bug report: https://syzkaller.appspot.com/bug?id=1870dd7791ba05f2ea7f47f7cbdde701173973fc Reported-and-tested-by: syzbot+c70d87ac1d001f29a058@syzkaller.appspotmail.com Signed-off-by: Siddh Raman Pant <code@siddh.me> --- Changes in v3: - Restore the original unlock order, and clear before unlock. - Use READ_ONCE() in post path. This was explained by David Howells <dhowells@redhat.com> in reply to v1. Not added Suggested-by since he didn't reply yet. Changes in v2: - Removed the superfluous ifdef guard. kernel/watch_queue.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c index bb9962b33f95..617425e34252 100644 --- a/kernel/watch_queue.c +++ b/kernel/watch_queue.c @@ -99,7 +99,7 @@ static bool post_one_notification(struct watch_queue *wqueue, struct watch_notification *n) { void *p; - struct pipe_inode_info *pipe = wqueue->pipe; + struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe); struct pipe_buffer *buf; struct page *page; unsigned int head, tail, mask, note, offset, len; @@ -637,6 +637,12 @@ void watch_queue_clear(struct watch_queue *wqueue) spin_lock_bh(&wqueue->lock); } + /* Clearing the watch queue, so we should clean the associated pipe. */ + if (wqueue->pipe) { + wqueue->pipe->watch_queue = NULL; + wqueue->pipe = NULL; + } + spin_unlock_bh(&wqueue->lock); rcu_read_unlock(); } -- 2.35.1 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-07-28 15:51 ` Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-01 9:34 ` Siddh Raman Pant via Linux-kernel-mentees -1 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant @ 2022-08-01 9:34 UTC (permalink / raw) To: Dipanjan Das Cc: David Howells, Christophe JAILLET, Eric Dumazet, Fabio M. De Francesco, linux-security-modules, linux-kernel, syzbot+c70d87ac1d001f29a058, linux-kernel-mentees Hello Dipanjan, It would be nice if you could test this patch and tell if it fixes the issue on v5.10, as you had reported it earlier. Please apply the following commits before applying this patch: db8facfc9faf ("watch_queue, pipe: Free watchqueue state after clearing pipe ring") 353f7988dd84 ("watchqueue: make sure to serialize 'wqueue->defunct' properly") I have tested locally on tag v5.10, using the reproducer available on syzkaller dashboard. The crash occurred when the patches weren't applied, and it no longer occurs after applying the three patches. Thanks, Siddh > If not done, a reference to a freed pipe remains in the watch_queue, > as this function is called before freeing a pipe in free_pipe_info() > (see line 834 of fs/pipe.c). > > This causes a UAF when post_one_notification() tries to access the pipe > on a key update, which is reported by syzbot. > > We also need to use READ_ONCE() in post_one_notification() to prevent the > compiler from optimising and loading a non-NULL value from wqueue->pipe. > > Bug report: https://syzkaller.appspot.com/bug?id=1870dd7791ba05f2ea7f47f7cbdde701173973fc > Reported-and-tested-by: syzbot+c70d87ac1d001f29a058@syzkaller.appspotmail.com > > Signed-off-by: Siddh Raman Pant <code@siddh.me> > --- > Changes in v3: > - Restore the original unlock order, and clear before unlock. > - Use READ_ONCE() in post path. > > This was explained by David Howells <dhowells@redhat.com> in > reply to v1. Not added Suggested-by since he didn't reply yet. > > Changes in v2: > - Removed the superfluous ifdef guard. > > kernel/watch_queue.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c > index bb9962b33f95..617425e34252 100644 > --- a/kernel/watch_queue.c > +++ b/kernel/watch_queue.c > @@ -99,7 +99,7 @@ static bool post_one_notification(struct watch_queue *wqueue, > struct watch_notification *n) > { > void *p; > - struct pipe_inode_info *pipe = wqueue->pipe; > + struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe); > struct pipe_buffer *buf; > struct page *page; > unsigned int head, tail, mask, note, offset, len; > @@ -637,6 +637,12 @@ void watch_queue_clear(struct watch_queue *wqueue) > spin_lock_bh(&wqueue->lock); > } > > + /* Clearing the watch queue, so we should clean the associated pipe. */ > + if (wqueue->pipe) { > + wqueue->pipe->watch_queue = NULL; > + wqueue->pipe = NULL; > + } > + > spin_unlock_bh(&wqueue->lock); > rcu_read_unlock(); > } > -- > 2.35.1 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-01 9:34 ` Siddh Raman Pant via Linux-kernel-mentees 0 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-01 9:34 UTC (permalink / raw) To: Dipanjan Das Cc: syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco Hello Dipanjan, It would be nice if you could test this patch and tell if it fixes the issue on v5.10, as you had reported it earlier. Please apply the following commits before applying this patch: db8facfc9faf ("watch_queue, pipe: Free watchqueue state after clearing pipe ring") 353f7988dd84 ("watchqueue: make sure to serialize 'wqueue->defunct' properly") I have tested locally on tag v5.10, using the reproducer available on syzkaller dashboard. The crash occurred when the patches weren't applied, and it no longer occurs after applying the three patches. Thanks, Siddh > If not done, a reference to a freed pipe remains in the watch_queue, > as this function is called before freeing a pipe in free_pipe_info() > (see line 834 of fs/pipe.c). > > This causes a UAF when post_one_notification() tries to access the pipe > on a key update, which is reported by syzbot. > > We also need to use READ_ONCE() in post_one_notification() to prevent the > compiler from optimising and loading a non-NULL value from wqueue->pipe. > > Bug report: https://syzkaller.appspot.com/bug?id=1870dd7791ba05f2ea7f47f7cbdde701173973fc > Reported-and-tested-by: syzbot+c70d87ac1d001f29a058@syzkaller.appspotmail.com > > Signed-off-by: Siddh Raman Pant <code@siddh.me> > --- > Changes in v3: > - Restore the original unlock order, and clear before unlock. > - Use READ_ONCE() in post path. > > This was explained by David Howells <dhowells@redhat.com> in > reply to v1. Not added Suggested-by since he didn't reply yet. > > Changes in v2: > - Removed the superfluous ifdef guard. > > kernel/watch_queue.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c > index bb9962b33f95..617425e34252 100644 > --- a/kernel/watch_queue.c > +++ b/kernel/watch_queue.c > @@ -99,7 +99,7 @@ static bool post_one_notification(struct watch_queue *wqueue, > struct watch_notification *n) > { > void *p; > - struct pipe_inode_info *pipe = wqueue->pipe; > + struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe); > struct pipe_buffer *buf; > struct page *page; > unsigned int head, tail, mask, note, offset, len; > @@ -637,6 +637,12 @@ void watch_queue_clear(struct watch_queue *wqueue) > spin_lock_bh(&wqueue->lock); > } > > + /* Clearing the watch queue, so we should clean the associated pipe. */ > + if (wqueue->pipe) { > + wqueue->pipe->watch_queue = NULL; > + wqueue->pipe = NULL; > + } > + > spin_unlock_bh(&wqueue->lock); > rcu_read_unlock(); > } > -- > 2.35.1 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-01 9:34 ` Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-01 10:24 ` Greg KH -1 siblings, 0 replies; 36+ messages in thread From: Greg KH @ 2022-08-01 10:24 UTC (permalink / raw) To: Siddh Raman Pant Cc: Dipanjan Das, syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco A: http://en.wikipedia.org/wiki/Top_post Q: Were do I find info about this thing called top-posting? A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Mon, Aug 01, 2022 at 03:04:33PM +0530, Siddh Raman Pant via Linux-kernel-mentees wrote: > Hello Dipanjan, > > It would be nice if you could test this patch and tell if it fixes the > issue on v5.10, as you had reported it earlier. > > Please apply the following commits before applying this patch: > db8facfc9faf ("watch_queue, pipe: Free watchqueue state after clearing pipe ring") > 353f7988dd84 ("watchqueue: make sure to serialize 'wqueue->defunct' properly") > > I have tested locally on tag v5.10, using the reproducer available on > syzkaller dashboard. The crash occurred when the patches weren't applied, > and it no longer occurs after applying the three patches. Trying anything on tag v5.10 is not a good test as that kernel is very old and obsolete and over 15000 changes behind what the latest 5.10.y kernel release has in it. So trying it on v5.10.134 would be best. thanks, greg k-h ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-01 10:24 ` Greg KH 0 siblings, 0 replies; 36+ messages in thread From: Greg KH @ 2022-08-01 10:24 UTC (permalink / raw) To: Siddh Raman Pant Cc: Eric Dumazet, linux-kernel, Fabio M. De Francesco, Dipanjan Das, syzbot+c70d87ac1d001f29a058, David Howells, linux-security-modules, Christophe JAILLET, linux-kernel-mentees A: http://en.wikipedia.org/wiki/Top_post Q: Were do I find info about this thing called top-posting? A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Mon, Aug 01, 2022 at 03:04:33PM +0530, Siddh Raman Pant via Linux-kernel-mentees wrote: > Hello Dipanjan, > > It would be nice if you could test this patch and tell if it fixes the > issue on v5.10, as you had reported it earlier. > > Please apply the following commits before applying this patch: > db8facfc9faf ("watch_queue, pipe: Free watchqueue state after clearing pipe ring") > 353f7988dd84 ("watchqueue: make sure to serialize 'wqueue->defunct' properly") > > I have tested locally on tag v5.10, using the reproducer available on > syzkaller dashboard. The crash occurred when the patches weren't applied, > and it no longer occurs after applying the three patches. Trying anything on tag v5.10 is not a good test as that kernel is very old and obsolete and over 15000 changes behind what the latest 5.10.y kernel release has in it. So trying it on v5.10.134 would be best. thanks, greg k-h _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-01 10:24 ` Greg KH @ 2022-08-01 12:58 ` Siddh Raman Pant -1 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-01 12:58 UTC (permalink / raw) To: Greg KH Cc: Eric Dumazet, linux-kernel, Fabio M. De Francesco, Dipanjan Das, syzbot+c70d87ac1d001f29a058, David Howells, linux-security-modules, Christophe JAILLET, linux-kernel-mentees On Mon, 01 Aug 2022 15:54:03 +0530 Greg KH <gregkh@linuxfoundation.org> wrote: > A: http://en.wikipedia.org/wiki/Top_post > Q: Were do I find info about this thing called top-posting? > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? > A: Top-posting. > Q: What is the most annoying thing in e-mail? > > A: No. > Q: Should I include quotations after my reply? > > http://daringfireball.net/2007/07/on_top > Oops, sorry. > Trying anything on tag v5.10 is not a good test as that kernel is very > old and obsolete and over 15000 changes behind what the latest 5.10.y > kernel release has in it. > > So trying it on v5.10.134 would be best. > > thanks, > > greg k-h > Noted. I now tried the 5.10.y branch of stable (which has v5.10.134), but the reproducer isn't triggering the bug for me. Thanks, Siddh _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-01 12:58 ` Siddh Raman Pant 0 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant @ 2022-08-01 12:58 UTC (permalink / raw) To: Greg KH Cc: Dipanjan Das, syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Mon, 01 Aug 2022 15:54:03 +0530 Greg KH <gregkh@linuxfoundation.org> wrote: > A: http://en.wikipedia.org/wiki/Top_post > Q: Were do I find info about this thing called top-posting? > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? > A: Top-posting. > Q: What is the most annoying thing in e-mail? > > A: No. > Q: Should I include quotations after my reply? > > http://daringfireball.net/2007/07/on_top > Oops, sorry. > Trying anything on tag v5.10 is not a good test as that kernel is very > old and obsolete and over 15000 changes behind what the latest 5.10.y > kernel release has in it. > > So trying it on v5.10.134 would be best. > > thanks, > > greg k-h > Noted. I now tried the 5.10.y branch of stable (which has v5.10.134), but the reproducer isn't triggering the bug for me. Thanks, Siddh ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-01 12:58 ` Siddh Raman Pant @ 2022-08-01 13:00 ` Siddh Raman Pant via Linux-kernel-mentees -1 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant @ 2022-08-01 13:00 UTC (permalink / raw) To: Greg KH Cc: Dipanjan Das, syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Mon, 01 Aug 2022 18:28:25 +0530 Siddh Raman Pant <code@siddh.me> wrote: > I now tried the 5.10.y branch of stable (which has v5.10.134), but the > reproducer isn't triggering the bug for me. By this, I mean without the patches. I should have been clear. Thanks, Siddh ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-01 13:00 ` Siddh Raman Pant via Linux-kernel-mentees 0 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-01 13:00 UTC (permalink / raw) To: Greg KH Cc: Eric Dumazet, linux-kernel, Fabio M. De Francesco, Dipanjan Das, syzbot+c70d87ac1d001f29a058, David Howells, linux-security-modules, Christophe JAILLET, linux-kernel-mentees On Mon, 01 Aug 2022 18:28:25 +0530 Siddh Raman Pant <code@siddh.me> wrote: > I now tried the 5.10.y branch of stable (which has v5.10.134), but the > reproducer isn't triggering the bug for me. By this, I mean without the patches. I should have been clear. Thanks, Siddh _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-01 13:00 ` Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-01 16:16 ` Dipanjan Das -1 siblings, 0 replies; 36+ messages in thread From: Dipanjan Das @ 2022-08-01 16:16 UTC (permalink / raw) To: Siddh Raman Pant Cc: Greg KH, syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Mon, Aug 01, 2022 at 06:30:58PM +0530, Siddh Raman Pant wrote: > On Mon, 01 Aug 2022 18:28:25 +0530 Siddh Raman Pant <code@siddh.me> wrote: > > I now tried the 5.10.y branch of stable (which has v5.10.134), but the > > reproducer isn't triggering the bug for me. Are you referring to the reproducer attached to our original report? https://lore.kernel.org/all/CANX2M5bHye2ZEEhEV6PUj1kYL2KdWYeJtgXw8KZRzwrNpLYz+A@mail.gmail.com/ ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-01 16:16 ` Dipanjan Das 0 siblings, 0 replies; 36+ messages in thread From: Dipanjan Das @ 2022-08-01 16:16 UTC (permalink / raw) To: Siddh Raman Pant Cc: Eric Dumazet, Fabio M. De Francesco, syzbot+c70d87ac1d001f29a058, linux-kernel, David Howells, linux-security-modules, Christophe JAILLET, linux-kernel-mentees On Mon, Aug 01, 2022 at 06:30:58PM +0530, Siddh Raman Pant wrote: > On Mon, 01 Aug 2022 18:28:25 +0530 Siddh Raman Pant <code@siddh.me> wrote: > > I now tried the 5.10.y branch of stable (which has v5.10.134), but the > > reproducer isn't triggering the bug for me. Are you referring to the reproducer attached to our original report? https://lore.kernel.org/all/CANX2M5bHye2ZEEhEV6PUj1kYL2KdWYeJtgXw8KZRzwrNpLYz+A@mail.gmail.com/ _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-01 16:16 ` Dipanjan Das @ 2022-08-01 18:49 ` Siddh Raman Pant -1 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-01 18:49 UTC (permalink / raw) To: Dipanjan Das Cc: Eric Dumazet, Fabio M. De Francesco, syzbot+c70d87ac1d001f29a058, linux-kernel, David Howells, linux-security-modules, Christophe JAILLET, linux-kernel-mentees On Mon, 01 Aug 2022 21:46:42 +0530 Dipanjan Das <mail.dipanjan.das@gmail.com> wrote: > Are you referring to the reproducer attached to our original report? > https://lore.kernel.org/all/CANX2M5bHye2ZEEhEV6PUj1kYL2KdWYeJtgXw8KZRzwrNpLYz+A@mail.gmail.com/ Yes, I meant the reproducer you gave. I suspect I must have missed CONFIG_WATCH_QUEUE=y while setting the kernel up, extremely sorry for it. I now tried 5.10.y with it (using a modification of syzkaller's dashboard config I had been using[1]), and I'm getting a __post_watch_notification() crash (which is a related crash, as the fix[2][3] for that causes the reproducer to not reproduce the post_one_notification crash on mainline), but not the post_one_notification() crash you had reported. It seems if I apply my patch, it doesn't trigger this related crash, so these bugs are seem to be very related maybe due to racing? I haven't looked at that yet. I then tried on v5.10.131 since that was the exact version you had reproduced on, and it reproduces the post_one_notification() error successfully. Applying 353f7988dd84 causes __post_watch_notification() crash, and then applying this v3 patch does not trigger the issue, but the patch to fix __post_watch_notification() crash is [2], which does not really address the issue of post_one_notification() crash which is due to the dangling reference to a freed pipe. Can you please try reproducer at your end? Thanks, Siddh [1] https://gist.github.com/siddhpant/06c7d64ca83273f0fd6604e4677f7c0d [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e64ab2dbd882933b65cd82ff6235d705ad65dbb6 [3] https://lore.kernel.org/linux-mm/18259769e5e.52eb2082293078.3991591702430862151@siddh.me/ _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-01 18:49 ` Siddh Raman Pant 0 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant @ 2022-08-01 18:49 UTC (permalink / raw) To: Dipanjan Das Cc: Greg KH, syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Mon, 01 Aug 2022 21:46:42 +0530 Dipanjan Das <mail.dipanjan.das@gmail.com> wrote: > Are you referring to the reproducer attached to our original report? > https://lore.kernel.org/all/CANX2M5bHye2ZEEhEV6PUj1kYL2KdWYeJtgXw8KZRzwrNpLYz+A@mail.gmail.com/ Yes, I meant the reproducer you gave. I suspect I must have missed CONFIG_WATCH_QUEUE=y while setting the kernel up, extremely sorry for it. I now tried 5.10.y with it (using a modification of syzkaller's dashboard config I had been using[1]), and I'm getting a __post_watch_notification() crash (which is a related crash, as the fix[2][3] for that causes the reproducer to not reproduce the post_one_notification crash on mainline), but not the post_one_notification() crash you had reported. It seems if I apply my patch, it doesn't trigger this related crash, so these bugs are seem to be very related maybe due to racing? I haven't looked at that yet. I then tried on v5.10.131 since that was the exact version you had reproduced on, and it reproduces the post_one_notification() error successfully. Applying 353f7988dd84 causes __post_watch_notification() crash, and then applying this v3 patch does not trigger the issue, but the patch to fix __post_watch_notification() crash is [2], which does not really address the issue of post_one_notification() crash which is due to the dangling reference to a freed pipe. Can you please try reproducer at your end? Thanks, Siddh [1] https://gist.github.com/siddhpant/06c7d64ca83273f0fd6604e4677f7c0d [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e64ab2dbd882933b65cd82ff6235d705ad65dbb6 [3] https://lore.kernel.org/linux-mm/18259769e5e.52eb2082293078.3991591702430862151@siddh.me/ ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-01 18:49 ` Siddh Raman Pant @ 2022-08-03 1:16 ` Eric Biggers -1 siblings, 0 replies; 36+ messages in thread From: Eric Biggers @ 2022-08-03 1:16 UTC (permalink / raw) To: Siddh Raman Pant Cc: Dipanjan Das, Greg KH, syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Tue, Aug 02, 2022 at 12:19:05AM +0530, Siddh Raman Pant wrote: > On Mon, 01 Aug 2022 21:46:42 +0530 Dipanjan Das <mail.dipanjan.das@gmail.com> wrote: > > Are you referring to the reproducer attached to our original report? > > https://lore.kernel.org/all/CANX2M5bHye2ZEEhEV6PUj1kYL2KdWYeJtgXw8KZRzwrNpLYz+A@mail.gmail.com/ > > Yes, I meant the reproducer you gave. > > I suspect I must have missed CONFIG_WATCH_QUEUE=y while setting the kernel > up, extremely sorry for it. > > I now tried 5.10.y with it (using a modification of syzkaller's dashboard > config I had been using[1]), and I'm getting a __post_watch_notification() > crash (which is a related crash, as the fix[2][3] for that causes the > reproducer to not reproduce the post_one_notification crash on mainline), > but not the post_one_notification() crash you had reported. It seems if I > apply my patch, it doesn't trigger this related crash, so these bugs are > seem to be very related maybe due to racing? I haven't looked at that yet. > > I then tried on v5.10.131 since that was the exact version you had > reproduced on, and it reproduces the post_one_notification() error > successfully. Applying 353f7988dd84 causes __post_watch_notification() > crash, and then applying this v3 patch does not trigger the issue, but > the patch to fix __post_watch_notification() crash is [2], which does > not really address the issue of post_one_notification() crash which > is due to the dangling reference to a freed pipe. > > Can you please try reproducer at your end? > > Thanks, > Siddh There were several watch_queue fixes that got merged in v5.10.134, so there's no point in testing v5.10.131. If you're still seeing a bug in the *latest* 5.10.y, then please check whether it's also present upstream. If yes, then send out a fix for it. If no, then tell stable@vger.kernel.org what commit(s) need to be backported. Please provide *full* details in each case, including any KASAN reports or other information that would help people understand the bug. - Eric ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-03 1:16 ` Eric Biggers 0 siblings, 0 replies; 36+ messages in thread From: Eric Biggers @ 2022-08-03 1:16 UTC (permalink / raw) To: Siddh Raman Pant Cc: linux-security-modules, Eric Dumazet, Fabio M. De Francesco, syzbot+c70d87ac1d001f29a058, linux-kernel, David Howells, Dipanjan Das, Christophe JAILLET, linux-kernel-mentees On Tue, Aug 02, 2022 at 12:19:05AM +0530, Siddh Raman Pant wrote: > On Mon, 01 Aug 2022 21:46:42 +0530 Dipanjan Das <mail.dipanjan.das@gmail.com> wrote: > > Are you referring to the reproducer attached to our original report? > > https://lore.kernel.org/all/CANX2M5bHye2ZEEhEV6PUj1kYL2KdWYeJtgXw8KZRzwrNpLYz+A@mail.gmail.com/ > > Yes, I meant the reproducer you gave. > > I suspect I must have missed CONFIG_WATCH_QUEUE=y while setting the kernel > up, extremely sorry for it. > > I now tried 5.10.y with it (using a modification of syzkaller's dashboard > config I had been using[1]), and I'm getting a __post_watch_notification() > crash (which is a related crash, as the fix[2][3] for that causes the > reproducer to not reproduce the post_one_notification crash on mainline), > but not the post_one_notification() crash you had reported. It seems if I > apply my patch, it doesn't trigger this related crash, so these bugs are > seem to be very related maybe due to racing? I haven't looked at that yet. > > I then tried on v5.10.131 since that was the exact version you had > reproduced on, and it reproduces the post_one_notification() error > successfully. Applying 353f7988dd84 causes __post_watch_notification() > crash, and then applying this v3 patch does not trigger the issue, but > the patch to fix __post_watch_notification() crash is [2], which does > not really address the issue of post_one_notification() crash which > is due to the dangling reference to a freed pipe. > > Can you please try reproducer at your end? > > Thanks, > Siddh There were several watch_queue fixes that got merged in v5.10.134, so there's no point in testing v5.10.131. If you're still seeing a bug in the *latest* 5.10.y, then please check whether it's also present upstream. If yes, then send out a fix for it. If no, then tell stable@vger.kernel.org what commit(s) need to be backported. Please provide *full* details in each case, including any KASAN reports or other information that would help people understand the bug. - Eric _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-03 1:16 ` Eric Biggers @ 2022-08-03 3:58 ` Siddh Raman Pant -1 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-03 3:58 UTC (permalink / raw) To: Eric Biggers Cc: linux-security-modules, Eric Dumazet, Fabio M. De Francesco, syzbot+c70d87ac1d001f29a058, linux-kernel, David Howells, Dipanjan Das, Christophe JAILLET, linux-kernel-mentees On Wed, 03 Aug 2022 06:46:13 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > There were several watch_queue fixes that got merged in v5.10.134, so there's no > point in testing v5.10.131. If you're still seeing a bug in the *latest* > 5.10.y, then please check whether it's also present upstream. If yes, then send > out a fix for it. If no, then tell stable@vger.kernel.org what commit(s) need > to be backported. Please provide *full* details in each case, including any > KASAN reports or other information that would help people understand the bug. > > - Eric > Okay, noted. Thanks, Siddh _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-03 3:58 ` Siddh Raman Pant 0 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant @ 2022-08-03 3:58 UTC (permalink / raw) To: Eric Biggers Cc: Dipanjan Das, Greg KH, syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Wed, 03 Aug 2022 06:46:13 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > There were several watch_queue fixes that got merged in v5.10.134, so there's no > point in testing v5.10.131. If you're still seeing a bug in the *latest* > 5.10.y, then please check whether it's also present upstream. If yes, then send > out a fix for it. If no, then tell stable@vger.kernel.org what commit(s) need > to be backported. Please provide *full* details in each case, including any > KASAN reports or other information that would help people understand the bug. > > - Eric > Okay, noted. Thanks, Siddh ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-01 18:49 ` Siddh Raman Pant @ 2022-08-03 4:10 ` Dipanjan Das -1 siblings, 0 replies; 36+ messages in thread From: Dipanjan Das @ 2022-08-03 4:10 UTC (permalink / raw) To: Siddh Raman Pant Cc: Greg KH, syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco, Priyanka Bose, Marius Fleischer On Mon, Aug 1, 2022 at 11:49 AM Siddh Raman Pant <code@siddh.me> wrote: > > On Mon, 01 Aug 2022 21:46:42 +0530 Dipanjan Das <mail.dipanjan.das@gmail.com> wrote: > > Are you referring to the reproducer attached to our original report? > > https://lore.kernel.org/all/CANX2M5bHye2ZEEhEV6PUj1kYL2KdWYeJtgXw8KZRzwrNpLYz+A@mail.gmail.com/ > > Yes, I meant the reproducer you gave. > > I suspect I must have missed CONFIG_WATCH_QUEUE=y while setting the kernel > up, extremely sorry for it. > > I now tried 5.10.y with it (using a modification of syzkaller's dashboard > config I had been using[1]), and I'm getting a __post_watch_notification() > crash (which is a related crash, as the fix[2][3] for that causes the > reproducer to not reproduce the post_one_notification crash on mainline), > but not the post_one_notification() crash you had reported. It seems if I > apply my patch, it doesn't trigger this related crash, so these bugs are > seem to be very related maybe due to racing? I haven't looked at that yet. > > I then tried on v5.10.131 since that was the exact version you had > reproduced on, and it reproduces the post_one_notification() error > successfully. Applying 353f7988dd84 causes __post_watch_notification() > crash, and then applying this v3 patch does not trigger the issue, but > the patch to fix __post_watch_notification() crash is [2], which does > not really address the issue of post_one_notification() crash which > is due to the dangling reference to a freed pipe. > > Can you please try reproducer at your end? We can confirm that the repro triggers the `post_watch_notification` crash on 5.10.134 (7a62a4b6212a7f04251fdaf8b049c47aec59c54a). After applying your v3 patch, the repro did not trigger the bug. > > [1] https://gist.github.com/siddhpant/06c7d64ca83273f0fd6604e4677f7c0d > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e64ab2dbd882933b65cd82ff6235d705ad65dbb6 > [3] https://lore.kernel.org/linux-mm/18259769e5e.52eb2082293078.3991591702430862151@siddh.me/ -- Thanks and Regards, Dipanjan ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-03 4:10 ` Dipanjan Das 0 siblings, 0 replies; 36+ messages in thread From: Dipanjan Das @ 2022-08-03 4:10 UTC (permalink / raw) To: Siddh Raman Pant Cc: Eric Dumazet, Marius Fleischer, Fabio M. De Francesco, syzbot+c70d87ac1d001f29a058, linux-kernel, David Howells, Priyanka Bose, linux-security-modules, Christophe JAILLET, linux-kernel-mentees On Mon, Aug 1, 2022 at 11:49 AM Siddh Raman Pant <code@siddh.me> wrote: > > On Mon, 01 Aug 2022 21:46:42 +0530 Dipanjan Das <mail.dipanjan.das@gmail.com> wrote: > > Are you referring to the reproducer attached to our original report? > > https://lore.kernel.org/all/CANX2M5bHye2ZEEhEV6PUj1kYL2KdWYeJtgXw8KZRzwrNpLYz+A@mail.gmail.com/ > > Yes, I meant the reproducer you gave. > > I suspect I must have missed CONFIG_WATCH_QUEUE=y while setting the kernel > up, extremely sorry for it. > > I now tried 5.10.y with it (using a modification of syzkaller's dashboard > config I had been using[1]), and I'm getting a __post_watch_notification() > crash (which is a related crash, as the fix[2][3] for that causes the > reproducer to not reproduce the post_one_notification crash on mainline), > but not the post_one_notification() crash you had reported. It seems if I > apply my patch, it doesn't trigger this related crash, so these bugs are > seem to be very related maybe due to racing? I haven't looked at that yet. > > I then tried on v5.10.131 since that was the exact version you had > reproduced on, and it reproduces the post_one_notification() error > successfully. Applying 353f7988dd84 causes __post_watch_notification() > crash, and then applying this v3 patch does not trigger the issue, but > the patch to fix __post_watch_notification() crash is [2], which does > not really address the issue of post_one_notification() crash which > is due to the dangling reference to a freed pipe. > > Can you please try reproducer at your end? We can confirm that the repro triggers the `post_watch_notification` crash on 5.10.134 (7a62a4b6212a7f04251fdaf8b049c47aec59c54a). After applying your v3 patch, the repro did not trigger the bug. > > [1] https://gist.github.com/siddhpant/06c7d64ca83273f0fd6604e4677f7c0d > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e64ab2dbd882933b65cd82ff6235d705ad65dbb6 > [3] https://lore.kernel.org/linux-mm/18259769e5e.52eb2082293078.3991591702430862151@siddh.me/ -- Thanks and Regards, Dipanjan _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-07-28 15:51 ` Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-03 1:08 ` Eric Biggers -1 siblings, 0 replies; 36+ messages in thread From: Eric Biggers @ 2022-08-03 1:08 UTC (permalink / raw) To: Siddh Raman Pant Cc: David Howells, Christophe JAILLET, Eric Dumazet, Fabio M. De Francesco, linux-security-modules, linux-kernel, linux-kernel-mentees, syzbot+c70d87ac1d001f29a058 On Thu, Jul 28, 2022 at 09:21:21PM +0530, Siddh Raman Pant wrote: > If not done, a reference to a freed pipe remains in the watch_queue, > as this function is called before freeing a pipe in free_pipe_info() > (see line 834 of fs/pipe.c). > > This causes a UAF when post_one_notification() tries to access the pipe > on a key update, which is reported by syzbot. > > We also need to use READ_ONCE() in post_one_notification() to prevent the > compiler from optimising and loading a non-NULL value from wqueue->pipe. Didn't this already get fixed by the following commit? commit 353f7988dd8413c47718f7ca79c030b6fb62cfe5 Author: Linus Torvalds <torvalds@linux-foundation.org> Date: Tue Jul 19 11:09:01 2022 -0700 watchqueue: make sure to serialize 'wqueue->defunct' properly With that, post_one_notification() only runs while the watch_queue is locked and not "defunct". So it's guaranteed that the pipe still exists. Any concurrent free_pipe_info() waits for the watch_queue to be unlocked in watch_queue_clear() before proceeding to free the pipe. So where is there still a bug? > > Bug report: https://syzkaller.appspot.com/bug?id=1870dd7791ba05f2ea7f47f7cbdde701173973fc > Reported-and-tested-by: syzbot+c70d87ac1d001f29a058@syzkaller.appspotmail.com If this actually does fix something, then it's mixing Fixes and Cc stable tags. > diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c > index bb9962b33f95..617425e34252 100644 > --- a/kernel/watch_queue.c > +++ b/kernel/watch_queue.c > @@ -99,7 +99,7 @@ static bool post_one_notification(struct watch_queue *wqueue, > struct watch_notification *n) > { > void *p; > - struct pipe_inode_info *pipe = wqueue->pipe; > + struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe); > struct pipe_buffer *buf; > struct page *page; > unsigned int head, tail, mask, note, offset, len; > @@ -637,6 +637,12 @@ void watch_queue_clear(struct watch_queue *wqueue) > spin_lock_bh(&wqueue->lock); > } > > + /* Clearing the watch queue, so we should clean the associated pipe. */ > + if (wqueue->pipe) { > + wqueue->pipe->watch_queue = NULL; > + wqueue->pipe = NULL; > + } > + > spin_unlock_bh(&wqueue->lock); > rcu_read_unlock(); > } And this is clearly the wrong fix anyway, since it makes the call to put_watch_queue() in free_pipe_info() never be executed. So AFAICT, this patch introduces a memory leak, and doesn't actually fix anything... - Eric ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-03 1:08 ` Eric Biggers 0 siblings, 0 replies; 36+ messages in thread From: Eric Biggers @ 2022-08-03 1:08 UTC (permalink / raw) To: Siddh Raman Pant Cc: syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Thu, Jul 28, 2022 at 09:21:21PM +0530, Siddh Raman Pant wrote: > If not done, a reference to a freed pipe remains in the watch_queue, > as this function is called before freeing a pipe in free_pipe_info() > (see line 834 of fs/pipe.c). > > This causes a UAF when post_one_notification() tries to access the pipe > on a key update, which is reported by syzbot. > > We also need to use READ_ONCE() in post_one_notification() to prevent the > compiler from optimising and loading a non-NULL value from wqueue->pipe. Didn't this already get fixed by the following commit? commit 353f7988dd8413c47718f7ca79c030b6fb62cfe5 Author: Linus Torvalds <torvalds@linux-foundation.org> Date: Tue Jul 19 11:09:01 2022 -0700 watchqueue: make sure to serialize 'wqueue->defunct' properly With that, post_one_notification() only runs while the watch_queue is locked and not "defunct". So it's guaranteed that the pipe still exists. Any concurrent free_pipe_info() waits for the watch_queue to be unlocked in watch_queue_clear() before proceeding to free the pipe. So where is there still a bug? > > Bug report: https://syzkaller.appspot.com/bug?id=1870dd7791ba05f2ea7f47f7cbdde701173973fc > Reported-and-tested-by: syzbot+c70d87ac1d001f29a058@syzkaller.appspotmail.com If this actually does fix something, then it's mixing Fixes and Cc stable tags. > diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c > index bb9962b33f95..617425e34252 100644 > --- a/kernel/watch_queue.c > +++ b/kernel/watch_queue.c > @@ -99,7 +99,7 @@ static bool post_one_notification(struct watch_queue *wqueue, > struct watch_notification *n) > { > void *p; > - struct pipe_inode_info *pipe = wqueue->pipe; > + struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe); > struct pipe_buffer *buf; > struct page *page; > unsigned int head, tail, mask, note, offset, len; > @@ -637,6 +637,12 @@ void watch_queue_clear(struct watch_queue *wqueue) > spin_lock_bh(&wqueue->lock); > } > > + /* Clearing the watch queue, so we should clean the associated pipe. */ > + if (wqueue->pipe) { > + wqueue->pipe->watch_queue = NULL; > + wqueue->pipe = NULL; > + } > + > spin_unlock_bh(&wqueue->lock); > rcu_read_unlock(); > } And this is clearly the wrong fix anyway, since it makes the call to put_watch_queue() in free_pipe_info() never be executed. So AFAICT, this patch introduces a memory leak, and doesn't actually fix anything... - Eric _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-03 1:08 ` Eric Biggers @ 2022-08-03 3:56 ` Siddh Raman Pant via Linux-kernel-mentees -1 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant @ 2022-08-03 3:56 UTC (permalink / raw) To: Eric Biggers Cc: David Howells, Christophe JAILLET, Eric Dumazet, Fabio M. De Francesco, linux-security-modules, linux-kernel, linux-kernel-mentees, syzbot+c70d87ac1d001f29a058 On Wed, 03 Aug 2022 06:38:36 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > On Thu, Jul 28, 2022 at 09:21:21PM +0530, Siddh Raman Pant wrote: > > If not done, a reference to a freed pipe remains in the watch_queue, > > as this function is called before freeing a pipe in free_pipe_info() > > (see line 834 of fs/pipe.c). > > > > This causes a UAF when post_one_notification() tries to access the pipe > > on a key update, which is reported by syzbot. > > > > We also need to use READ_ONCE() in post_one_notification() to prevent the > > compiler from optimising and loading a non-NULL value from wqueue->pipe. > > Didn't this already get fixed by the following commit? > > commit 353f7988dd8413c47718f7ca79c030b6fb62cfe5 > Author: Linus Torvalds <torvalds@linux-foundation.org> > Date: Tue Jul 19 11:09:01 2022 -0700 > > watchqueue: make sure to serialize 'wqueue->defunct' properly > > With that, post_one_notification() only runs while the watch_queue is locked and > not "defunct". So it's guaranteed that the pipe still exists. Any concurrent > free_pipe_info() waits for the watch_queue to be unlocked in watch_queue_clear() > before proceeding to free the pipe. So where is there still a bug? It doesn't fix the dangling pointer to the freed pipe in the watch_queue, which had caused this crash. > > > > Bug report: https://syzkaller.appspot.com/bug?id=1870dd7791ba05f2ea7f47f7cbdde701173973fc > > Reported-and-tested-by: syzbot+c70d87ac1d001f29a058@syzkaller.appspotmail.com > > If this actually does fix something, then it's mixing Fixes and Cc stable tags. Noted. > > diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c > > index bb9962b33f95..617425e34252 100644 > > --- a/kernel/watch_queue.c > > +++ b/kernel/watch_queue.c > > @@ -99,7 +99,7 @@ static bool post_one_notification(struct watch_queue *wqueue, > > struct watch_notification *n) > > { > > void *p; > > - struct pipe_inode_info *pipe = wqueue->pipe; > > + struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe); > > struct pipe_buffer *buf; > > struct page *page; > > unsigned int head, tail, mask, note, offset, len; > > @@ -637,6 +637,12 @@ void watch_queue_clear(struct watch_queue *wqueue) > > spin_lock_bh(&wqueue->lock); > > } > > > > + /* Clearing the watch queue, so we should clean the associated pipe. */ > > + if (wqueue->pipe) { > > + wqueue->pipe->watch_queue = NULL; > > + wqueue->pipe = NULL; > > + } > > + > > spin_unlock_bh(&wqueue->lock); > > rcu_read_unlock(); > > } > > And this is clearly the wrong fix anyway, since it makes the call to > put_watch_queue() in free_pipe_info() never be executed. So AFAICT, this patch > introduces a memory leak, and doesn't actually fix anything... > > - Eric > Sorry for overlooking that. wqueue->pipe->watch_queue = NULL; is unnecessary, but wqueue->pipe = NULL; is needed. I will send a final v4 once all issues are resolved in this thread. Thanks, Siddh ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-03 3:56 ` Siddh Raman Pant via Linux-kernel-mentees 0 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-03 3:56 UTC (permalink / raw) To: Eric Biggers Cc: syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Wed, 03 Aug 2022 06:38:36 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > On Thu, Jul 28, 2022 at 09:21:21PM +0530, Siddh Raman Pant wrote: > > If not done, a reference to a freed pipe remains in the watch_queue, > > as this function is called before freeing a pipe in free_pipe_info() > > (see line 834 of fs/pipe.c). > > > > This causes a UAF when post_one_notification() tries to access the pipe > > on a key update, which is reported by syzbot. > > > > We also need to use READ_ONCE() in post_one_notification() to prevent the > > compiler from optimising and loading a non-NULL value from wqueue->pipe. > > Didn't this already get fixed by the following commit? > > commit 353f7988dd8413c47718f7ca79c030b6fb62cfe5 > Author: Linus Torvalds <torvalds@linux-foundation.org> > Date: Tue Jul 19 11:09:01 2022 -0700 > > watchqueue: make sure to serialize 'wqueue->defunct' properly > > With that, post_one_notification() only runs while the watch_queue is locked and > not "defunct". So it's guaranteed that the pipe still exists. Any concurrent > free_pipe_info() waits for the watch_queue to be unlocked in watch_queue_clear() > before proceeding to free the pipe. So where is there still a bug? It doesn't fix the dangling pointer to the freed pipe in the watch_queue, which had caused this crash. > > > > Bug report: https://syzkaller.appspot.com/bug?id=1870dd7791ba05f2ea7f47f7cbdde701173973fc > > Reported-and-tested-by: syzbot+c70d87ac1d001f29a058@syzkaller.appspotmail.com > > If this actually does fix something, then it's mixing Fixes and Cc stable tags. Noted. > > diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c > > index bb9962b33f95..617425e34252 100644 > > --- a/kernel/watch_queue.c > > +++ b/kernel/watch_queue.c > > @@ -99,7 +99,7 @@ static bool post_one_notification(struct watch_queue *wqueue, > > struct watch_notification *n) > > { > > void *p; > > - struct pipe_inode_info *pipe = wqueue->pipe; > > + struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe); > > struct pipe_buffer *buf; > > struct page *page; > > unsigned int head, tail, mask, note, offset, len; > > @@ -637,6 +637,12 @@ void watch_queue_clear(struct watch_queue *wqueue) > > spin_lock_bh(&wqueue->lock); > > } > > > > + /* Clearing the watch queue, so we should clean the associated pipe. */ > > + if (wqueue->pipe) { > > + wqueue->pipe->watch_queue = NULL; > > + wqueue->pipe = NULL; > > + } > > + > > spin_unlock_bh(&wqueue->lock); > > rcu_read_unlock(); > > } > > And this is clearly the wrong fix anyway, since it makes the call to > put_watch_queue() in free_pipe_info() never be executed. So AFAICT, this patch > introduces a memory leak, and doesn't actually fix anything... > > - Eric > Sorry for overlooking that. wqueue->pipe->watch_queue = NULL; is unnecessary, but wqueue->pipe = NULL; is needed. I will send a final v4 once all issues are resolved in this thread. Thanks, Siddh _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-03 3:56 ` Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-03 4:12 ` Eric Biggers -1 siblings, 0 replies; 36+ messages in thread From: Eric Biggers @ 2022-08-03 4:12 UTC (permalink / raw) To: Siddh Raman Pant Cc: David Howells, Christophe JAILLET, Eric Dumazet, Fabio M. De Francesco, linux-security-modules, linux-kernel, linux-kernel-mentees, syzbot+c70d87ac1d001f29a058 On Wed, Aug 03, 2022 at 09:26:04AM +0530, Siddh Raman Pant wrote: > On Wed, 03 Aug 2022 06:38:36 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > > On Thu, Jul 28, 2022 at 09:21:21PM +0530, Siddh Raman Pant wrote: > > > If not done, a reference to a freed pipe remains in the watch_queue, > > > as this function is called before freeing a pipe in free_pipe_info() > > > (see line 834 of fs/pipe.c). > > > > > > This causes a UAF when post_one_notification() tries to access the pipe > > > on a key update, which is reported by syzbot. > > > > > > We also need to use READ_ONCE() in post_one_notification() to prevent the > > > compiler from optimising and loading a non-NULL value from wqueue->pipe. > > > > Didn't this already get fixed by the following commit? > > > > commit 353f7988dd8413c47718f7ca79c030b6fb62cfe5 > > Author: Linus Torvalds <torvalds@linux-foundation.org> > > Date: Tue Jul 19 11:09:01 2022 -0700 > > > > watchqueue: make sure to serialize 'wqueue->defunct' properly > > > > With that, post_one_notification() only runs while the watch_queue is locked and > > not "defunct". So it's guaranteed that the pipe still exists. Any concurrent > > free_pipe_info() waits for the watch_queue to be unlocked in watch_queue_clear() > > before proceeding to free the pipe. So where is there still a bug? > > It doesn't fix the dangling pointer to the freed pipe in the watch_queue, which > had caused this crash. > Under what circumstances is the pipe pointer still being dereferenced after the pipe has been freed? I don't see how it can be; see my explanation above. - Eric ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-03 4:12 ` Eric Biggers 0 siblings, 0 replies; 36+ messages in thread From: Eric Biggers @ 2022-08-03 4:12 UTC (permalink / raw) To: Siddh Raman Pant Cc: syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Wed, Aug 03, 2022 at 09:26:04AM +0530, Siddh Raman Pant wrote: > On Wed, 03 Aug 2022 06:38:36 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > > On Thu, Jul 28, 2022 at 09:21:21PM +0530, Siddh Raman Pant wrote: > > > If not done, a reference to a freed pipe remains in the watch_queue, > > > as this function is called before freeing a pipe in free_pipe_info() > > > (see line 834 of fs/pipe.c). > > > > > > This causes a UAF when post_one_notification() tries to access the pipe > > > on a key update, which is reported by syzbot. > > > > > > We also need to use READ_ONCE() in post_one_notification() to prevent the > > > compiler from optimising and loading a non-NULL value from wqueue->pipe. > > > > Didn't this already get fixed by the following commit? > > > > commit 353f7988dd8413c47718f7ca79c030b6fb62cfe5 > > Author: Linus Torvalds <torvalds@linux-foundation.org> > > Date: Tue Jul 19 11:09:01 2022 -0700 > > > > watchqueue: make sure to serialize 'wqueue->defunct' properly > > > > With that, post_one_notification() only runs while the watch_queue is locked and > > not "defunct". So it's guaranteed that the pipe still exists. Any concurrent > > free_pipe_info() waits for the watch_queue to be unlocked in watch_queue_clear() > > before proceeding to free the pipe. So where is there still a bug? > > It doesn't fix the dangling pointer to the freed pipe in the watch_queue, which > had caused this crash. > Under what circumstances is the pipe pointer still being dereferenced after the pipe has been freed? I don't see how it can be; see my explanation above. - Eric _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-03 4:12 ` Eric Biggers @ 2022-08-03 5:13 ` Siddh Raman Pant via Linux-kernel-mentees -1 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant @ 2022-08-03 5:13 UTC (permalink / raw) To: Eric Biggers Cc: David Howells, Christophe JAILLET, Eric Dumazet, Fabio M. De Francesco, linux-security-modules, linux-kernel, linux-kernel-mentees, syzbot+c70d87ac1d001f29a058 On Wed, 03 Aug 2022 09:42:28 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > Under what circumstances is the pipe pointer still being dereferenced after the > pipe has been freed? I don't see how it can be; see my explanation above. It really didn't fix the crash. It caused the same crash reported here, which I was already locally getting: https://syzkaller.appspot.com/bug?extid=03d7b43290037d1f87ca (It's same because __post_watch_notification calls post_one_notification, and this patch seems to stop that crash too, as was verified by Dipanjan here). While it has been fixed by e64ab2dbd882 ("watch_queue: Fix missing locking in add_watch_to_object()"), it just shows there can be paths leading to it. (Also, I posted this patch (v1, v2, v3) before that even landed, so I had no way of knowing about it). There is a null check in post_one_notification for the pipe, most probably because it *expects* the pointer to be NULL'd. Also, there is no reason to have a dangling pointer stay, it's just a recipe for further bugs. Thanks, Siddh ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-03 5:13 ` Siddh Raman Pant via Linux-kernel-mentees 0 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-03 5:13 UTC (permalink / raw) To: Eric Biggers Cc: syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Wed, 03 Aug 2022 09:42:28 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > Under what circumstances is the pipe pointer still being dereferenced after the > pipe has been freed? I don't see how it can be; see my explanation above. It really didn't fix the crash. It caused the same crash reported here, which I was already locally getting: https://syzkaller.appspot.com/bug?extid=03d7b43290037d1f87ca (It's same because __post_watch_notification calls post_one_notification, and this patch seems to stop that crash too, as was verified by Dipanjan here). While it has been fixed by e64ab2dbd882 ("watch_queue: Fix missing locking in add_watch_to_object()"), it just shows there can be paths leading to it. (Also, I posted this patch (v1, v2, v3) before that even landed, so I had no way of knowing about it). There is a null check in post_one_notification for the pipe, most probably because it *expects* the pointer to be NULL'd. Also, there is no reason to have a dangling pointer stay, it's just a recipe for further bugs. Thanks, Siddh _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-03 5:13 ` Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-03 5:41 ` Eric Biggers -1 siblings, 0 replies; 36+ messages in thread From: Eric Biggers @ 2022-08-03 5:41 UTC (permalink / raw) To: Siddh Raman Pant Cc: David Howells, Christophe JAILLET, Eric Dumazet, Fabio M. De Francesco, linux-security-modules, linux-kernel, linux-kernel-mentees, syzbot+c70d87ac1d001f29a058 On Wed, Aug 03, 2022 at 10:43:31AM +0530, Siddh Raman Pant wrote: > On Wed, 03 Aug 2022 09:42:28 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > > Under what circumstances is the pipe pointer still being dereferenced after the > > pipe has been freed? I don't see how it can be; see my explanation above. > > It really didn't fix the crash. It caused the same crash reported here, which > I was already locally getting: > https://syzkaller.appspot.com/bug?extid=03d7b43290037d1f87ca I tested the syzbot reproducer https://syzkaller.appspot.com/text?tag=ReproC&x=174ea97e080000, and it does *not* trigger the bug on the latest upstream. But, it does trigger the bug if I recent Linus's recent watch_queue fixes. So I don't currently see any evidence of an unfixed bug. If, nevertheless, you believe that a bug is still unfixed, then please provide a reproducer and a fix patch that clearly explains what it is fixing. > There is a null check in post_one_notification for the pipe, most probably > because it *expects* the pointer to be NULL'd. Also, there is no reason to have > a dangling pointer stay, it's just a recipe for further bugs. If you want to send a patch or patches to clean up the code, that is fine, but please make it super clear what is a cleanup and what is a fix. - Eric ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-03 5:41 ` Eric Biggers 0 siblings, 0 replies; 36+ messages in thread From: Eric Biggers @ 2022-08-03 5:41 UTC (permalink / raw) To: Siddh Raman Pant Cc: syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Wed, Aug 03, 2022 at 10:43:31AM +0530, Siddh Raman Pant wrote: > On Wed, 03 Aug 2022 09:42:28 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > > Under what circumstances is the pipe pointer still being dereferenced after the > > pipe has been freed? I don't see how it can be; see my explanation above. > > It really didn't fix the crash. It caused the same crash reported here, which > I was already locally getting: > https://syzkaller.appspot.com/bug?extid=03d7b43290037d1f87ca I tested the syzbot reproducer https://syzkaller.appspot.com/text?tag=ReproC&x=174ea97e080000, and it does *not* trigger the bug on the latest upstream. But, it does trigger the bug if I recent Linus's recent watch_queue fixes. So I don't currently see any evidence of an unfixed bug. If, nevertheless, you believe that a bug is still unfixed, then please provide a reproducer and a fix patch that clearly explains what it is fixing. > There is a null check in post_one_notification for the pipe, most probably > because it *expects* the pointer to be NULL'd. Also, there is no reason to have > a dangling pointer stay, it's just a recipe for further bugs. If you want to send a patch or patches to clean up the code, that is fine, but please make it super clear what is a cleanup and what is a fix. - Eric _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-03 5:41 ` Eric Biggers @ 2022-08-03 8:40 ` Siddh Raman Pant via Linux-kernel-mentees -1 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant @ 2022-08-03 8:40 UTC (permalink / raw) To: Eric Biggers Cc: David Howells, Christophe JAILLET, Eric Dumazet, Fabio M. De Francesco, linux-security-modules, linux-kernel, linux-kernel-mentees, syzbot+c70d87ac1d001f29a058 On Wed, 03 Aug 2022 11:11:31 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > I tested the syzbot reproducer > https://syzkaller.appspot.com/text?tag=ReproC&x=174ea97e080000, and it does > *not* trigger the bug on the latest upstream. But, it does trigger the bug if I > recent Linus's recent watch_queue fixes. > > So I don't currently see any evidence of an unfixed bug. If, nevertheless, you > believe that a bug is still unfixed, then please provide a reproducer and a fix > patch that clearly explains what it is fixing. > > > There is a null check in post_one_notification for the pipe, most probably > > because it *expects* the pointer to be NULL'd. Also, there is no reason to have > > a dangling pointer stay, it's just a recipe for further bugs. > > If you want to send a patch or patches to clean up the code, that is fine, but > please make it super clear what is a cleanup and what is a fix. > > - Eric > I honestly feel like I am repeating myself yet again, but okay. Of course, the race condition has been solved by a patch upstream, which I had myself mentioned earlier. But what I am saying is that it did *not* address *what* that race condition had triggered, i.e. the visible cause of the UAF crash, which, among other things, is *because* there is a dangling pointer to the freed pipe, which *caused* the crash in post_one_notification() when it tried to access &pipe->rd.wait_lock as an argument to spin_lock_irq(), a path it reached after checking if wqueue->pipe is NULL and proceeded when it was not the case. And the upstream commit was made *after* I had posted this patch, hence this was a fix for the syzkaller issue. While I am *not* saying to accept it just because this was posted earlier, I am saying this patch addresses a parallel issue, i.e. the *actual use-after-free crash* which was reproduced by those reproducers, i.e., what was attempted to be used after getting freed and detected by KASAN. We don't need to wait for another similar syzbot report to pop up before doing this change, and say let's not fix a dangling pointer reference because now another commit apparately fixes the specific syzkaller issue, causing the given specific reproducer with its specific way of reproducing to fail, when we in fact now know it *can* be a valid problem in practice and doing this change too causes the specific reproducer under consideration to fail reproducing, as was reported by the reproducer itself. I really don't know how to create stress tests / reproducers like how syzkaller makes, so if a similar new reproducer is really required for showing this patch's validity disregarding any earlier reproducers, I unfortunately cannot make it due to skill issue as I just started in kernel dev, and I am deeply sorry for wasting the time of everyone, and I am thankful for your criticism of my patch. Thanks, Siddh ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-03 8:40 ` Siddh Raman Pant via Linux-kernel-mentees 0 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-03 8:40 UTC (permalink / raw) To: Eric Biggers Cc: syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Wed, 03 Aug 2022 11:11:31 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > I tested the syzbot reproducer > https://syzkaller.appspot.com/text?tag=ReproC&x=174ea97e080000, and it does > *not* trigger the bug on the latest upstream. But, it does trigger the bug if I > recent Linus's recent watch_queue fixes. > > So I don't currently see any evidence of an unfixed bug. If, nevertheless, you > believe that a bug is still unfixed, then please provide a reproducer and a fix > patch that clearly explains what it is fixing. > > > There is a null check in post_one_notification for the pipe, most probably > > because it *expects* the pointer to be NULL'd. Also, there is no reason to have > > a dangling pointer stay, it's just a recipe for further bugs. > > If you want to send a patch or patches to clean up the code, that is fine, but > please make it super clear what is a cleanup and what is a fix. > > - Eric > I honestly feel like I am repeating myself yet again, but okay. Of course, the race condition has been solved by a patch upstream, which I had myself mentioned earlier. But what I am saying is that it did *not* address *what* that race condition had triggered, i.e. the visible cause of the UAF crash, which, among other things, is *because* there is a dangling pointer to the freed pipe, which *caused* the crash in post_one_notification() when it tried to access &pipe->rd.wait_lock as an argument to spin_lock_irq(), a path it reached after checking if wqueue->pipe is NULL and proceeded when it was not the case. And the upstream commit was made *after* I had posted this patch, hence this was a fix for the syzkaller issue. While I am *not* saying to accept it just because this was posted earlier, I am saying this patch addresses a parallel issue, i.e. the *actual use-after-free crash* which was reproduced by those reproducers, i.e., what was attempted to be used after getting freed and detected by KASAN. We don't need to wait for another similar syzbot report to pop up before doing this change, and say let's not fix a dangling pointer reference because now another commit apparately fixes the specific syzkaller issue, causing the given specific reproducer with its specific way of reproducing to fail, when we in fact now know it *can* be a valid problem in practice and doing this change too causes the specific reproducer under consideration to fail reproducing, as was reported by the reproducer itself. I really don't know how to create stress tests / reproducers like how syzkaller makes, so if a similar new reproducer is really required for showing this patch's validity disregarding any earlier reproducers, I unfortunately cannot make it due to skill issue as I just started in kernel dev, and I am deeply sorry for wasting the time of everyone, and I am thankful for your criticism of my patch. Thanks, Siddh _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-03 8:40 ` Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-03 18:15 ` Eric Biggers -1 siblings, 0 replies; 36+ messages in thread From: Eric Biggers @ 2022-08-03 18:15 UTC (permalink / raw) To: Siddh Raman Pant Cc: syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Wed, Aug 03, 2022 at 02:10:06PM +0530, Siddh Raman Pant wrote: > On Wed, 03 Aug 2022 11:11:31 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > > I tested the syzbot reproducer > > https://syzkaller.appspot.com/text?tag=ReproC&x=174ea97e080000, and it does > > *not* trigger the bug on the latest upstream. But, it does trigger the bug if I > > recent Linus's recent watch_queue fixes. > > > > So I don't currently see any evidence of an unfixed bug. If, nevertheless, you > > believe that a bug is still unfixed, then please provide a reproducer and a fix > > patch that clearly explains what it is fixing. > > > > > There is a null check in post_one_notification for the pipe, most probably > > > because it *expects* the pointer to be NULL'd. Also, there is no reason to have > > > a dangling pointer stay, it's just a recipe for further bugs. > > > > If you want to send a patch or patches to clean up the code, that is fine, but > > please make it super clear what is a cleanup and what is a fix. > > > > - Eric > > > > I honestly feel like I am repeating myself yet again, but okay. Well, you should try listening instead. Because you are not listening. > Of course, the race condition has been solved by a patch upstream, which I had > myself mentioned earlier. > > But what I am saying is that it did *not* address *what* that race condition > had triggered, i.e. the visible cause of the UAF crash, which, among other > things, is *because* there is a dangling pointer to the freed pipe, which > *caused* the crash in post_one_notification() when it tried to access > &pipe->rd.wait_lock as an argument to spin_lock_irq(), a path it reached > after checking if wqueue->pipe is NULL and proceeded when it was not the case. > > And the upstream commit was made *after* I had posted this patch, hence this > was a fix for the syzkaller issue. While I am *not* saying to accept it just > because this was posted earlier, I am saying this patch addresses a parallel > issue, i.e. the *actual use-after-free crash* which was reproduced by those > reproducers, i.e., what was attempted to be used after getting freed and > detected by KASAN. Even if wqueue->pipe was set to NULL during free_pipe_info(), there would still have been a use-after-free, as the real bug was the lack of synchronization between post_one_notification() and free_pipe_info(). That is fixed now. > > We don't need to wait for another similar syzbot report to pop up before doing > this change, and say let's not fix a dangling pointer reference because now > another commit apparately fixes the specific syzkaller issue, causing the given > specific reproducer with its specific way of reproducing to fail, when we in > fact now know it *can* be a valid problem in practice and doing this change > too causes the specific reproducer under consideration to fail reproducing, as > was reported by the reproducer itself. To re-iterate, I encourage you to send a cleanup patch if you see an opportunity. It looks like the state wqueue->defunct==true could be replaced with wqueue->pipe==NULL, which would be simpler, so how about doing that? Just don't claim that it is "fixing" something, unless it is, as that makes things very confusing and difficult for everyone. > > I really don't know how to create stress tests / reproducers like how syzkaller > makes, so if a similar new reproducer is really required for showing this > patch's validity disregarding any earlier reproducers, I unfortunately cannot > make it due to skill issue as I just started in kernel dev, and I am deeply > sorry for wasting the time of everyone, and I am thankful for your criticism of > my patch. A reproducer can just be written as a normal program, in C or another language. The syzkaller reproducers are really hard to read as they are auto-generated, so don't read too much into them -- they're certainly not examples of good code. - Eric _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-03 18:15 ` Eric Biggers 0 siblings, 0 replies; 36+ messages in thread From: Eric Biggers @ 2022-08-03 18:15 UTC (permalink / raw) To: Siddh Raman Pant Cc: David Howells, Christophe JAILLET, Eric Dumazet, Fabio M. De Francesco, linux-security-modules, linux-kernel, linux-kernel-mentees, syzbot+c70d87ac1d001f29a058 On Wed, Aug 03, 2022 at 02:10:06PM +0530, Siddh Raman Pant wrote: > On Wed, 03 Aug 2022 11:11:31 +0530 Eric Biggers <ebiggers@kernel.org> wrote: > > I tested the syzbot reproducer > > https://syzkaller.appspot.com/text?tag=ReproC&x=174ea97e080000, and it does > > *not* trigger the bug on the latest upstream. But, it does trigger the bug if I > > recent Linus's recent watch_queue fixes. > > > > So I don't currently see any evidence of an unfixed bug. If, nevertheless, you > > believe that a bug is still unfixed, then please provide a reproducer and a fix > > patch that clearly explains what it is fixing. > > > > > There is a null check in post_one_notification for the pipe, most probably > > > because it *expects* the pointer to be NULL'd. Also, there is no reason to have > > > a dangling pointer stay, it's just a recipe for further bugs. > > > > If you want to send a patch or patches to clean up the code, that is fine, but > > please make it super clear what is a cleanup and what is a fix. > > > > - Eric > > > > I honestly feel like I am repeating myself yet again, but okay. Well, you should try listening instead. Because you are not listening. > Of course, the race condition has been solved by a patch upstream, which I had > myself mentioned earlier. > > But what I am saying is that it did *not* address *what* that race condition > had triggered, i.e. the visible cause of the UAF crash, which, among other > things, is *because* there is a dangling pointer to the freed pipe, which > *caused* the crash in post_one_notification() when it tried to access > &pipe->rd.wait_lock as an argument to spin_lock_irq(), a path it reached > after checking if wqueue->pipe is NULL and proceeded when it was not the case. > > And the upstream commit was made *after* I had posted this patch, hence this > was a fix for the syzkaller issue. While I am *not* saying to accept it just > because this was posted earlier, I am saying this patch addresses a parallel > issue, i.e. the *actual use-after-free crash* which was reproduced by those > reproducers, i.e., what was attempted to be used after getting freed and > detected by KASAN. Even if wqueue->pipe was set to NULL during free_pipe_info(), there would still have been a use-after-free, as the real bug was the lack of synchronization between post_one_notification() and free_pipe_info(). That is fixed now. > > We don't need to wait for another similar syzbot report to pop up before doing > this change, and say let's not fix a dangling pointer reference because now > another commit apparately fixes the specific syzkaller issue, causing the given > specific reproducer with its specific way of reproducing to fail, when we in > fact now know it *can* be a valid problem in practice and doing this change > too causes the specific reproducer under consideration to fail reproducing, as > was reported by the reproducer itself. To re-iterate, I encourage you to send a cleanup patch if you see an opportunity. It looks like the state wqueue->defunct==true could be replaced with wqueue->pipe==NULL, which would be simpler, so how about doing that? Just don't claim that it is "fixing" something, unless it is, as that makes things very confusing and difficult for everyone. > > I really don't know how to create stress tests / reproducers like how syzkaller > makes, so if a similar new reproducer is really required for showing this > patch's validity disregarding any earlier reproducers, I unfortunately cannot > make it due to skill issue as I just started in kernel dev, and I am deeply > sorry for wasting the time of everyone, and I am thankful for your criticism of > my patch. A reproducer can just be written as a normal program, in C or another language. The syzkaller reproducers are really hard to read as they are auto-generated, so don't read too much into them -- they're certainly not examples of good code. - Eric ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue 2022-08-03 18:15 ` Eric Biggers @ 2022-08-04 8:39 ` Siddh Raman Pant via Linux-kernel-mentees -1 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant @ 2022-08-04 8:39 UTC (permalink / raw) To: Eric Biggers Cc: David Howells, Christophe JAILLET, Eric Dumazet, Fabio M. De Francesco, linux-security-modules, linux-kernel, linux-kernel-mentees, syzbot+c70d87ac1d001f29a058 On Wed, 03 Aug 2022 23:45:31 +0530 Eric Biggers wrote: > Well, you should try listening instead. Because you are not listening. Sorry for that, never meant to come across like that. > Even if wqueue->pipe was set to NULL during free_pipe_info(), there would still > have been a use-after-free, as the real bug was the lack of synchronization > between post_one_notification() and free_pipe_info(). That is fixed now. Okay, noted. > To re-iterate, I encourage you to send a cleanup patch if you see an > opportunity. It looks like the state wqueue->defunct==true could be replaced > with wqueue->pipe==NULL, which would be simpler, so how about doing that? Just > don't claim that it is "fixing" something, unless it is, as that makes things > very confusing and difficult for everyone. Okay, I will do that. That actually seems like a plausible thing to do, in v2 convo, David Howells had also remarked similarly about `defunct` to a reply. https://lore.kernel.org/linux-kernel/3565221.1658933355@warthog.procyon.org.uk/ > A reproducer can just be written as a normal program, in C or another language. > The syzkaller reproducers are really hard to read as they are auto-generated, so > don't read too much into them -- they're certainly not examples of good code. > > - Eric Okay, noted. Thanks for your patience, I probably annoyed you. Thanks, Siddh ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue @ 2022-08-04 8:39 ` Siddh Raman Pant via Linux-kernel-mentees 0 siblings, 0 replies; 36+ messages in thread From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-04 8:39 UTC (permalink / raw) To: Eric Biggers Cc: syzbot+c70d87ac1d001f29a058, linux-kernel-mentees, linux-security-modules, linux-kernel, David Howells, Eric Dumazet, Christophe JAILLET, Fabio M. De Francesco On Wed, 03 Aug 2022 23:45:31 +0530 Eric Biggers wrote: > Well, you should try listening instead. Because you are not listening. Sorry for that, never meant to come across like that. > Even if wqueue->pipe was set to NULL during free_pipe_info(), there would still > have been a use-after-free, as the real bug was the lack of synchronization > between post_one_notification() and free_pipe_info(). That is fixed now. Okay, noted. > To re-iterate, I encourage you to send a cleanup patch if you see an > opportunity. It looks like the state wqueue->defunct==true could be replaced > with wqueue->pipe==NULL, which would be simpler, so how about doing that? Just > don't claim that it is "fixing" something, unless it is, as that makes things > very confusing and difficult for everyone. Okay, I will do that. That actually seems like a plausible thing to do, in v2 convo, David Howells had also remarked similarly about `defunct` to a reply. https://lore.kernel.org/linux-kernel/3565221.1658933355@warthog.procyon.org.uk/ > A reproducer can just be written as a normal program, in C or another language. > The syzkaller reproducers are really hard to read as they are auto-generated, so > don't read too much into them -- they're certainly not examples of good code. > > - Eric Okay, noted. Thanks for your patience, I probably annoyed you. Thanks, Siddh _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2022-08-04 8:40 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-28 15:51 [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue Siddh Raman Pant 2022-07-28 15:51 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-01 9:34 ` Siddh Raman Pant 2022-08-01 9:34 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-01 10:24 ` Greg KH 2022-08-01 10:24 ` Greg KH 2022-08-01 12:58 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-01 12:58 ` Siddh Raman Pant 2022-08-01 13:00 ` Siddh Raman Pant 2022-08-01 13:00 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-01 16:16 ` Dipanjan Das 2022-08-01 16:16 ` Dipanjan Das 2022-08-01 18:49 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-01 18:49 ` Siddh Raman Pant 2022-08-03 1:16 ` Eric Biggers 2022-08-03 1:16 ` Eric Biggers 2022-08-03 3:58 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-03 3:58 ` Siddh Raman Pant 2022-08-03 4:10 ` Dipanjan Das 2022-08-03 4:10 ` Dipanjan Das 2022-08-03 1:08 ` Eric Biggers 2022-08-03 1:08 ` Eric Biggers 2022-08-03 3:56 ` Siddh Raman Pant 2022-08-03 3:56 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-03 4:12 ` Eric Biggers 2022-08-03 4:12 ` Eric Biggers 2022-08-03 5:13 ` Siddh Raman Pant 2022-08-03 5:13 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-03 5:41 ` Eric Biggers 2022-08-03 5:41 ` Eric Biggers 2022-08-03 8:40 ` Siddh Raman Pant 2022-08-03 8:40 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-03 18:15 ` Eric Biggers 2022-08-03 18:15 ` Eric Biggers 2022-08-04 8:39 ` Siddh Raman Pant 2022-08-04 8:39 ` Siddh Raman Pant via Linux-kernel-mentees
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.