From: Fedor Pchelkin <[email protected]>
Syzkaller reports a memory leak:
BUG: memory leak
unreferenced object 0xffff88810b279e00 (size 96):
comm "syz-executor399", pid 3631, jiffies 4294964921 (age 23.870s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 08 9e 27 0b 81 88 ff ff ..........'.....
08 9e 27 0b 81 88 ff ff 00 00 00 00 00 00 00 00 ..'.............
backtrace:
[<ffffffff814cfc90>] kmalloc_trace+0x20/0x90 mm/slab_common.c:1046
[<ffffffff81bb75ca>] kmalloc include/linux/slab.h:576 [inline]
[<ffffffff81bb75ca>] autofs_wait+0x3fa/0x9a0 fs/autofs/waitq.c:378
[<ffffffff81bb88a7>] autofs_do_expire_multi+0xa7/0x3e0 fs/autofs/expire.c:593
[<ffffffff81bb8c33>] autofs_expire_multi+0x53/0x80 fs/autofs/expire.c:619
[<ffffffff81bb6972>] autofs_root_ioctl_unlocked+0x322/0x3b0 fs/autofs/root.c:897
[<ffffffff81bb6a95>] autofs_root_ioctl+0x25/0x30 fs/autofs/root.c:910
[<ffffffff81602a9c>] vfs_ioctl fs/ioctl.c:51 [inline]
[<ffffffff81602a9c>] __do_sys_ioctl fs/ioctl.c:870 [inline]
[<ffffffff81602a9c>] __se_sys_ioctl fs/ioctl.c:856 [inline]
[<ffffffff81602a9c>] __x64_sys_ioctl+0xfc/0x140 fs/ioctl.c:856
[<ffffffff84608225>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
[<ffffffff84608225>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
[<ffffffff84800087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
autofs_wait_queue structs should be freed if their wait_ctr becomes zero.
Otherwise they will be lost.
In this case an AUTOFS_IOC_EXPIRE_MULTI ioctl is done, then a new
waitqueue struct is allocated in autofs_wait(), its initial wait_ctr
equals 2. After that wait_event_killable() is interrupted (it returns
-ERESTARTSYS), so that 'wq->name.name == NULL' condition may be not
satisfied. Actually, this condition can be satisfied when
autofs_wait_release() or autofs_catatonic_mode() is called and, what is
also important, wait_ctr is decremented in those places. Upon the exit of
autofs_wait(), wait_ctr is decremented to 1. Then the unmounting process
begins: kill_sb calls autofs_catatonic_mode(), which should have freed the
waitqueues, but it only decrements its usage counter to zero which is not
a correct behaviour.
edit:imk
This description is of course not correct. The umount performed as a result
of an expire is a umount of a mount that has been automounted, it's not the
autofs mount itself. They happen independently, usually after everything
mounted within the autofs file system has been expired away. If everything
hasn't been expired away the automount daemon can still exit leaving mounts
in place. But expires done in both cases will result in a notification that
calls autofs_wait_release() with a result status. The problem case is the
summary execution of of the automount daemon. In this case any waiting
processes won't be woken up until either they are terminated or the mount
is umounted.
end edit: imk
So in catatonic mode we should free waitqueues which counter becomes zero.
edit: imk
Initially I was concerned that the calling of autofs_wait_release() and
autofs_catatonic_mode() was not mutually exclusive but that can't be the
case (obviously) because the queue entry (or entries) is removed from the
list when either of these two functions are called. Consequently the wait
entry will be freed by only one of these functions or by the woken process
in autofs_wait() depending on the order of the calls.
end edit: imk
Reported-by: [email protected]
Suggested-by: Takeshi Misawa <[email protected]>
Signed-off-by: Fedor Pchelkin <[email protected]>
Signed-off-by: Alexey Khoroshilov <[email protected]>
Signed-off-by: Ian Kent <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Andrei Vagin <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
fs/autofs/waitq.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/autofs/waitq.c b/fs/autofs/waitq.c
index 54c1f8b8b075..efdc76732fae 100644
--- a/fs/autofs/waitq.c
+++ b/fs/autofs/waitq.c
@@ -32,8 +32,9 @@ void autofs_catatonic_mode(struct autofs_sb_info *sbi)
wq->status = -ENOENT; /* Magic is gone - report failure */
kfree(wq->name.name - wq->offset);
wq->name.name = NULL;
- wq->wait_ctr--;
wake_up_interruptible(&wq->queue);
+ if (!--wq->wait_ctr)
+ kfree(wq);
wq = nwq;
}
fput(sbi->pipe); /* Close the pipe */
In autofs_wait_release() wake_up() is used to wake up processes waiting
on a mount callback to complete which matches the wait_event_killable()
in autofs_wait().
But in autofs_catatonic_mode() the wake_up_interruptible() was not also
changed at the time autofs_wait_release() was changed.
Signed-off-by: Ian Kent <[email protected]>
---
fs/autofs/waitq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/autofs/waitq.c b/fs/autofs/waitq.c
index efdc76732fae..33dd4660d82f 100644
--- a/fs/autofs/waitq.c
+++ b/fs/autofs/waitq.c
@@ -32,7 +32,7 @@ void autofs_catatonic_mode(struct autofs_sb_info *sbi)
wq->status = -ENOENT; /* Magic is gone - report failure */
kfree(wq->name.name - wq->offset);
wq->name.name = NULL;
- wake_up_interruptible(&wq->queue);
+ wake_up(&wq->queue);
if (!--wq->wait_ctr)
kfree(wq);
wq = nwq;
On Fri, Aug 04, 2023 at 01:33:12PM +0800, Ian Kent wrote:
> From: Fedor Pchelkin <[email protected]>
>
> Syzkaller reports a memory leak:
>
> BUG: memory leak
> unreferenced object 0xffff88810b279e00 (size 96):
> comm "syz-executor399", pid 3631, jiffies 4294964921 (age 23.870s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 08 9e 27 0b 81 88 ff ff ..........'.....
> 08 9e 27 0b 81 88 ff ff 00 00 00 00 00 00 00 00 ..'.............
> backtrace:
> [<ffffffff814cfc90>] kmalloc_trace+0x20/0x90 mm/slab_common.c:1046
> [<ffffffff81bb75ca>] kmalloc include/linux/slab.h:576 [inline]
> [<ffffffff81bb75ca>] autofs_wait+0x3fa/0x9a0 fs/autofs/waitq.c:378
> [<ffffffff81bb88a7>] autofs_do_expire_multi+0xa7/0x3e0 fs/autofs/expire.c:593
> [<ffffffff81bb8c33>] autofs_expire_multi+0x53/0x80 fs/autofs/expire.c:619
> [<ffffffff81bb6972>] autofs_root_ioctl_unlocked+0x322/0x3b0 fs/autofs/root.c:897
> [<ffffffff81bb6a95>] autofs_root_ioctl+0x25/0x30 fs/autofs/root.c:910
> [<ffffffff81602a9c>] vfs_ioctl fs/ioctl.c:51 [inline]
> [<ffffffff81602a9c>] __do_sys_ioctl fs/ioctl.c:870 [inline]
> [<ffffffff81602a9c>] __se_sys_ioctl fs/ioctl.c:856 [inline]
> [<ffffffff81602a9c>] __x64_sys_ioctl+0xfc/0x140 fs/ioctl.c:856
> [<ffffffff84608225>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> [<ffffffff84608225>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> [<ffffffff84800087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> autofs_wait_queue structs should be freed if their wait_ctr becomes zero.
> Otherwise they will be lost.
>
> In this case an AUTOFS_IOC_EXPIRE_MULTI ioctl is done, then a new
> waitqueue struct is allocated in autofs_wait(), its initial wait_ctr
> equals 2. After that wait_event_killable() is interrupted (it returns
> -ERESTARTSYS), so that 'wq->name.name == NULL' condition may be not
> satisfied. Actually, this condition can be satisfied when
> autofs_wait_release() or autofs_catatonic_mode() is called and, what is
> also important, wait_ctr is decremented in those places. Upon the exit of
> autofs_wait(), wait_ctr is decremented to 1. Then the unmounting process
> begins: kill_sb calls autofs_catatonic_mode(), which should have freed the
> waitqueues, but it only decrements its usage counter to zero which is not
> a correct behaviour.
>
> edit:imk
> This description is of course not correct. The umount performed as a result
> of an expire is a umount of a mount that has been automounted, it's not the
> autofs mount itself. They happen independently, usually after everything
> mounted within the autofs file system has been expired away. If everything
> hasn't been expired away the automount daemon can still exit leaving mounts
> in place. But expires done in both cases will result in a notification that
> calls autofs_wait_release() with a result status. The problem case is the
> summary execution of of the automount daemon. In this case any waiting
> processes won't be woken up until either they are terminated or the mount
> is umounted.
> end edit: imk
>
> So in catatonic mode we should free waitqueues which counter becomes zero.
>
> edit: imk
> Initially I was concerned that the calling of autofs_wait_release() and
> autofs_catatonic_mode() was not mutually exclusive but that can't be the
> case (obviously) because the queue entry (or entries) is removed from the
> list when either of these two functions are called. Consequently the wait
> entry will be freed by only one of these functions or by the woken process
> in autofs_wait() depending on the order of the calls.
> end edit: imk
>
> Reported-by: [email protected]
> Suggested-by: Takeshi Misawa <[email protected]>
> Signed-off-by: Fedor Pchelkin <[email protected]>
> Signed-off-by: Alexey Khoroshilov <[email protected]>
> Signed-off-by: Ian Kent <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Andrei Vagin <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> fs/autofs/waitq.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/autofs/waitq.c b/fs/autofs/waitq.c
> index 54c1f8b8b075..efdc76732fae 100644
> --- a/fs/autofs/waitq.c
> +++ b/fs/autofs/waitq.c
> @@ -32,8 +32,9 @@ void autofs_catatonic_mode(struct autofs_sb_info *sbi)
> wq->status = -ENOENT; /* Magic is gone - report failure */
> kfree(wq->name.name - wq->offset);
> wq->name.name = NULL;
> - wq->wait_ctr--;
> wake_up_interruptible(&wq->queue);
> + if (!--wq->wait_ctr)
> + kfree(wq);
The only thing that peeked my interest was:
autofs_wait()
-> if (!wq)
-> wq->wait_ctr = 2;
-> autofs_notify_daemon()
Let's say autofs_write() fails with -EIO or for whatever reason and so
we end up calling:
-> autofs_catatonic_mode()
If wait_ctr can be decremented in between so that
autofs_catatonic_mode() frees it and then autofs_wait() would cause a
UAF when it tries to much with wq again. But afaict, this can't happen
because and would also affect autofs_notify_daemon() then.
On Fri, 04 Aug 2023 13:33:12 +0800, Ian Kent wrote:
> Syzkaller reports a memory leak:
>
> BUG: memory leak
> unreferenced object 0xffff88810b279e00 (size 96):
> comm "syz-executor399", pid 3631, jiffies 4294964921 (age 23.870s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 08 9e 27 0b 81 88 ff ff ..........'.....
> 08 9e 27 0b 81 88 ff ff 00 00 00 00 00 00 00 00 ..'.............
> backtrace:
> [<ffffffff814cfc90>] kmalloc_trace+0x20/0x90 mm/slab_common.c:1046
> [<ffffffff81bb75ca>] kmalloc include/linux/slab.h:576 [inline]
> [<ffffffff81bb75ca>] autofs_wait+0x3fa/0x9a0 fs/autofs/waitq.c:378
> [<ffffffff81bb88a7>] autofs_do_expire_multi+0xa7/0x3e0 fs/autofs/expire.c:593
> [<ffffffff81bb8c33>] autofs_expire_multi+0x53/0x80 fs/autofs/expire.c:619
> [<ffffffff81bb6972>] autofs_root_ioctl_unlocked+0x322/0x3b0 fs/autofs/root.c:897
> [<ffffffff81bb6a95>] autofs_root_ioctl+0x25/0x30 fs/autofs/root.c:910
> [<ffffffff81602a9c>] vfs_ioctl fs/ioctl.c:51 [inline]
> [<ffffffff81602a9c>] __do_sys_ioctl fs/ioctl.c:870 [inline]
> [<ffffffff81602a9c>] __se_sys_ioctl fs/ioctl.c:856 [inline]
> [<ffffffff81602a9c>] __x64_sys_ioctl+0xfc/0x140 fs/ioctl.c:856
> [<ffffffff84608225>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> [<ffffffff84608225>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> [<ffffffff84800087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> [...]
Applied to the vfs.autofs branch of the vfs/vfs.git tree.
Patches in the vfs.autofs branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.autofs
[1/2] autofs: fix memory leak of waitqueues in autofs_catatonic_mode
https://git.kernel.org/vfs/vfs/c/ccbe77f7e45d
[2/2] autofs: use wake_up() instead of wake_up_interruptible(()
https://git.kernel.org/vfs/vfs/c/17fce12e7c0a
On 4/8/23 19:14, Christian Brauner wrote:
> On Fri, Aug 04, 2023 at 01:33:12PM +0800, Ian Kent wrote:
>> From: Fedor Pchelkin <[email protected]>
>>
>> Syzkaller reports a memory leak:
>>
>> BUG: memory leak
>> unreferenced object 0xffff88810b279e00 (size 96):
>> comm "syz-executor399", pid 3631, jiffies 4294964921 (age 23.870s)
>> hex dump (first 32 bytes):
>> 00 00 00 00 00 00 00 00 08 9e 27 0b 81 88 ff ff ..........'.....
>> 08 9e 27 0b 81 88 ff ff 00 00 00 00 00 00 00 00 ..'.............
>> backtrace:
>> [<ffffffff814cfc90>] kmalloc_trace+0x20/0x90 mm/slab_common.c:1046
>> [<ffffffff81bb75ca>] kmalloc include/linux/slab.h:576 [inline]
>> [<ffffffff81bb75ca>] autofs_wait+0x3fa/0x9a0 fs/autofs/waitq.c:378
>> [<ffffffff81bb88a7>] autofs_do_expire_multi+0xa7/0x3e0 fs/autofs/expire.c:593
>> [<ffffffff81bb8c33>] autofs_expire_multi+0x53/0x80 fs/autofs/expire.c:619
>> [<ffffffff81bb6972>] autofs_root_ioctl_unlocked+0x322/0x3b0 fs/autofs/root.c:897
>> [<ffffffff81bb6a95>] autofs_root_ioctl+0x25/0x30 fs/autofs/root.c:910
>> [<ffffffff81602a9c>] vfs_ioctl fs/ioctl.c:51 [inline]
>> [<ffffffff81602a9c>] __do_sys_ioctl fs/ioctl.c:870 [inline]
>> [<ffffffff81602a9c>] __se_sys_ioctl fs/ioctl.c:856 [inline]
>> [<ffffffff81602a9c>] __x64_sys_ioctl+0xfc/0x140 fs/ioctl.c:856
>> [<ffffffff84608225>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>> [<ffffffff84608225>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
>> [<ffffffff84800087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> autofs_wait_queue structs should be freed if their wait_ctr becomes zero.
>> Otherwise they will be lost.
>>
>> In this case an AUTOFS_IOC_EXPIRE_MULTI ioctl is done, then a new
>> waitqueue struct is allocated in autofs_wait(), its initial wait_ctr
>> equals 2. After that wait_event_killable() is interrupted (it returns
>> -ERESTARTSYS), so that 'wq->name.name == NULL' condition may be not
>> satisfied. Actually, this condition can be satisfied when
>> autofs_wait_release() or autofs_catatonic_mode() is called and, what is
>> also important, wait_ctr is decremented in those places. Upon the exit of
>> autofs_wait(), wait_ctr is decremented to 1. Then the unmounting process
>> begins: kill_sb calls autofs_catatonic_mode(), which should have freed the
>> waitqueues, but it only decrements its usage counter to zero which is not
>> a correct behaviour.
>>
>> edit:imk
>> This description is of course not correct. The umount performed as a result
>> of an expire is a umount of a mount that has been automounted, it's not the
>> autofs mount itself. They happen independently, usually after everything
>> mounted within the autofs file system has been expired away. If everything
>> hasn't been expired away the automount daemon can still exit leaving mounts
>> in place. But expires done in both cases will result in a notification that
>> calls autofs_wait_release() with a result status. The problem case is the
>> summary execution of of the automount daemon. In this case any waiting
>> processes won't be woken up until either they are terminated or the mount
>> is umounted.
>> end edit: imk
>>
>> So in catatonic mode we should free waitqueues which counter becomes zero.
>>
>> edit: imk
>> Initially I was concerned that the calling of autofs_wait_release() and
>> autofs_catatonic_mode() was not mutually exclusive but that can't be the
>> case (obviously) because the queue entry (or entries) is removed from the
>> list when either of these two functions are called. Consequently the wait
>> entry will be freed by only one of these functions or by the woken process
>> in autofs_wait() depending on the order of the calls.
>> end edit: imk
>>
>> Reported-by: [email protected]
>> Suggested-by: Takeshi Misawa <[email protected]>
>> Signed-off-by: Fedor Pchelkin <[email protected]>
>> Signed-off-by: Alexey Khoroshilov <[email protected]>
>> Signed-off-by: Ian Kent <[email protected]>
>> Cc: Matthew Wilcox <[email protected]>
>> Cc: Andrei Vagin <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> fs/autofs/waitq.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/autofs/waitq.c b/fs/autofs/waitq.c
>> index 54c1f8b8b075..efdc76732fae 100644
>> --- a/fs/autofs/waitq.c
>> +++ b/fs/autofs/waitq.c
>> @@ -32,8 +32,9 @@ void autofs_catatonic_mode(struct autofs_sb_info *sbi)
>> wq->status = -ENOENT; /* Magic is gone - report failure */
>> kfree(wq->name.name - wq->offset);
>> wq->name.name = NULL;
>> - wq->wait_ctr--;
>> wake_up_interruptible(&wq->queue);
>> + if (!--wq->wait_ctr)
>> + kfree(wq);
> The only thing that peeked my interest was:
>
> autofs_wait()
> -> if (!wq)
> -> wq->wait_ctr = 2;
> -> autofs_notify_daemon()
>
> Let's say autofs_write() fails with -EIO or for whatever reason and so
> we end up calling:
>
> -> autofs_catatonic_mode()
>
> If wait_ctr can be decremented in between so that
> autofs_catatonic_mode() frees it and then autofs_wait() would cause a
> UAF when it tries to much with wq again. But afaict, this can't happen
> because and would also affect autofs_notify_daemon() then.
Interesting observation.
I'll think about it some more.
But I think a call autofs_catatonic_mode() or autofs_wait_release()
from autofs_notify_daemon() will reduce the count by one. At this
point there can't be any other calls to autofs_wait_release() for
this wait id since they come back as a result of the notification. But
perhaps there could be a call for another wait id which implies that
catatonic mode might cause a problem ... I'm not sure that can happen ...
if the pipe isn't setup then the autofs mount hasn't been done ... if
the pipe has gone away the daemon has gone away so no calls to
autofs_wait_release() ...
It is worth some more thought though ...
I guess there could be something odd where some process accesses
a path and triggers a request when the daemon is killed and then
the mount is umounted at the same time of the request but it's
hard to see how that could happen.
Ian