Skip to content

Commit

Permalink
ksmbd: fix race condition with fp
Browse files Browse the repository at this point in the history
fp can used in each command. If smb2_close command is coming at the
same time, UAF issue can happen by race condition.

                           Time
                            +
Thread A                    | Thread B1 B2 .... B5
smb2_open                   | smb2_close
                            |
 __open_id                  |
   insert fp to file_table  |
                            |
                            |   atomic_dec_and_test(&fp->refcount)
                            |   if fp->refcount == 0, free fp by kfree.
 // UAF!                    |
 use fp                     |
                            +
This patch add f_state not to use freed fp is used and not to free fp in
use.

Signed-off-by: Namjae Jeon <[email protected]>
  • Loading branch information
namjaejeon committed Oct 4, 2023
1 parent 7696f47 commit 36eb997
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 4 deletions.
4 changes: 3 additions & 1 deletion smb2pdu.c
Original file line number Diff line number Diff line change
Expand Up @@ -3564,8 +3564,10 @@ int smb2_open(struct ksmbd_work *work)
#endif
ksmbd_revert_fsids(work);
err_out1:
if (!rc)
if (!rc) {
ksmbd_update_fstate(&work->sess->file_table, fp, FP_INITED);
rc = ksmbd_iov_pin_rsp(work, (void *)rsp, iov_len);
}
if (rc) {
if (rc == -EINVAL)
rsp->hdr.Status = STATUS_INVALID_PARAMETER;
Expand Down
23 changes: 20 additions & 3 deletions vfs_cache.c
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,9 @@ static void __ksmbd_close_fd(struct ksmbd_file_table *ft, struct ksmbd_file *fp)

static struct ksmbd_file *ksmbd_fp_get(struct ksmbd_file *fp)
{
if (fp->f_state != FP_INITED)
return NULL;

if (!atomic_inc_not_zero(&fp->refcount))
return NULL;
return fp;
Expand Down Expand Up @@ -406,15 +409,20 @@ int ksmbd_close_fd(struct ksmbd_work *work, u64 id)
return 0;

ft = &work->sess->file_table;
read_lock(&ft->lock);
write_lock(&ft->lock);
fp = idr_find(ft->idr, id);
if (fp) {
set_close_state_blocked_works(fp);

if (!atomic_dec_and_test(&fp->refcount))
if (fp->f_state != FP_INITED)
fp = NULL;
else {
fp->f_state = FP_CLOSED;
if (!atomic_dec_and_test(&fp->refcount))
fp = NULL;
}
}
read_unlock(&ft->lock);
write_unlock(&ft->lock);

if (!fp)
return -EINVAL;
Expand Down Expand Up @@ -630,6 +638,7 @@ struct ksmbd_file *ksmbd_open_fd(struct ksmbd_work *work, struct file *filp)
fp->tcon = work->tcon;
fp->volatile_id = KSMBD_NO_FID;
fp->persistent_id = KSMBD_NO_FID;
fp->f_state = FP_NEW;
fp->f_ci = ksmbd_inode_get(fp);

if (!fp->f_ci) {
Expand All @@ -651,6 +660,14 @@ struct ksmbd_file *ksmbd_open_fd(struct ksmbd_work *work, struct file *filp)
return ERR_PTR(ret);
}

void ksmbd_update_fstate(struct ksmbd_file_table *ft, struct ksmbd_file *fp,
unsigned int state)
{
write_lock(&ft->lock);
fp->f_state = state;
write_unlock(&ft->lock);
}

static int
__close_file_table_ids(struct ksmbd_file_table *ft,
struct ksmbd_tree_connect *tcon,
Expand Down
9 changes: 9 additions & 0 deletions vfs_cache.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ struct ksmbd_inode {
__le32 m_fattr;
};

enum {
FP_NEW = 0,
FP_INITED,
FP_CLOSED
};

struct ksmbd_file {
struct file *filp;
u64 persistent_id;
Expand Down Expand Up @@ -112,6 +118,7 @@ struct ksmbd_file {
/* if ls is happening on directory, below is valid*/
struct ksmbd_readdir_data readdir_data;
int dot_dotdot[2];
unsigned int f_state;
};

static inline void set_ctx_actor(struct dir_context *ctx,
Expand Down Expand Up @@ -160,6 +167,8 @@ int ksmbd_init_global_file_table(void);
void ksmbd_free_global_file_table(void);
int ksmbd_file_table_flush(struct ksmbd_work *work);
void ksmbd_set_fd_limit(unsigned long limit);
void ksmbd_update_fstate(struct ksmbd_file_table *ft, struct ksmbd_file *fp,
unsigned int state);

/*
* INODE hash
Expand Down

0 comments on commit 36eb997

Please sign in to comment.