2023-05-31 10:19:48

by Pradeep Pragallapati

[permalink] [raw]
Subject: [PATCH V1] fuse: Abort the requests under processing queue with a spin_lock

There is a potential race/timing issue while aborting the
requests on processing list between fuse_dev_release() and
fuse_abort_conn(). This is resulting into below warnings
and can even result into UAF issues.

[22809.190255][T31644] refcount_t: underflow; use-after-free.
[22809.190266][T31644] WARNING: CPU: 2 PID: 31644 at lib/refcount.c:28
refcount_warn_saturate+0x110/0x158
...
[22809.190567][T31644] Call trace:
[22809.190567][T31644] refcount_warn_saturate+0x110/0x158
[22809.190569][T31644] fuse_file_put+0xfc/0x104
[22809.190575][T31644] fuse_readpages_end+0x210/0x29c
[22809.190579][T31644] fuse_request_end+0x17c/0x200
[22809.190580][T31644] fuse_dev_release+0xe0/0x1e4
[22809.190582][T31644] __fput+0xfc/0x294
[22809.190588][T31644] ____fput+0x18/0x2c
[22809.190590][T31644] task_work_run+0xd8/0x104
[22809.190599][T31644] do_exit+0x2a8/0xa5c
[22809.190605][T31644] do_group_exit+0x78/0xa4
[22809.190608][T31644] get_signal+0x778/0x8a8
[22809.190614][T31644] do_notify_resume+0x134/0x340
[22809.190617][T31644] el0_svc+0x68/0xc4
[22809.190623][T31644] el0t_64_sync_handler+0x8c/0xfc
[22809.190626][T31644] el0t_64_sync+0x1a0/0x1a4

Fix this by aborting the requests in fuse_dev_release()
under fpq spin lock.

Signed-off-by: Pradeep P V K <[email protected]>
---
fs/fuse/dev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 1a8f82f478cb..bbc33a97ab7c 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -2208,9 +2208,8 @@ int fuse_dev_release(struct inode *inode, struct file *file)
WARN_ON(!list_empty(&fpq->io));
for (i = 0; i < FUSE_PQ_HASH_SIZE; i++)
list_splice_init(&fpq->processing[i], &to_end);
- spin_unlock(&fpq->lock);
-
end_requests(&to_end);
+ spin_unlock(&fpq->lock);

/* Are we the last open device? */
if (atomic_dec_and_test(&fc->dev_count)) {
--
2.17.1



2023-05-31 12:05:20

by Miklos Szeredi

[permalink] [raw]
Subject: Re: [PATCH V1] fuse: Abort the requests under processing queue with a spin_lock

On Wed, 31 May 2023 at 11:26, Pradeep P V K <[email protected]> wrote:
>
> There is a potential race/timing issue while aborting the
> requests on processing list between fuse_dev_release() and
> fuse_abort_conn(). This is resulting into below warnings
> and can even result into UAF issues.

Okay, but...

>
> [22809.190255][T31644] refcount_t: underflow; use-after-free.
> [22809.190266][T31644] WARNING: CPU: 2 PID: 31644 at lib/refcount.c:28
> refcount_warn_saturate+0x110/0x158
> ...
> [22809.190567][T31644] Call trace:
> [22809.190567][T31644] refcount_warn_saturate+0x110/0x158
> [22809.190569][T31644] fuse_file_put+0xfc/0x104

...how can this cause the file refcount to underflow? That would
imply that fuse_request_end() will be called for the same request
twice. I can't see how that can happen with or without the locking
change.

Do you have a reproducer?

Thanks,
Miklos

2023-06-01 10:13:08

by Pradeep Pragallapati

[permalink] [raw]
Subject: Re: [PATCH V1] fuse: Abort the requests under processing queue with a spin_lock


On 5/31/2023 5:22 PM, Miklos Szeredi wrote:
> On Wed, 31 May 2023 at 11:26, Pradeep P V K <[email protected]> wrote:
>> There is a potential race/timing issue while aborting the
>> requests on processing list between fuse_dev_release() and
>> fuse_abort_conn(). This is resulting into below warnings
>> and can even result into UAF issues.
> Okay, but...
>
>> [22809.190255][T31644] refcount_t: underflow; use-after-free.
>> [22809.190266][T31644] WARNING: CPU: 2 PID: 31644 at lib/refcount.c:28
>> refcount_warn_saturate+0x110/0x158
>> ...
>> [22809.190567][T31644] Call trace:
>> [22809.190567][T31644] refcount_warn_saturate+0x110/0x158
>> [22809.190569][T31644] fuse_file_put+0xfc/0x104
> ...how can this cause the file refcount to underflow? That would
> imply that fuse_request_end() will be called for the same request
> twice. I can't see how that can happen with or without the locking
> change.
Please ignore this patch. i overlooked it as list_splice in
fuse_dev_release() and made the change.
> Do you have a reproducer?

don't have exact/specific steps but i will try to recreate. This is
observed during stability testing (involves io, reboot, monkey, e.t.c.)
for 24hrs. So, far this is seen on both 5.15 and 6.1 kernels. Do you
have any points or speculations to share ?

Thanks,

Pradeep

> Thanks,
> Miklos

2023-06-01 12:21:38

by Miklos Szeredi

[permalink] [raw]
Subject: Re: [PATCH V1] fuse: Abort the requests under processing queue with a spin_lock

On Thu, 1 Jun 2023 at 12:02, Pradeep Pragallapati
<[email protected]> wrote:
>
>
> On 5/31/2023 5:22 PM, Miklos Szeredi wrote:
> > On Wed, 31 May 2023 at 11:26, Pradeep P V K <[email protected]> wrote:
> >> There is a potential race/timing issue while aborting the
> >> requests on processing list between fuse_dev_release() and
> >> fuse_abort_conn(). This is resulting into below warnings
> >> and can even result into UAF issues.
> > Okay, but...
> >
> >> [22809.190255][T31644] refcount_t: underflow; use-after-free.
> >> [22809.190266][T31644] WARNING: CPU: 2 PID: 31644 at lib/refcount.c:28
> >> refcount_warn_saturate+0x110/0x158
> >> ...
> >> [22809.190567][T31644] Call trace:
> >> [22809.190567][T31644] refcount_warn_saturate+0x110/0x158
> >> [22809.190569][T31644] fuse_file_put+0xfc/0x104
> > ...how can this cause the file refcount to underflow? That would
> > imply that fuse_request_end() will be called for the same request
> > twice. I can't see how that can happen with or without the locking
> > change.
> Please ignore this patch. i overlooked it as list_splice in
> fuse_dev_release() and made the change.
> > Do you have a reproducer?
>
> don't have exact/specific steps but i will try to recreate. This is
> observed during stability testing (involves io, reboot, monkey, e.t.c.)
> for 24hrs. So, far this is seen on both 5.15 and 6.1 kernels. Do you
> have any points or speculations to share ?

Do you have KASAN enabled in the kernel? That might help UAF issues easier.

Thanks,
Miklos