| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAKYAXd_N2Hgba2EsDkem7mmGrWxxpXOtz4L_ReXwqUCG_BRU=w@mail.gmail.com>
Date: Thu, 22 Aug 2024 09:41:21 +0900
From: Namjae Jeon <linkinjeon@...nel.org>
To: chenxiaosong@...nxiaosong.com
Cc: sfrench@...ba.org, senozhatsky@...omium.org, tom@...pey.com,
linux-cifs@...r.kernel.org, linux-kernel@...r.kernel.org, pc@...guebit.com,
ronniesahlberg@...il.com, sprasad@...rosoft.com, bharathsm@...rosoft.com,
chenxiaosong@...inos.cn, liuzhengyuan@...inos.cn, huhai@...inos.cn,
liuyun01@...inos.cn
Subject: Re: [PATCH 2/8] smb/server: fix potential null-ptr-deref of
lease_ctx_info in smb2_open()
On Tue, Aug 20, 2024 at 11:35 PM <chenxiaosong@...nxiaosong.com> wrote:
>
> From: ChenXiaoSong <chenxiaosong@...inos.cn>
>
> null-ptr-deref will occur when (req_op_level == SMB2_OPLOCK_LEVEL_LEASE)
> and parse_lease_state() return NULL.
>
> Fix this by returning error pointer on parse_lease_state() and checking
> error.
>
> Signed-off-by: ChenXiaoSong <chenxiaosong@...nxiaosong.com>
We intended for it to return null. We shouldn't handle the error even
if it fails.
All places check if lc is null except the one.
We can fix it like the following one. please send this patch if you are okay.
diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index b6c5a8ea3887..884e21992c92 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -3404,7 +3404,7 @@ int smb2_open(struct ksmbd_work *work)
goto err_out1;
}
} else {
- if (req_op_level == SMB2_OPLOCK_LEVEL_LEASE) {
+ if (req_op_level == SMB2_OPLOCK_LEVEL_LEASE && lc) {
if (S_ISDIR(file_inode(filp)->i_mode)) {
lc->req_state &= ~SMB2_LEASE_WRITE_CACHING_LE;
lc->is_dir = true;
> ---
> fs/smb/server/oplock.c | 11 +++++++----
> fs/smb/server/smb2pdu.c | 17 ++++++++++++-----
> 2 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/fs/smb/server/oplock.c b/fs/smb/server/oplock.c
> index a8f52c4ebbda..e8591686a037 100644
> --- a/fs/smb/server/oplock.c
> +++ b/fs/smb/server/oplock.c
> @@ -1510,7 +1510,8 @@ void create_lease_buf(u8 *rbuf, struct lease *lease)
> * parse_lease_state() - parse lease context containted in file open request
> * @open_req: buffer containing smb2 file open(create) request
> *
> - * Return: oplock state, -ENOENT if create lease context not found
> + * Return: allocated lease context object on success, otherwise error pointer.
> + * -ENOENT pointer if create lease context not found.
> */
> struct lease_ctx_info *parse_lease_state(void *open_req)
> {
> @@ -1519,12 +1520,14 @@ struct lease_ctx_info *parse_lease_state(void *open_req)
> struct lease_ctx_info *lreq;
>
> cc = smb2_find_context_vals(req, SMB2_CREATE_REQUEST_LEASE, 4);
> - if (IS_ERR_OR_NULL(cc))
> - return NULL;
> + if (!cc)
> + return ERR_PTR(-ENOENT);
> + if (IS_ERR(cc))
> + return ERR_CAST(cc);
>
> lreq = kzalloc(sizeof(struct lease_ctx_info), GFP_KERNEL);
> if (!lreq)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> if (sizeof(struct lease_context_v2) == le32_to_cpu(cc->DataLength)) {
> struct create_lease_v2 *lc = (struct create_lease_v2 *)cc;
> diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
> index d8a827e0dced..119c1ba5f255 100644
> --- a/fs/smb/server/smb2pdu.c
> +++ b/fs/smb/server/smb2pdu.c
> @@ -2767,8 +2767,9 @@ static int parse_durable_handle_context(struct ksmbd_work *work,
> }
> }
>
> - if (((lc && (lc->req_state & SMB2_LEASE_HANDLE_CACHING_LE)) ||
> - req_op_level == SMB2_OPLOCK_LEVEL_BATCH)) {
> + if ((!IS_ERR_OR_NULL(lc) > 0 &&
> + (lc->req_state & SMB2_LEASE_HANDLE_CACHING_LE)) ||
> + req_op_level == SMB2_OPLOCK_LEVEL_BATCH) {
> dh_info->CreateGuid =
> durable_v2_blob->CreateGuid;
> dh_info->persistent =
> @@ -2788,8 +2789,9 @@ static int parse_durable_handle_context(struct ksmbd_work *work,
> goto out;
> }
>
> - if (((lc && (lc->req_state & SMB2_LEASE_HANDLE_CACHING_LE)) ||
> - req_op_level == SMB2_OPLOCK_LEVEL_BATCH)) {
> + if ((!IS_ERR_OR_NULL(lc) &&
> + (lc->req_state & SMB2_LEASE_HANDLE_CACHING_LE)) ||
> + req_op_level == SMB2_OPLOCK_LEVEL_BATCH) {
> ksmbd_debug(SMB, "Request for durable open\n");
> dh_info->type = dh_idx;
> }
> @@ -2935,8 +2937,13 @@ int smb2_open(struct ksmbd_work *work)
> ksmbd_put_durable_fd(fp);
> goto reconnected_fp;
> }
> - } else if (req_op_level == SMB2_OPLOCK_LEVEL_LEASE)
> + } else if (req_op_level == SMB2_OPLOCK_LEVEL_LEASE) {
> lc = parse_lease_state(req);
> + if (IS_ERR(lc)) {
> + rc = PTR_ERR(lc);
> + goto err_out2;
> + }
> + }
>
> if (le32_to_cpu(req->ImpersonationLevel) > le32_to_cpu(IL_DELEGATE)) {
> pr_err("Invalid impersonationlevel : 0x%x\n",
> --
> 2.34.1
>
Powered by blists - more mailing lists