| Message ID | 20230214063650.12832-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] ksmbd: fix possible memory leak in smb2_lock() | expand |
2023-02-14 15: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() > v3: avoid race condition between smb2_lock() and smb2_cancel() > v4: use another way to avoid race condition > > fs/ksmbd/smb2pdu.c | 20 +++++++++++++------- > fs/ksmbd/vfs_cache.c | 2 ++ > 2 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index d681f91947d9..1b517d3ca2ef 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -6666,7 +6666,8 @@ int smb2_cancel(struct ksmbd_work *work) > "smb2 with AsyncId %llu cancelled command = 0x%x\n", > le64_to_cpu(hdr->Id.AsyncId), > le16_to_cpu(chdr->Command)); > - cancel_work = iter; > + if (iter->cancel_fn) > + cancel_work = iter; You have freed ->cancel_argv in smb2_lock(). Wouldn't this cause UAF issue the below? if (cancel_work) { cancel_work->state = KSMBD_WORK_CANCELLED; if (cancel_work->cancel_fn) cancel_work->cancel_fn(cancel_work->cancel_argv); } > break; > } > spin_unlock(&conn->request_lock); > @@ -7050,6 +7051,7 @@ int smb2_lock(struct ksmbd_work *work) > smb2_remove_blocked_lock, > argv); > if (rc) { > + kfree(argv); > err = -ENOMEM; > goto out; > } > @@ -7061,6 +7063,16 @@ int smb2_lock(struct ksmbd_work *work) > > ksmbd_vfs_posix_lock_wait(flock); > > + spin_lock(&work->conn->request_lock); > + spin_lock(&fp->f_lock); > + if (work->state != KSMBD_WORK_CLOSED) { I think that this check is not needed if we don't delete entry in set_close_state_blocked_works(). > + list_del(&work->fp_entry); > + work->cancel_fn = NULL; > + kfree(argv); > + } > + spin_unlock(&fp->f_lock); > + spin_unlock(&work->conn->request_lock); > + > if (work->state != KSMBD_WORK_ACTIVE) { > list_del(&smb_lock->llist); > spin_lock(&work->conn->llist_lock); > @@ -7069,9 +7081,6 @@ int smb2_lock(struct ksmbd_work *work) > locks_free_lock(flock); > > if (work->state == KSMBD_WORK_CANCELLED) { > - spin_lock(&fp->f_lock); > - list_del(&work->fp_entry); > - spin_unlock(&fp->f_lock); > rsp->hdr.Status = > STATUS_CANCELLED; > kfree(smb_lock); > @@ -7093,9 +7102,6 @@ int smb2_lock(struct ksmbd_work *work) > list_del(&smb_lock->clist); > spin_unlock(&work->conn->llist_lock); > > - spin_lock(&fp->f_lock); > - list_del(&work->fp_entry); > - spin_unlock(&fp->f_lock); > 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..761a8aa540ce 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); > + cancel_work->cancel_fn = NULL; > + kfree(cancel_work->cancel_argv); If we remove list_del, we don't need to do them here ? and we can change _safe version to list_for_each(). > } > spin_unlock(&fp->f_lock); > } > -- > 2.34.1 > >
On 15/2/2023 09:04, Namjae Jeon wrote: > 2023-02-14 15: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() >> v3: avoid race condition between smb2_lock() and smb2_cancel() >> v4: use another way to avoid race condition >> >> fs/ksmbd/smb2pdu.c | 20 +++++++++++++------- >> fs/ksmbd/vfs_cache.c | 2 ++ >> 2 files changed, 15 insertions(+), 7 deletions(-) >> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> index d681f91947d9..1b517d3ca2ef 100644 >> --- a/fs/ksmbd/smb2pdu.c >> +++ b/fs/ksmbd/smb2pdu.c >> @@ -6666,7 +6666,8 @@ int smb2_cancel(struct ksmbd_work *work) >> "smb2 with AsyncId %llu cancelled command = 0x%x\n", >> le64_to_cpu(hdr->Id.AsyncId), >> le16_to_cpu(chdr->Command)); >> - cancel_work = iter; >> + if (iter->cancel_fn) >> + cancel_work = iter; > You have freed ->cancel_argv in smb2_lock(). Wouldn't this cause UAF > issue the below? > > if (cancel_work) { > cancel_work->state = KSMBD_WORK_CANCELLED; > if (cancel_work->cancel_fn) > cancel_work->cancel_fn(cancel_work->cancel_argv); > } I think this can't cause UAF beacause cancel_fn will be set to NULL before releasing argv in smb2_lock(). There is no way to use a argv. >> break; >> } >> spin_unlock(&conn->request_lock); >> @@ -7050,6 +7051,7 @@ int smb2_lock(struct ksmbd_work *work) >> smb2_remove_blocked_lock, >> argv); >> if (rc) { >> + kfree(argv); >> err = -ENOMEM; >> goto out; >> } >> @@ -7061,6 +7063,16 @@ int smb2_lock(struct ksmbd_work *work) >> >> ksmbd_vfs_posix_lock_wait(flock); >> >> + spin_lock(&work->conn->request_lock); >> + spin_lock(&fp->f_lock); >> + if (work->state != KSMBD_WORK_CLOSED) { > I think that this check is not needed if we don't delete entry in > set_close_state_blocked_works(). >> + list_del(&work->fp_entry); >> + work->cancel_fn = NULL; >> + kfree(argv); >> + } >> + spin_unlock(&fp->f_lock); >> + spin_unlock(&work->conn->request_lock); >> + >> if (work->state != KSMBD_WORK_ACTIVE) { >> list_del(&smb_lock->llist); >> spin_lock(&work->conn->llist_lock); >> @@ -7069,9 +7081,6 @@ int smb2_lock(struct ksmbd_work *work) >> locks_free_lock(flock); >> >> if (work->state == KSMBD_WORK_CANCELLED) { >> - spin_lock(&fp->f_lock); >> - list_del(&work->fp_entry); >> - spin_unlock(&fp->f_lock); >> rsp->hdr.Status = >> STATUS_CANCELLED; >> kfree(smb_lock); >> @@ -7093,9 +7102,6 @@ int smb2_lock(struct ksmbd_work *work) >> list_del(&smb_lock->clist); >> spin_unlock(&work->conn->llist_lock); >> >> - spin_lock(&fp->f_lock); >> - list_del(&work->fp_entry); >> - spin_unlock(&fp->f_lock); >> 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..761a8aa540ce 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); >> + cancel_work->cancel_fn = NULL; >> + kfree(cancel_work->cancel_argv); > If we remove list_del, we don't need to do them here ? and we can > change _safe version to list_for_each(). Agreed. I will move this list_del() to smb2_lock() in v5. Thanks, Hangyu >> } >> spin_unlock(&fp->f_lock); >> } >> -- >> 2.34.1 >> >>
2023-02-14 15: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> Acked-by: Namjae Jeon <linkinjeon@kernel.org> Thanks.
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index d681f91947d9..1b517d3ca2ef 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -6666,7 +6666,8 @@ int smb2_cancel(struct ksmbd_work *work) "smb2 with AsyncId %llu cancelled command = 0x%x\n", le64_to_cpu(hdr->Id.AsyncId), le16_to_cpu(chdr->Command)); - cancel_work = iter; + if (iter->cancel_fn) + cancel_work = iter; break; } spin_unlock(&conn->request_lock); @@ -7050,6 +7051,7 @@ int smb2_lock(struct ksmbd_work *work) smb2_remove_blocked_lock, argv); if (rc) { + kfree(argv); err = -ENOMEM; goto out; } @@ -7061,6 +7063,16 @@ int smb2_lock(struct ksmbd_work *work) ksmbd_vfs_posix_lock_wait(flock); + spin_lock(&work->conn->request_lock); + spin_lock(&fp->f_lock); + if (work->state != KSMBD_WORK_CLOSED) { + list_del(&work->fp_entry); + work->cancel_fn = NULL; + kfree(argv); + } + spin_unlock(&fp->f_lock); + spin_unlock(&work->conn->request_lock); + if (work->state != KSMBD_WORK_ACTIVE) { list_del(&smb_lock->llist); spin_lock(&work->conn->llist_lock); @@ -7069,9 +7081,6 @@ int smb2_lock(struct ksmbd_work *work) locks_free_lock(flock); if (work->state == KSMBD_WORK_CANCELLED) { - spin_lock(&fp->f_lock); - list_del(&work->fp_entry); - spin_unlock(&fp->f_lock); rsp->hdr.Status = STATUS_CANCELLED; kfree(smb_lock); @@ -7093,9 +7102,6 @@ int smb2_lock(struct ksmbd_work *work) list_del(&smb_lock->clist); spin_unlock(&work->conn->llist_lock); - spin_lock(&fp->f_lock); - list_del(&work->fp_entry); - spin_unlock(&fp->f_lock); 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..761a8aa540ce 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); + cancel_work->cancel_fn = NULL; + kfree(cancel_work->cancel_argv); } spin_unlock(&fp->f_lock); }
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() v3: avoid race condition between smb2_lock() and smb2_cancel() v4: use another way to avoid race condition fs/ksmbd/smb2pdu.c | 20 +++++++++++++------- fs/ksmbd/vfs_cache.c | 2 ++ 2 files changed, 15 insertions(+), 7 deletions(-)