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.
Bug report: https://syzkaller.appspot.com/bug?id=1870dd7791ba05f2ea7f47f7cbdde701173973fc
Reported-and-tested-by: [email protected]
Signed-off-by: Siddh Raman Pant <[email protected]>
---
kernel/watch_queue.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
index bb9962b33f95..bab9e09c74cf 100644
--- a/kernel/watch_queue.c
+++ b/kernel/watch_queue.c
@@ -637,8 +637,17 @@ void watch_queue_clear(struct watch_queue *wqueue)
spin_lock_bh(&wqueue->lock);
}
- spin_unlock_bh(&wqueue->lock);
rcu_read_unlock();
+
+ /* Clearing the watch queue, so we should clean the associated pipe. */
+#ifdef CONFIG_WATCH_QUEUE
+ if (wqueue->pipe) {
+ wqueue->pipe->watch_queue = NULL;
+ wqueue->pipe = NULL;
+ }
+#endif
+
+ spin_unlock_bh(&wqueue->lock);
}
/**
--
2.35.1
On Sat, Jul 23, 2022 at 07:24:47PM +0530, Siddh Raman Pant via Linux-kernel-mentees 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.
>
> Bug report: https://syzkaller.appspot.com/bug?id=1870dd7791ba05f2ea7f47f7cbdde701173973fc
> Reported-and-tested-by: [email protected]
>
> Signed-off-by: Siddh Raman Pant <[email protected]>
> ---
> kernel/watch_queue.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
> index bb9962b33f95..bab9e09c74cf 100644
> --- a/kernel/watch_queue.c
> +++ b/kernel/watch_queue.c
> @@ -637,8 +637,17 @@ void watch_queue_clear(struct watch_queue *wqueue)
> spin_lock_bh(&wqueue->lock);
> }
>
> - spin_unlock_bh(&wqueue->lock);
> rcu_read_unlock();
Also you now have a spinlock held when calling rcu_read_unlock(), are
you sure that's ok?
On Sat, 23 Jul 2022 19:34:17 +0530 Greg KH <[email protected]> wrote:
> Also you now have a spinlock held when calling rcu_read_unlock(), are
> you sure that's ok?
>
>
We logically should not do write operations in a read critical section, so the
nulling of `wqueue->pipe->watch_queue` should happen after rcu_read_unlock().
Also, since we already have a spinlock, we can use it to ensure the nulling.
So I think it is okay.
Though, it is my first time encountering a spinlock and an rcu lock together,
so if I am wrong, please do correct me.
Thanks,
Siddh
Siddh Raman Pant <[email protected]> wrote:
> +++ b/kernel/watch_queue.c
> ...
>+#ifdef CONFIG_WATCH_QUEUE
But it says:
obj-$(CONFIG_WATCH_QUEUE) += watch_queue.o
in the Makefile.
David
Siddh Raman Pant <[email protected]> wrote:
> Greg KH <[email protected]> wrote:
> > > - spin_unlock_bh(&wqueue->lock);
> > > rcu_read_unlock();
> >
> > Also you now have a spinlock held when calling rcu_read_unlock(), are
> > you sure that's ok?
Worse, we have softirqs disabled still, which might cause problems for
rcu_read_unlock()?
> We logically should not do write operations in a read critical section, so the
> nulling of `wqueue->pipe->watch_queue` should happen after rcu_read_unlock().
> Also, since we already have a spinlock, we can use it to ensure the nulling.
> So I think it is okay.
Read/write locks are perhaps misnamed in this sense; they perhaps should be
shared/exclusive. But, yes, we *can* do certain write operations with the
lock held - if we're careful. Locks are required if we need to pairs of
related memory accesses; if we're only making a single non-dependent write,
then we don't necessarily need a write lock.
However, you're referring to RCU read lock. That's a very special lock that
has to do with maintenance of persistence of objects without taking any other
lock. The moment you drop that lock, anything you accessed under RCU protocol
rules should be considered to have evaporated.
Think of it more as a way to have a deferred destructor/deallocator.
So I would do:
+
+ /* 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();
}
However, since you're now changing wqueue->pipe whilst a notification is being
posted, you need a barrier in post_one_notification() to prevent the compiler
from reloading the value:
struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe);
David
On Wed, 27 Jul 2022 19:45:42 +0530 David Howells <[email protected]> wrote:
> Siddh Raman Pant <[email protected]> wrote:
>
> > +++ b/kernel/watch_queue.c
> > ...
> >+#ifdef CONFIG_WATCH_QUEUE
>
> But it says:
>
> obj-$(CONFIG_WATCH_QUEUE) += watch_queue.o
>
> in the Makefile.
>
> David
>
>
Yes, that's what I realised and meant in reply to Khalid.
I had sent a v2, which you can find here:
https://lore.kernel.org/linux-kernel/[email protected]/
Thanks,
Siddh
On Wed, 27 Jul 2022 20:16:40 +0530 David Howells <[email protected]> wrote:
> Siddh Raman Pant <[email protected]> wrote:
>
> > Greg KH <[email protected]> wrote:
>
> > > > - spin_unlock_bh(&wqueue->lock);
> > > > rcu_read_unlock();
> > >
> > > Also you now have a spinlock held when calling rcu_read_unlock(), are
> > > you sure that's ok?
>
> Worse, we have softirqs disabled still, which might cause problems for
> rcu_read_unlock()?
>
> > We logically should not do write operations in a read critical section, so the
> > nulling of `wqueue->pipe->watch_queue` should happen after rcu_read_unlock().
> > Also, since we already have a spinlock, we can use it to ensure the nulling.
> > So I think it is okay.
>
> Read/write locks are perhaps misnamed in this sense; they perhaps should be
> shared/exclusive. But, yes, we *can* do certain write operations with the
> lock held - if we're careful. Locks are required if we need to pairs of
> related memory accesses; if we're only making a single non-dependent write,
> then we don't necessarily need a write lock.
>
> However, you're referring to RCU read lock. That's a very special lock that
> has to do with maintenance of persistence of objects without taking any other
> lock. The moment you drop that lock, anything you accessed under RCU protocol
> rules should be considered to have evaporated.
>
> Think of it more as a way to have a deferred destructor/deallocator.
>
> So I would do:
>
> +
> + /* 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();
> }
>
> However, since you're now changing wqueue->pipe whilst a notification is being
> posted, you need a barrier in post_one_notification() to prevent the compiler
> from reloading the value:
>
> struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe);
>
> David
>
Thank you for explaining it!
I will send a v3. Should I add a Suggested-by tag mentioning you?
Thanks,
Siddh
On Wed, Jul 27, 2022 at 09:50:52PM +0530, Siddh Raman Pant wrote:
> Thank you for explaining it!
>
> I will send a v3. Should I add a Suggested-by tag mentioning you?
Sorry for jumping in.
We have reported the same bug in kernel v5.10.131 [https://lore.kernel.org/all/CANX2M5bHye2ZEEhEV6PUj1kYL2KdWYeJtgXw8KZRzwrNpLYz+A@mail.gmail.com]. We have been suggested to join this discussion so that we can have appropriate meta-information injected in this patch’s commit message to make sure that it gets backported to v5.10.y. Therefore, we would like to be in the loop so that we can offer help in the process, if needed.
On Sun, 31 Jul 2022 23:41:31 +0530 Dipanjan Das <[email protected]> wrote:
> On Wed, Jul 27, 2022 at 09:50:52PM +0530, Siddh Raman Pant wrote:
> > Thank you for explaining it!
> >
> > I will send a v3. Should I add a Suggested-by tag mentioning you?
>
> Sorry for jumping in.
>
> We have reported the same bug in kernel v5.10.131 [https://lore.kernel.org/all/CANX2M5bHye2ZEEhEV6PUj1kYL2KdWYeJtgXw8KZRzwrNpLYz+A@mail.gmail.com]. We have been suggested to join this discussion so that we can have appropriate meta-information injected in this patch’s commit message to make sure that it gets backported to v5.10.y. Therefore, we would like to be in the loop so that we can offer help in the process, if needed.
>
As you are suggesting for backporting, I should CC the stable list, or mail
after it gets merged. You have reproduced it on v5.10, but the change seems to
be introduced by c73be61cede5 ("pipe: Add general notification queue support"),
which got in at v5.8. So should it be backported till v5.8 instead?
I actually looked this up on the internet / lore now for any other reports, and
it seems this fixes a CVE (CVE-2022-1882).
The reporter of CVE seems to have linked his patch as a part of CVE report, of
which he sent v2, but he seems to do it in a roundabout way, and also in a way
similar to what Hillf Danton had replied to my v2 patch, wherein he missed
353f7988dd84 ("watchqueue: make sure to serialize 'wqueue->defunct' properly"),
so I guess I can propose my patch as a fix for the CVE.
Note: I have already sent the v3, so please suggest any new improvements etc.
(except replying to the conversation here) to the v3, which can be found here:
https://lore.kernel.org/linux-kernel/[email protected]/
Also, you may want to break text into multiples lines instead of one huge line.
Thanks,
Siddh
On Mon, Aug 01, 2022 at 12:16:43AM +0530, Siddh Raman Pant wrote:
> On Sun, 31 Jul 2022 23:41:31 +0530 Dipanjan Das <[email protected]> wrote:
> > On Wed, Jul 27, 2022 at 09:50:52PM +0530, Siddh Raman Pant wrote:
> > > Thank you for explaining it!
> > >
> > > I will send a v3. Should I add a Suggested-by tag mentioning you?
> >
> > Sorry for jumping in.
> >
> > We have reported the same bug in kernel v5.10.131 [https://lore.kernel.org/all/CANX2M5bHye2ZEEhEV6PUj1kYL2KdWYeJtgXw8KZRzwrNpLYz+A@mail.gmail.com]. We have been suggested to join this discussion so that we can have appropriate meta-information injected in this patch’s commit message to make sure that it gets backported to v5.10.y. Therefore, we would like to be in the loop so that we can offer help in the process, if needed.
> >
>
> As you are suggesting for backporting, I should CC the stable list, or mail
> after it gets merged. You have reproduced it on v5.10, but the change seems to
> be introduced by c73be61cede5 ("pipe: Add general notification queue support"),
> which got in at v5.8. So should it be backported till v5.8 instead?
There are no active supported kernels other than the ones listed on
kernel.org, so 5.8 doesn't make much sense here, only 5.10 and 5.15 and
5.18 at the moment.
thanks,
greg k-h
On Mon, 01 Aug 2022 14:17:23 +0530 Greg KH <[email protected]> wrote:
> There are no active supported kernels other than the ones listed on
> kernel.org, so 5.8 doesn't make much sense here, only 5.10 and 5.15 and
> 5.18 at the moment.
>
> thanks,
>
> greg k-h
Okay, thanks for correcting me.
-- Siddh
On Mon, 01 Aug 2022 17:45:13 +0530 Hillf Danton <[email protected]> wrote:
> What is not clear is what you are fixing, with CVE-2022-1882 put aside,
> given the mainline tree survived the syzbot test [1] irrespective of
> other fixing efforts [2, 3].
>
> Hillf
>
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> // syzbot has tested the proposed patch and the reproducer did not trigger any issue:
> //
> // Reported-and-tested-by: [email protected]
> //
> // Tested on:
> //
> // commit: 3d7cb6b0 Linux 5.19
> // git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> // console output: https://syzkaller.appspot.com/x/log.txt?x=14066d7a080000
> // kernel config: https://syzkaller.appspot.com/x/.config?x=70dd99d568a89e0
> // dashboard link: https://syzkaller.appspot.com/bug?extid=c70d87ac1d001f29a058
> // compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> //
> // Note: no patches were applied.
> // Note: testing is done by a robot and is best-effort only.
>
> [2] https://lore.kernel.org/lkml/[email protected]/
>
> [3] https://lore.kernel.org/lkml/[email protected]/
>
(Fixed broken formatting)
This bug is about watch_queue still having a reference to a freed pipe,
which was being accessed by post_one_notification() at the time of when
I posted the v1 patch for fixing it on 23rd July, by removing the
reference to the freed pipe in the watch_queue.
Given ref. [3] by you leads to a bug about UAF in __post_watch_notification():
https://syzkaller.appspot.com/bug?extid=03d7b43290037d1f87ca
That bug is fixed by the following commit by David Howells on 28th July:
e64ab2dbd882 ("watch_queue: Fix missing locking in add_watch_to_object()")
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e64ab2dbd882933b65cd82ff6235d705ad65dbb6
Given ref. [2] by you is of a patch tested by you, which can be found below:
https://groups.google.com/g/syzkaller-bugs/c/RbmAFTAIuyY/m/-vMjf-BXAQAJ
This had overlooked the existing serialization of wqueue->defunct, which
you had yourself pointed out in the reply to v2, which can be found below:
https://lore.kernel.org/linux-kernel-mentees/[email protected]/
Given ref. [1] by you is about a syzbot test which was ran today, which no
longer triggers the issue. This probably happens due to the commit by David
Howells referenced earlier by me. While it does cause the reproducer to fail,
it doesn't really fix the particular issue concerned by this patch, which is
that the watch_queue has a reference to a freed pipe, which had caused a UAF.
Hope everything is clear.
Thanks,
Siddh
I don't know why you would send this again, this
was already replied here:
https://lore.kernel.org/linux-kernel-mentees/[email protected]/
Thanks,
Siddh