| Message ID | 20220509131726.59664-1-tcs.kernel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fs/pipe: Deinitialize the watch_queue when pipe is freed | expand |
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
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/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.
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?
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?
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.
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.
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; +}