fs/pipe.c | 4 +++- include/linux/watch_queue.h | 5 +++++ kernel/watch_queue.c | 34 ++++++++++++++++++++++++++++++---- 3 files changed, 38 insertions(+), 5 deletions(-)
From: Haimin Zhang <tcs_kernel@tencent.com>
Add a new function call to deinitialize the watch_queue of a freed pipe.
When a pipe node is freed, it doesn't make pipe->watch_queue->pipe null.
Later when function post_one_notification is called, it will use this
field, but it has been freed and watch_queue->pipe is a dangling pointer.
It makes a uaf issue.
Check wqueu->defunct before pipe check since pipe becomes invalid once all
watch queues were cleared.
Reported-by: TCS Robot <tcs_robot@tencent.com>
Signed-off-by: Haimin Zhang <tcs_kernel@tencent.com>
---
The following is the callstacks:
1. The pipe was created as follows:
```
kmalloc build/../include/linux/slab.h:581 [inline]
kzalloc build/../include/linux/slab.h:714 [inline]
alloc_pipe_info+0x105/0x590 build/../fs/pipe.c:790
get_pipe_inode build/../fs/pipe.c:881 [inline]
create_pipe_files+0x8d/0x880 build/../fs/pipe.c:913
__do_pipe_flags build/../fs/pipe.c:962 [inline]
do_pipe2+0x96/0x1b0 build/../fs/pipe.c:1010
__do_sys_pipe2 build/../fs/pipe.c:1028 [inline]
__se_sys_pipe2 build/../fs/pipe.c:1026 [inline]
__x64_sys_pipe2+0x50/0x70 build/../fs/pipe.c:1026
do_syscall_x64 build/../arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0x80 build/../arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
```
2. The pipe was freed as follows:
```
kfree+0xd6/0x4d0 build/../mm/slub.c:4552
put_pipe_info build/../fs/pipe.c:711 [inline]
pipe_release+0x2b6/0x310 build/../fs/pipe.c:734
__fput+0x277/0x9d0 build/../fs/file_table.c:317
task_work_run+0xdd/0x1a0 build/../kernel/task_work.c:164
resume_user_mode_work build/../include/linux/resume_user_mode.h: 49 [inline]
exit_to_user_mode_loop build/../kernel/entry/common.c:169 [inline]
exit_to_user_mode_prepare+0x23c/0x250 build/../kernel/entry/common.c:201
__syscall_exit_to_user_mode_work build/../kernel/entry/common.c:283 [inline]
syscall_exit_to_user_mode+0x19/0x60 build/../kernel/entry/common.c:294
do_syscall_64+0x42/0x80 build/../arch/x86/entry/common.c:86
entry_SYSCALL_64_after_hwframe+0x44/0xae
```
3. The dangling pointer was used:
```
__lock_acquire+0x3eb0/0x56c0 build/../kernel/locking/lockdep.c:4899
lock_acquire build/../kernel/locking/lockdep.c:5641 [inline]
lock_acquire+0x1ab/0x510 build/../kernel/locking/lockdep.c:5606
__raw_spin_lock_irq build/../include/linux/spinlock_api_smp.h:119 [inline]
_raw_spin_lock_irq+0x32/0x50 build/../kernel/locking/spinlock.c:170
spin_lock_irq build/../include/linux/spinlock.h:374 [inline]
post_one_notification.isra.0+0x59/0x990 build/../kernel/watch_queue.c:86
remove_watch_from_object+0x35a/0x9d0 build/../kernel/watch_queue.c:527
remove_watch_list build/../include/linux/watch_queue.h:115 [inline]
key_gc_unused_keys.constprop.0+0x2e5/0x600 build/../security/keys/gc.c:135
key_garbage_collector+0x3d7/0x920 build/../security/keys/gc.c:297
process_one_work+0x996/0x1610 build/../kernel/workqueue.c:2289
worker_thread+0x665/0x1080 build/../kernel/workqueue.c:2436
kthread+0x2e9/0x3a0 build/../kernel/kthread.c:376
ret_from_fork+0x1f/0x30 build/../arch/x86/entry/entry_64.S:298
```
fs/pipe.c | 4 +++-
include/linux/watch_queue.h | 5 +++++
kernel/watch_queue.c | 34 ++++++++++++++++++++++++++++++----
3 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/fs/pipe.c b/fs/pipe.c
index e140ea150bbb..d0e9d8697810 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -844,8 +844,10 @@ void free_pipe_info(struct pipe_inode_info *pipe)
pipe_buf_release(pipe, buf);
}
#ifdef CONFIG_WATCH_QUEUE
- if (pipe->watch_queue)
+ if (pipe->watch_queue) {
+ watch_queue_deinit(pipe);
put_watch_queue(pipe->watch_queue);
+ }
#endif
if (pipe->tmp_page)
__free_page(pipe->tmp_page);
diff --git a/include/linux/watch_queue.h b/include/linux/watch_queue.h
index 3b9a40ae8bdb..e5086b195fb7 100644
--- a/include/linux/watch_queue.h
+++ b/include/linux/watch_queue.h
@@ -90,6 +90,7 @@ extern long watch_queue_set_size(struct pipe_inode_info *, unsigned int);
extern long watch_queue_set_filter(struct pipe_inode_info *,
struct watch_notification_filter __user *);
extern int watch_queue_init(struct pipe_inode_info *);
+extern int watch_queue_deinit(struct pipe_inode_info *);
extern void watch_queue_clear(struct watch_queue *);
static inline void init_watch_list(struct watch_list *wlist,
@@ -129,6 +130,10 @@ static inline int watch_queue_init(struct pipe_inode_info *pipe)
return -ENOPKG;
}
+static inline int watch_queue_deinit(struct pipe_inode_info *pipe)
+{
+ return -ENOPKG;
+}
#endif
#endif /* _LINUX_WATCH_QUEUE_H */
diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
index 230038d4f908..4357511880f5 100644
--- a/kernel/watch_queue.c
+++ b/kernel/watch_queue.c
@@ -80,13 +80,22 @@ static bool post_one_notification(struct watch_queue *wqueue,
unsigned int head, tail, mask, note, offset, len;
bool done = false;
- if (!pipe)
+ if (!kref_get_unless_zero(&wqueue->usage))
return false;
- spin_lock_irq(&pipe->rd_wait.lock);
-
- if (wqueue->defunct)
+ spin_lock_bh(&wqueue->lock);
+ if (wqueue->defunct) {
+ spin_unlock_bh(&wqueue->lock);
goto out;
+ }
+ spin_unlock_bh(&wqueue->lock);
+
+ if (!pipe) {
+ put_watch_queue(wqueue);
+ return false;
+ }
+
+ spin_lock_irq(&pipe->rd_wait.lock);
mask = pipe->ring_size - 1;
head = pipe->head;
@@ -123,6 +132,7 @@ static bool post_one_notification(struct watch_queue *wqueue,
done = true;
out:
+ put_watch_queue(wqueue);
spin_unlock_irq(&pipe->rd_wait.lock);
if (done)
kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
@@ -663,3 +673,19 @@ int watch_queue_init(struct pipe_inode_info *pipe)
pipe->watch_queue = wqueue;
return 0;
}
+
+/*
+ * Deinitialise a watch queue
+ */
+int watch_queue_deinit(struct pipe_inode_info *pipe)
+{
+ struct watch_queue *wqueue;
+
+ if (pipe) {
+ wqueue = pipe->watch_queue;
+ if (wqueue)
+ wqueue->pipe = NULL;
+ pipe->watch_queue = NULL;
+ }
+ return 0;
+}
--
2.27.0Hi Haimin,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.18-rc6 next-20220509]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Haimin-Zhang/fs-pipe-Deinitialize-the-watch_queue-when-pipe-is-freed/20220509-212415
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git c5eb0a61238dd6faf37f58c9ce61c9980aaffd7a
config: s390-randconfig-r004-20220509 (https://download.01.org/0day-ci/archive/20220510/202205100814.M4Aiy8hF-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 3abb68a626160e019c30a4860e569d7bc75e486a)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/22e9d0a2c66d49444376e55348c8a6fa26e6d150
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Haimin-Zhang/fs-pipe-Deinitialize-the-watch_queue-when-pipe-is-freed/20220509-212415
git checkout 22e9d0a2c66d49444376e55348c8a6fa26e6d150
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
<inline asm>:5:5: error: expected absolute expression
.if 6651b-6641b > 254
^
<inline asm>:6:2: error: cpu alternatives does not support instructions blocks > 254 bytes
.error "cpu alternatives does not support instructions blocks > 254 bytes"
^
<inline asm>:8:5: error: expected absolute expression
.if (6651b-6641b) % 2
^
<inline asm>:9:2: error: cpu alternatives instructions length is odd
.error "cpu alternatives instructions length is odd"
^
<inline asm>:15:5: error: expected absolute expression
.if -(((6651b-6641b)-(662b-661b)) > 0) * ((6651b-6641b)-(662b-661b)) > 6
^
>> <inline asm>:18:8: error: unexpected token in '.rept' directive
.rept (-(((6651b-6641b)-(662b-661b)) > 0) * ((6651b-6641b)-(662b-661b)) - (6620b-662b)) / 2
^
<inline asm>:21:8: error: unexpected token in '.rept' directive
.rept -(((6651b-6641b)-(662b-661b)) > 0) * ((6651b-6641b)-(662b-661b)) / 6
^
>> <inline asm>:22:2: error: unknown directive
.brcl 0,0
^
>> <inline asm>:23:7: error: unmatched '.endr' directive
.endr
^
<inline asm>:24:8: error: unexpected token in '.rept' directive
.rept -(((6651b-6641b)-(662b-661b)) > 0) * ((6651b-6641b)-(662b-661b)) % 6 / 4
^
<inline asm>:26:7: error: unmatched '.endr' directive
.endr
^
<inline asm>:27:8: error: unexpected token in '.rept' directive
.rept -(((6651b-6641b)-(662b-661b)) > 0) * ((6651b-6641b)-(662b-661b)) % 6 % 4 / 2
^
<inline asm>:29:6: error: unmatched '.endr' directive
.endr
^
<inline asm>:32:5: error: expected absolute expression
.if 662b-661b > 254
^
<inline asm>:33:2: error: cpu alternatives does not support instructions blocks > 254 bytes
.error "cpu alternatives does not support instructions blocks > 254 bytes"
^
<inline asm>:35:5: error: expected absolute expression
.if (662b-661b) % 2
^
<inline asm>:36:2: error: cpu alternatives instructions length is odd
.error "cpu alternatives instructions length is odd"
^
In file included from kernel/watch_queue.c:11:
In file included from include/linux/module.h:14:
In file included from include/linux/buildid.h:5:
In file included from include/linux/mm_types.h:8:
In file included from include/linux/kref.h:16:
In file included from include/linux/spinlock.h:93:
arch/s390/include/asm/spinlock.h:81:3: error: expected absolute expression
ALTERNATIVE("", ".insn rre,0xb2fa0000,7,0", 49) /* NIAI 7 */
^
arch/s390/include/asm/alternative.h:118:2: note: expanded from macro 'ALTERNATIVE'
ALTINSTR_REPLACEMENT(altinstr, 1) \
^
arch/s390/include/asm/alternative.h:113:2: note: expanded from macro 'ALTINSTR_REPLACEMENT'
INSTR_LEN_SANITY_CHECK(altinstr_len(num))
^
arch/s390/include/asm/alternative.h:62:3: note: expanded from macro 'INSTR_LEN_SANITY_CHECK'
".if " len " > 254\n" \
^
<inline asm>:5:5: note: instantiated into assembly here
.if 6651b-6641b > 254
^
In file included from kernel/watch_queue.c:11:
In file included from include/linux/module.h:14:
In file included from include/linux/buildid.h:5:
In file included from include/linux/mm_types.h:8:
In file included from include/linux/kref.h:16:
In file included from include/linux/spinlock.h:93:
arch/s390/include/asm/spinlock.h:81:3: error: cpu alternatives does not support instructions blocks > 254 bytes
ALTERNATIVE("", ".insn rre,0xb2fa0000,7,0", 49) /* NIAI 7 */
^
arch/s390/include/asm/alternative.h:118:2: note: expanded from macro 'ALTERNATIVE'
ALTINSTR_REPLACEMENT(altinstr, 1) \
^
arch/s390/include/asm/alternative.h:113:2: note: expanded from macro 'ALTINSTR_REPLACEMENT'
INSTR_LEN_SANITY_CHECK(altinstr_len(num))
^
arch/s390/include/asm/alternative.h:63:3: note: expanded from macro 'INSTR_LEN_SANITY_CHECK'
"\t.error \"cpu alternatives does not support instructions " \
^
<inline asm>:6:2: note: instantiated into assembly here
.error "cpu alternatives does not support instructions blocks > 254 bytes"
^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Mon, May 09, 2022 at 09:17:26PM +0800, Haimin Zhang wrote: > From: Haimin Zhang <tcs_kernel@tencent.com> > > Add a new function call to deinitialize the watch_queue of a freed pipe. > When a pipe node is freed, it doesn't make pipe->watch_queue->pipe null. > Later when function post_one_notification is called, it will use this > field, but it has been freed and watch_queue->pipe is a dangling pointer. > It makes a uaf issue. > Check wqueu->defunct before pipe check since pipe becomes invalid once all > watch queues were cleared. > > Reported-by: TCS Robot <tcs_robot@tencent.com> > Signed-off-by: Haimin Zhang <tcs_kernel@tencent.com> Is this fixing something? If so it should have a "Fixes" tag. - Eric
On Mon, 09 May 2022, Eric Biggers wrote: > On Mon, May 09, 2022 at 09:17:26PM +0800, Haimin Zhang wrote: > > From: Haimin Zhang <tcs_kernel@tencent.com> > > > > Add a new function call to deinitialize the watch_queue of a freed pipe. > > When a pipe node is freed, it doesn't make pipe->watch_queue->pipe null. > > Later when function post_one_notification is called, it will use this > > field, but it has been freed and watch_queue->pipe is a dangling pointer. > > It makes a uaf issue. > > Check wqueu->defunct before pipe check since pipe becomes invalid once all > > watch queues were cleared. > > > > Reported-by: TCS Robot <tcs_robot@tencent.com> > > Signed-off-by: Haimin Zhang <tcs_kernel@tencent.com> > > Is this fixing something? If so it should have a "Fixes" tag. It sure is. Haimin, are you planning a v3? -- Lee Jones [李琼斯] Principal Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
On Wed, 06 Jul 2022, Lee Jones wrote: > On Mon, 09 May 2022, Eric Biggers wrote: > > > On Mon, May 09, 2022 at 09:17:26PM +0800, Haimin Zhang wrote: > > > From: Haimin Zhang <tcs_kernel@tencent.com> > > > > > > Add a new function call to deinitialize the watch_queue of a freed pipe. > > > When a pipe node is freed, it doesn't make pipe->watch_queue->pipe null. > > > Later when function post_one_notification is called, it will use this > > > field, but it has been freed and watch_queue->pipe is a dangling pointer. > > > It makes a uaf issue. > > > Check wqueu->defunct before pipe check since pipe becomes invalid once all > > > watch queues were cleared. > > > > > > Reported-by: TCS Robot <tcs_robot@tencent.com> > > > Signed-off-by: Haimin Zhang <tcs_kernel@tencent.com> > > > > Is this fixing something? If so it should have a "Fixes" tag. > > It sure is. > > Haimin, are you planning a v3? This patch is set to fix a pretty public / important bug. Has there been any more activity that I may have missed? Perhaps it's been superseded? -- Lee Jones [李琼斯] Principal Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
On Tue, Jul 19, 2022 at 03:04:41PM +0100, Lee Jones wrote: > On Wed, 06 Jul 2022, Lee Jones wrote: > > > On Mon, 09 May 2022, Eric Biggers wrote: > > > > > On Mon, May 09, 2022 at 09:17:26PM +0800, Haimin Zhang wrote: > > > > From: Haimin Zhang <tcs_kernel@tencent.com> > > > > > > > > Add a new function call to deinitialize the watch_queue of a freed pipe. > > > > When a pipe node is freed, it doesn't make pipe->watch_queue->pipe null. > > > > Later when function post_one_notification is called, it will use this > > > > field, but it has been freed and watch_queue->pipe is a dangling pointer. > > > > It makes a uaf issue. > > > > Check wqueu->defunct before pipe check since pipe becomes invalid once all > > > > watch queues were cleared. > > > > > > > > Reported-by: TCS Robot <tcs_robot@tencent.com> > > > > Signed-off-by: Haimin Zhang <tcs_kernel@tencent.com> > > > > > > Is this fixing something? If so it should have a "Fixes" tag. > > > > It sure is. > > > > Haimin, are you planning a v3? > > This patch is set to fix a pretty public / important bug. > > Has there been any more activity that I may have missed? > > Perhaps it's been superseded? I think this was already fixed (correctly, unlike the above patch which is very broken) 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 - Eric
On Tue, 02 Aug 2022, Eric Biggers wrote: > On Tue, Jul 19, 2022 at 03:04:41PM +0100, Lee Jones wrote: > > On Wed, 06 Jul 2022, Lee Jones wrote: > > > > > On Mon, 09 May 2022, Eric Biggers wrote: > > > > > > > On Mon, May 09, 2022 at 09:17:26PM +0800, Haimin Zhang wrote: > > > > > From: Haimin Zhang <tcs_kernel@tencent.com> > > > > > > > > > > Add a new function call to deinitialize the watch_queue of a freed pipe. > > > > > When a pipe node is freed, it doesn't make pipe->watch_queue->pipe null. > > > > > Later when function post_one_notification is called, it will use this > > > > > field, but it has been freed and watch_queue->pipe is a dangling pointer. > > > > > It makes a uaf issue. > > > > > Check wqueu->defunct before pipe check since pipe becomes invalid once all > > > > > watch queues were cleared. > > > > > > > > > > Reported-by: TCS Robot <tcs_robot@tencent.com> > > > > > Signed-off-by: Haimin Zhang <tcs_kernel@tencent.com> > > > > > > > > Is this fixing something? If so it should have a "Fixes" tag. > > > > > > It sure is. > > > > > > Haimin, are you planning a v3? > > > > This patch is set to fix a pretty public / important bug. > > > > Has there been any more activity that I may have missed? > > > > Perhaps it's been superseded? > > I think this was already fixed (correctly, unlike the above patch which is very > broken) 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 Thanks Eric, I'll back-port this one instead. -- DEPRECATED: Please use lee@kernel.org
On Tue, Aug 09, 2022 at 04:38:07PM +0100, Lee Jones wrote: > > 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 > > Thanks Eric, I'll back-port this one instead. > It's already in all LTS kernels that were affected (5.10 and later). - Eric
On Tue, 09 Aug 2022, Eric Biggers wrote: > On Tue, Aug 09, 2022 at 04:38:07PM +0100, Lee Jones wrote: > > > 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 > > > > Thanks Eric, I'll back-port this one instead. > > > > It's already in all LTS kernels that were affected (5.10 and later). Right, but it's missing from a bunch of ACKs. I'll sort it. Thanks for all your help. -- DEPRECATED: Please use lee@kernel.org
© 2016 - 2026 Red Hat, Inc.