* [PATCH v2] ksmbd: fix possible memory leak in smb2_lock()
@ 2023-02-06 2:36 Hangyu Hua
2023-02-06 23:44 ` Namjae Jeon
0 siblings, 1 reply; 4+ messages in thread
From: Hangyu Hua @ 2023-02-06 2:36 UTC (permalink / raw)
To: linkinjeon, sfrench, senozhatsky, tom, lsahlber, hyc.lee
Cc: linux-cifs, linux-kernel, Hangyu Hua
argv needs to be free when setup_async_work fails or when the current
process is woken up.
Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
---
v2: avoid NULL pointer dereference in set_close_state_blocked_works()
fs/ksmbd/smb2pdu.c | 5 +++++
fs/ksmbd/vfs_cache.c | 2 ++
2 files changed, 7 insertions(+)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index d681f91947d9..177a24704021 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7050,6 +7050,7 @@ int smb2_lock(struct ksmbd_work *work)
smb2_remove_blocked_lock,
argv);
if (rc) {
+ kfree(argv);
err = -ENOMEM;
goto out;
}
@@ -7072,6 +7073,8 @@ int smb2_lock(struct ksmbd_work *work)
spin_lock(&fp->f_lock);
list_del(&work->fp_entry);
spin_unlock(&fp->f_lock);
+ kfree(argv);
+ work->cancel_fn = NULL;
rsp->hdr.Status =
STATUS_CANCELLED;
kfree(smb_lock);
@@ -7096,6 +7099,8 @@ int smb2_lock(struct ksmbd_work *work)
spin_lock(&fp->f_lock);
list_del(&work->fp_entry);
spin_unlock(&fp->f_lock);
+ kfree(argv);
+ work->cancel_fn = NULL;
goto retry;
} else if (!rc) {
spin_lock(&work->conn->llist_lock);
diff --git a/fs/ksmbd/vfs_cache.c b/fs/ksmbd/vfs_cache.c
index da9163b00350..eb95c16393b7 100644
--- a/fs/ksmbd/vfs_cache.c
+++ b/fs/ksmbd/vfs_cache.c
@@ -372,6 +372,8 @@ static void set_close_state_blocked_works(struct ksmbd_file *fp)
list_del(&cancel_work->fp_entry);
cancel_work->state = KSMBD_WORK_CLOSED;
cancel_work->cancel_fn(cancel_work->cancel_argv);
+ kfree(cancel_work->cancel_argv);
+ cancel_work->cancel_fn = NULL;
}
spin_unlock(&fp->f_lock);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2] ksmbd: fix possible memory leak in smb2_lock()
2023-02-06 2:36 [PATCH v2] ksmbd: fix possible memory leak in smb2_lock() Hangyu Hua
@ 2023-02-06 23:44 ` Namjae Jeon
2023-02-07 3:22 ` Hangyu Hua
0 siblings, 1 reply; 4+ messages in thread
From: Namjae Jeon @ 2023-02-06 23:44 UTC (permalink / raw)
To: Hangyu Hua
Cc: sfrench, senozhatsky, tom, lsahlber, hyc.lee, linux-cifs,
linux-kernel
2023-02-06 11:36 GMT+09:00, Hangyu Hua <hbh25y@gmail.com>:
> argv needs to be free when setup_async_work fails or when the current
> process is woken up.
>
> Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> ---
>
> v2: avoid NULL pointer dereference in set_close_state_blocked_works()
>
> fs/ksmbd/smb2pdu.c | 5 +++++
> fs/ksmbd/vfs_cache.c | 2 ++
> 2 files changed, 7 insertions(+)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index d681f91947d9..177a24704021 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -7050,6 +7050,7 @@ int smb2_lock(struct ksmbd_work *work)
> smb2_remove_blocked_lock,
> argv);
> if (rc) {
> + kfree(argv);
> err = -ENOMEM;
> goto out;
> }
> @@ -7072,6 +7073,8 @@ int smb2_lock(struct ksmbd_work *work)
> spin_lock(&fp->f_lock);
> list_del(&work->fp_entry);
> spin_unlock(&fp->f_lock);
> + kfree(argv);
> + work->cancel_fn = NULL;
> rsp->hdr.Status =
> STATUS_CANCELLED;
> kfree(smb_lock);
> @@ -7096,6 +7099,8 @@ int smb2_lock(struct ksmbd_work *work)
> spin_lock(&fp->f_lock);
> list_del(&work->fp_entry);
> spin_unlock(&fp->f_lock);
> + kfree(argv);
> + work->cancel_fn = NULL;
This doesn't seem so simple... You have to consider the racy issue
between this change and smb2_cancel(). Also, how are you testing this
patch?
> goto retry;
> } else if (!rc) {
> spin_lock(&work->conn->llist_lock);
> diff --git a/fs/ksmbd/vfs_cache.c b/fs/ksmbd/vfs_cache.c
> index da9163b00350..eb95c16393b7 100644
> --- a/fs/ksmbd/vfs_cache.c
> +++ b/fs/ksmbd/vfs_cache.c
> @@ -372,6 +372,8 @@ static void set_close_state_blocked_works(struct
> ksmbd_file *fp)
> list_del(&cancel_work->fp_entry);
> cancel_work->state = KSMBD_WORK_CLOSED;
> cancel_work->cancel_fn(cancel_work->cancel_argv);
> + kfree(cancel_work->cancel_argv);
> + cancel_work->cancel_fn = NULL;
> }
> spin_unlock(&fp->f_lock);
> }
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] ksmbd: fix possible memory leak in smb2_lock()
2023-02-06 23:44 ` Namjae Jeon
@ 2023-02-07 3:22 ` Hangyu Hua
2023-02-07 4:22 ` Namjae Jeon
0 siblings, 1 reply; 4+ messages in thread
From: Hangyu Hua @ 2023-02-07 3:22 UTC (permalink / raw)
To: Namjae Jeon
Cc: sfrench, senozhatsky, tom, lsahlber, hyc.lee, linux-cifs,
linux-kernel
On 7/2/2023 07:44, Namjae Jeon wrote:
> 2023-02-06 11:36 GMT+09:00, Hangyu Hua <hbh25y@gmail.com>:
>> argv needs to be free when setup_async_work fails or when the current
>> process is woken up.
>>
>> Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
>> ---
>>
>> v2: avoid NULL pointer dereference in set_close_state_blocked_works()
>>
>> fs/ksmbd/smb2pdu.c | 5 +++++
>> fs/ksmbd/vfs_cache.c | 2 ++
>> 2 files changed, 7 insertions(+)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index d681f91947d9..177a24704021 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -7050,6 +7050,7 @@ int smb2_lock(struct ksmbd_work *work)
>> smb2_remove_blocked_lock,
>> argv);
>> if (rc) {
>> + kfree(argv);
>> err = -ENOMEM;
>> goto out;
>> }
>> @@ -7072,6 +7073,8 @@ int smb2_lock(struct ksmbd_work *work)
>> spin_lock(&fp->f_lock);
>> list_del(&work->fp_entry);
>> spin_unlock(&fp->f_lock);
>> + kfree(argv);
>> + work->cancel_fn = NULL;
>> rsp->hdr.Status =
>> STATUS_CANCELLED;
>> kfree(smb_lock);
>> @@ -7096,6 +7099,8 @@ int smb2_lock(struct ksmbd_work *work)
>> spin_lock(&fp->f_lock);
>> list_del(&work->fp_entry);
>> spin_unlock(&fp->f_lock);
>> + kfree(argv);
>> + work->cancel_fn = NULL;
> This doesn't seem so simple... You have to consider the racy issue
> between this change and smb2_cancel(). Also, how are you testing this
> patch?
>
Do you means a race condition between smb2_lock() and smb2_cancel()?
This look like another bug. This problem exists even if this patch does
not exist. If state becomes KSMBD_WORK_CANCELLED we shouldn't go retry.
But you are right. This patch will cause UAF in race condition. I think
setting cancel_fn to NULL before releasing argv can fix this UAF. After
this memory leak is solved we should use another patch to fix the race
condition. What do you think?
Thanks,
Hangyu
>> goto retry;
>> } else if (!rc) {
>> spin_lock(&work->conn->llist_lock);
>> diff --git a/fs/ksmbd/vfs_cache.c b/fs/ksmbd/vfs_cache.c
>> index da9163b00350..eb95c16393b7 100644
>> --- a/fs/ksmbd/vfs_cache.c
>> +++ b/fs/ksmbd/vfs_cache.c
>> @@ -372,6 +372,8 @@ static void set_close_state_blocked_works(struct
>> ksmbd_file *fp)
>> list_del(&cancel_work->fp_entry);
>> cancel_work->state = KSMBD_WORK_CLOSED;
>> cancel_work->cancel_fn(cancel_work->cancel_argv);
>> + kfree(cancel_work->cancel_argv);
>> + cancel_work->cancel_fn = NULL;
>> }
>> spin_unlock(&fp->f_lock);
>> }
>> --
>> 2.34.1
>>
>>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] ksmbd: fix possible memory leak in smb2_lock()
2023-02-07 3:22 ` Hangyu Hua
@ 2023-02-07 4:22 ` Namjae Jeon
0 siblings, 0 replies; 4+ messages in thread
From: Namjae Jeon @ 2023-02-07 4:22 UTC (permalink / raw)
To: Hangyu Hua
Cc: sfrench, senozhatsky, tom, lsahlber, hyc.lee, linux-cifs,
linux-kernel
2023-02-07 12:22 GMT+09:00, Hangyu Hua <hbh25y@gmail.com>:
> On 7/2/2023 07:44, Namjae Jeon wrote:
>> 2023-02-06 11:36 GMT+09:00, Hangyu Hua <hbh25y@gmail.com>:
>>> argv needs to be free when setup_async_work fails or when the current
>>> process is woken up.
>>>
>>> Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
>>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
>>> ---
>>>
>>> v2: avoid NULL pointer dereference in set_close_state_blocked_works()
>>>
>>> fs/ksmbd/smb2pdu.c | 5 +++++
>>> fs/ksmbd/vfs_cache.c | 2 ++
>>> 2 files changed, 7 insertions(+)
>>>
>>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>>> index d681f91947d9..177a24704021 100644
>>> --- a/fs/ksmbd/smb2pdu.c
>>> +++ b/fs/ksmbd/smb2pdu.c
>>> @@ -7050,6 +7050,7 @@ int smb2_lock(struct ksmbd_work *work)
>>> smb2_remove_blocked_lock,
>>> argv);
>>> if (rc) {
>>> + kfree(argv);
>>> err = -ENOMEM;
>>> goto out;
>>> }
>>> @@ -7072,6 +7073,8 @@ int smb2_lock(struct ksmbd_work *work)
>>> spin_lock(&fp->f_lock);
>>> list_del(&work->fp_entry);
>>> spin_unlock(&fp->f_lock);
>>> + kfree(argv);
>>> + work->cancel_fn = NULL;
>>> rsp->hdr.Status =
>>> STATUS_CANCELLED;
>>> kfree(smb_lock);
>>> @@ -7096,6 +7099,8 @@ int smb2_lock(struct ksmbd_work *work)
>>> spin_lock(&fp->f_lock);
>>> list_del(&work->fp_entry);
>>> spin_unlock(&fp->f_lock);
>>> + kfree(argv);
>>> + work->cancel_fn = NULL;
>> This doesn't seem so simple... You have to consider the racy issue
>> between this change and smb2_cancel(). Also, how are you testing this
>> patch?
>>
>
> Do you means a race condition between smb2_lock() and smb2_cancel()?
> This look like another bug. This problem exists even if this patch does
> not exist. If state becomes KSMBD_WORK_CANCELLED we shouldn't go retry.
>
> But you are right. This patch will cause UAF in race condition. I think
> setting cancel_fn to NULL before releasing argv can fix this UAF.
I don't know until I see your updated patch. It may be protected by lock.
> After
> this memory leak is solved we should use another patch to fix the race
> condition. What do you think?
It doesn't matter whether it's a single patch or a multi-patches, as
long as this patch doesn't cause racy issue...
Thanks.
>
> Thanks,
> Hangyu
>
>>> goto retry;
>>> } else if (!rc) {
>>> spin_lock(&work->conn->llist_lock);
>>> diff --git a/fs/ksmbd/vfs_cache.c b/fs/ksmbd/vfs_cache.c
>>> index da9163b00350..eb95c16393b7 100644
>>> --- a/fs/ksmbd/vfs_cache.c
>>> +++ b/fs/ksmbd/vfs_cache.c
>>> @@ -372,6 +372,8 @@ static void set_close_state_blocked_works(struct
>>> ksmbd_file *fp)
>>> list_del(&cancel_work->fp_entry);
>>> cancel_work->state = KSMBD_WORK_CLOSED;
>>> cancel_work->cancel_fn(cancel_work->cancel_argv);
>>> + kfree(cancel_work->cancel_argv);
>>> + cancel_work->cancel_fn = NULL;
>>> }
>>> spin_unlock(&fp->f_lock);
>>> }
>>> --
>>> 2.34.1
>>>
>>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-02-07 4:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-06 2:36 [PATCH v2] ksmbd: fix possible memory leak in smb2_lock() Hangyu Hua
2023-02-06 23:44 ` Namjae Jeon
2023-02-07 3:22 ` Hangyu Hua
2023-02-07 4:22 ` Namjae Jeon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox