diff mbox series

[2/8] smb/server: fix potential null-ptr-deref of lease_ctx_info in smb2_open()

Message ID 20240820143319.274033-3-chenxiaosong@chenxiaosong.com (mailing list archive)
State New, archived
Headers show
Series smb: fix some bugs, move duplicate definitions to common header file, and some small cleanups | expand

Commit Message

ChenXiaoSong Aug. 20, 2024, 2:33 p.m. UTC 
From: ChenXiaoSong <chenxiaosong@kylinos.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@chenxiaosong.com>
---
 fs/smb/server/oplock.c  | 11 +++++++----
 fs/smb/server/smb2pdu.c | 17 ++++++++++++-----
 2 files changed, 19 insertions(+), 9 deletions(-)

Comments

Namjae Jeon Aug. 22, 2024, 12:41 a.m. UTC | #1 
On Tue, Aug 20, 2024 at 11:35 PM <chenxiaosong@chenxiaosong.com> wrote:
>
> From: ChenXiaoSong <chenxiaosong@kylinos.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@chenxiaosong.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
>
ChenXiaoSong Aug. 22, 2024, 12:59 a.m. UTC | #2 
Thanks for your reply. I will send v2 patchset soon based on your 
suggestions.

在 2024/8/22 08:41, Namjae Jeon 写道:
> On Tue, Aug 20, 2024 at 11:35 PM <chenxiaosong@chenxiaosong.com> wrote:
>>
>> From: ChenXiaoSong <chenxiaosong@kylinos.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@chenxiaosong.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
>>
>
diff mbox series

Patch

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",