fs/pipe: Deinitialize the watch_queue when pipe is freed
PATCH v2 1/1 fs/pipe: Deinitialize the watch_queue when pipe is freed
From: Haimin Zhang <[email protected]>
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.
---
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(-)
fs/pipe.c +3 -1
@@ -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);
include/linux/watch_queue.h +5 -0
@@ -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 */
kernel/watch_queue.c +30 -4
@@ -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 ; +}
Signature · 1 line
2.27.0
Quoted 10 lines · Haimin Zhang
From: Haimin Zhang <[email protected]>
Add a new function call to deinitialize the
watch_queueof a freed pipe. When a pipe node is freed, it doesn't makepipe->watch_queue->pipenull. Later when functionpost_one_notificationis called, it will use this field, but it has been freed andwatch_queue->pipeis a dangling pointer. It makes a uaf issue.
Checkwqueu->defunctbefore pipe check since pipe becomes invalid once all watch queues were cleared.
Quoted 2 lines
Reported-by TCS Robot <[email protected]>Signed-off-by Haimin Zhang <[email protected]>
Is this fixing something? If so it should have a "Fixes" tag.
- Eric
kernel test robot
build botThank you for the patch! Yet something to improve:
Hi 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/[email protected]/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
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.
Signature · 2 lines
0-DAY CI Kernel Test Service https://01.org/lkp
Quoted 11 lines · Eric Biggers
Quoted 10 lines · Haimin Zhang
From: Haimin Zhang <[email protected]>
Add a new function call to deinitialize the
watch_queueof a freed pipe. When a pipe node is freed, it doesn't makepipe->watch_queue->pipenull. Later when functionpost_one_notificationis called, it will use this field, but it has been freed andwatch_queue->pipeis a dangling pointer. It makes a uaf issue.
Checkwqueu->defunctbefore pipe check since pipe becomes invalid once all watch queues were cleared.
Quoted 4 lines
Quoted 2 lines
Reported-by TCS Robot <[email protected]>Signed-off-by Haimin Zhang <[email protected]>Is this fixing something? If so it should have a "Fixes" tag.
It sure is.
Haimin, are you planning a v3?
Signature · 4 lines
Lee Jones [李琼斯] Principal Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Quoted 13 lines · Lee Jones
Quoted 11 lines · Eric Biggers
Quoted 10 lines · Haimin Zhang
From: Haimin Zhang <[email protected]>
Add a new function call to deinitialize the
watch_queueof a freed pipe. When a pipe node is freed, it doesn't makepipe->watch_queue->pipenull. Later when functionpost_one_notificationis called, it will use this field, but it has been freed andwatch_queue->pipeis a dangling pointer. It makes a uaf issue.
Checkwqueu->defunctbefore pipe check since pipe becomes invalid once all watch queues were cleared.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Reported-by TCS Robot <[email protected]>Signed-off-by Haimin Zhang <[email protected]>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?
Signature · 4 lines
Lee Jones [李琼斯] Principal Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Quoted 22 lines · Lee Jones
Quoted 20 lines · Lee Jones
Quoted 11 lines · Eric Biggers
Quoted 10 lines · Haimin Zhang
From: Haimin Zhang <[email protected]>
Add a new function call to deinitialize the
watch_queueof a freed pipe. When a pipe node is freed, it doesn't makepipe->watch_queue->pipenull. Later when functionpost_one_notificationis called, it will use this field, but it has been freed andwatch_queue->pipeis a dangling pointer. It makes a uaf issue.
Checkwqueu->defunctbefore pipe check since pipe becomes invalid once all watch queues were cleared.Quoted 4 lines
Quoted 2 lines
Reported-by TCS Robot <[email protected]>Signed-off-by Haimin Zhang <[email protected]>Is this fixing something? If so it should have a "Fixes" tag.
It sure is.
Quoted 7 lines
Quoted 1 line
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 <[email protected]> Date: Tue Jul 19 11:09:01 2022 -0700 watchqueue: make sure to serialize 'wqueue->defunct' properly
- Eric
Quoted 31 lines · Eric Biggers
Quoted 22 lines · Lee Jones
Quoted 20 lines · Lee Jones
Quoted 11 lines · Eric Biggers
Quoted 10 lines · Haimin Zhang
From: Haimin Zhang <[email protected]>
Add a new function call to deinitialize the
watch_queueof a freed pipe. When a pipe node is freed, it doesn't makepipe->watch_queue->pipenull. Later when functionpost_one_notificationis called, it will use this field, but it has been freed andwatch_queue->pipeis a dangling pointer. It makes a uaf issue.
Checkwqueu->defunctbefore pipe check since pipe becomes invalid once all watch queues were cleared.Quoted 4 lines
Quoted 2 lines
Reported-by TCS Robot <[email protected]>Signed-off-by Haimin Zhang <[email protected]>Is this fixing something? If so it should have a "Fixes" tag.
It sure is.
Quoted 7 lines
Quoted 1 line
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?
Quoted 8 lines
I think this was already fixed (correctly, unlike the above patch which is very broken) by the following commit:
commit 353f7988dd8413c47718f7ca79c030b6fb62cfe5 Author: Linus Torvalds <[email protected]> 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.
Signature · 1 line
DEPRECATED: Please use [email protected]
Quoted 8 lines · Lee Jones
Quoted 5 lines
commit 353f7988dd8413c47718f7ca79c030b6fb62cfe5 Author: Linus Torvalds <[email protected]> Date: Tue Jul 19 11:09:01 2022 -0700 watchqueue: make sure to serialize 'wqueue->defunct' properlyThanks Eric, I'll back-port this one instead.
It's already in all LTS kernels that were affected (5.10 and later).
- Eric
Quoted 11 lines · Eric Biggers
Quoted 8 lines · Lee Jones
Quoted 5 lines
commit 353f7988dd8413c47718f7ca79c030b6fb62cfe5 Author: Linus Torvalds <[email protected]> Date: Tue Jul 19 11:09:01 2022 -0700 watchqueue: make sure to serialize 'wqueue->defunct' properlyThanks 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.
Signature · 1 line
DEPRECATED: Please use [email protected]