[PATCH 1/3] ksmbd: use buf_data_size instead of recalculation in smb3_decrypt_req()

[PATCH 1/3] ksmbd: use buf_data_size instead of recalculation in smb3_decrypt_req()

[Namjae Jeon]

Tom suggested to use buf_data_size that is already calculated, to verify
these offsets.

Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Suggested-by: Tom Talpey <tom@talpey.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb2pdu.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index b06361313889..4d1be224dd8e 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -8457,15 +8457,13 @@ int smb3_decrypt_req(struct ksmbd_work *work)
 	struct smb2_transform_hdr *tr_hdr = (struct smb2_transform_hdr *)buf;
 	int rc = 0;
 
-	if (pdu_length + 4 <
-	    sizeof(struct smb2_transform_hdr) + sizeof(struct smb2_hdr)) {
+	if (buf_data_size < sizeof(struct smb2_hdr)) {
 		pr_err("Transform message is too small (%u)\n",
 		       pdu_length);
 		return -ECONNABORTED;
 	}
 
-	if (pdu_length + 4 <
-	    le32_to_cpu(tr_hdr->OriginalMessageSize) + sizeof(struct smb2_transform_hdr)) {
+	if (buf_data_size < le32_to_cpu(tr_hdr->OriginalMessageSize)) {
 		pr_err("Transform message is broken\n");
 		return -ECONNABORTED;
 	}
-- 
2.25.1

[PATCH 2/3] ksmbd: fix version mismatch with out of tree

[Namjae Jeon]

Fix version mismatch with out of tree, This updated version will be
matched with ksmbd-tools.

Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/glob.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ksmbd/glob.h b/fs/ksmbd/glob.h
index 49a5a3afa118..5b8f3e0ebdb3 100644
--- a/fs/ksmbd/glob.h
+++ b/fs/ksmbd/glob.h
@@ -12,7 +12,7 @@
 #include "unicode.h"
 #include "vfs_cache.h"
 
-#define KSMBD_VERSION	"3.1.9"
+#define KSMBD_VERSION	"3.4.2"
 
 extern int ksmbd_debug_types;
 
-- 
2.25.1

[PATCH 3/3] ksmbd: fix oops from fuse driver

[Namjae Jeon]

Marios reported kernel oops from fuse driver when ksmbd call
mark_inode_dirty(). This patch directly update ->i_ctime after removing
mark_inode_ditry() and notify_change will put inode to dirty list.

Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Reported-by: Marios Makassikis <mmakassikis@freebox.fr>
Tested-by: Marios Makassikis <mmakassikis@freebox.fr>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb2pdu.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 4d1be224dd8e..ed8324f9c2bd 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -5483,7 +5483,6 @@ static int set_file_basic_info(struct ksmbd_file *fp,
 			       struct ksmbd_share_config *share)
 {
 	struct iattr attrs;
-	struct timespec64 ctime;
 	struct file *filp;
 	struct inode *inode;
 	struct user_namespace *user_ns;
@@ -5505,13 +5504,11 @@ static int set_file_basic_info(struct ksmbd_file *fp,
 		attrs.ia_valid |= (ATTR_ATIME | ATTR_ATIME_SET);
 	}
 
-	if (file_info->ChangeTime) {
+	attrs.ia_valid |= ATTR_CTIME;
+	if (file_info->ChangeTime)
 		attrs.ia_ctime = ksmbd_NTtimeToUnix(file_info->ChangeTime);
-		ctime = attrs.ia_ctime;
-		attrs.ia_valid |= ATTR_CTIME;
-	} else {
-		ctime = inode->i_ctime;
-	}
+	else
+		attrs.ia_ctime = inode->i_ctime;
 
 	if (file_info->LastWriteTime) {
 		attrs.ia_mtime = ksmbd_NTtimeToUnix(file_info->LastWriteTime);
@@ -5557,11 +5554,9 @@ static int set_file_basic_info(struct ksmbd_file *fp,
 			return -EACCES;
 
 		inode_lock(inode);
+		inode->i_ctime = attrs.ia_ctime;
+		attrs.ia_valid &= ~ATTR_CTIME;
 		rc = notify_change(user_ns, dentry, &attrs, NULL);
-		if (!rc) {
-			inode->i_ctime = ctime;
-			mark_inode_dirty(inode);
-		}
 		inode_unlock(inode);
 	}
 	return rc;
-- 
2.25.1

Re: [PATCH 1/3] ksmbd: use buf_data_size instead of recalculation in smb3_decrypt_req()

[Hyunchul Lee]

2021년 10월 3일 (일) 오후 1:31, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> Tom suggested to use buf_data_size that is already calculated, to verify
> these offsets.
>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Suggested-by: Tom Talpey <tom@talpey.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>  fs/ksmbd/smb2pdu.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index b06361313889..4d1be224dd8e 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -8457,15 +8457,13 @@ int smb3_decrypt_req(struct ksmbd_work *work)
>         struct smb2_transform_hdr *tr_hdr = (struct smb2_transform_hdr *)buf;
>         int rc = 0;
>
> -       if (pdu_length + 4 <
> -           sizeof(struct smb2_transform_hdr) + sizeof(struct smb2_hdr)) {
> +       if (buf_data_size < sizeof(struct smb2_hdr)) {

Could integer overflow occur when buf_data_size is initialized?
buf_data_size is initialized with "pdu_length + 4 -
sizeof(struct smb2_transform_hdr)".

There was the check that the pdu size is greater than at least
__SMB2_HEADER_STRUCTURE_SIZE at ksmbd_conn_handler_loop(),
But I can't find this check in the latest patch set.


>                 pr_err("Transform message is too small (%u)\n",
>                        pdu_length);
>                 return -ECONNABORTED;
>         }
>
> -       if (pdu_length + 4 <
> -           le32_to_cpu(tr_hdr->OriginalMessageSize) + sizeof(struct smb2_transform_hdr)) {
> +       if (buf_data_size < le32_to_cpu(tr_hdr->OriginalMessageSize)) {
>                 pr_err("Transform message is broken\n");
>                 return -ECONNABORTED;
>         }
> --
> 2.25.1
>


--
Thanks,
Hyunchul

Re: [PATCH 1/3] ksmbd: use buf_data_size instead of recalculation in smb3_decrypt_req()

[Namjae Jeon]

2021-10-04 17:38 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> 2021년 10월 3일 (일) 오후 1:31, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>>
>> Tom suggested to use buf_data_size that is already calculated, to verify
>> these offsets.
>>
>> Cc: Tom Talpey <tom@talpey.com>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>> Cc: Hyunchul Lee <hyc.lee@gmail.com>
>> Suggested-by: Tom Talpey <tom@talpey.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>  fs/ksmbd/smb2pdu.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index b06361313889..4d1be224dd8e 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -8457,15 +8457,13 @@ int smb3_decrypt_req(struct ksmbd_work *work)
>>         struct smb2_transform_hdr *tr_hdr = (struct smb2_transform_hdr
>> *)buf;
>>         int rc = 0;
>>
>> -       if (pdu_length + 4 <
>> -           sizeof(struct smb2_transform_hdr) + sizeof(struct smb2_hdr))
>> {
>> +       if (buf_data_size < sizeof(struct smb2_hdr)) {
>
> Could integer overflow occur when buf_data_size is initialized?
> buf_data_size is initialized with "pdu_length + 4 -
> sizeof(struct smb2_transform_hdr)".
overflow does not occur. See the comments below.
>
> There was the check that the pdu size is greater than at least
> __SMB2_HEADER_STRUCTURE_SIZE at ksmbd_conn_handler_loop(),
> But I can't find this check in the latest patch set.
Please check "ksmbd: add the check to vaildate if stream protocol
length exceeds maximum value". pdu_length will never exceed
MAX_STREAM_PROT_LEN(0x00FFFFFF).

Thanks!
>
>
>>                 pr_err("Transform message is too small (%u)\n",
>>                        pdu_length);
>>                 return -ECONNABORTED;
>>         }
>>
>> -       if (pdu_length + 4 <
>> -           le32_to_cpu(tr_hdr->OriginalMessageSize) + sizeof(struct
>> smb2_transform_hdr)) {
>> +       if (buf_data_size < le32_to_cpu(tr_hdr->OriginalMessageSize)) {
>>                 pr_err("Transform message is broken\n");
>>                 return -ECONNABORTED;
>>         }
>> --
>> 2.25.1
>>
>
>
> --
> Thanks,
> Hyunchul
>

Re: [PATCH 1/3] ksmbd: use buf_data_size instead of recalculation in smb3_decrypt_req()

[Hyunchul Lee]

2021년 10월 4일 (월) 오후 5:58, Namjae Jeon <linkinjeon@kernel.org>님이 작성:

>
> 2021-10-04 17:38 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> > 2021년 10월 3일 (일) 오후 1:31, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
> >>
> >> Tom suggested to use buf_data_size that is already calculated, to verify
> >> these offsets.
> >>
> >> Cc: Tom Talpey <tom@talpey.com>
> >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> >> Cc: Ralph Böhme <slow@samba.org>
> >> Cc: Steve French <smfrench@gmail.com>
> >> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> >> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> >> Suggested-by: Tom Talpey <tom@talpey.com>
> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> >> ---
> >>  fs/ksmbd/smb2pdu.c | 6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> >> index b06361313889..4d1be224dd8e 100644
> >> --- a/fs/ksmbd/smb2pdu.c
> >> +++ b/fs/ksmbd/smb2pdu.c
> >> @@ -8457,15 +8457,13 @@ int smb3_decrypt_req(struct ksmbd_work *work)
> >>         struct smb2_transform_hdr *tr_hdr = (struct smb2_transform_hdr
> >> *)buf;
> >>         int rc = 0;
> >>
> >> -       if (pdu_length + 4 <
> >> -           sizeof(struct smb2_transform_hdr) + sizeof(struct smb2_hdr))
> >> {
> >> +       if (buf_data_size < sizeof(struct smb2_hdr)) {
> >
> > Could integer overflow occur when buf_data_size is initialized?
> > buf_data_size is initialized with "pdu_length + 4 -
> > sizeof(struct smb2_transform_hdr)".
> overflow does not occur. See the comments below.
> >

Ah, I am worried that pdu_length + 4 is less than
sizeof(struct smb2_transform_hdr). And I can't find the check
that pdu size is enough before this function is called.


> > There was the check that the pdu size is greater than at least
> > __SMB2_HEADER_STRUCTURE_SIZE at ksmbd_conn_handler_loop(),
> > But I can't find this check in the latest patch set.
> Please check "ksmbd: add the check to vaildate if stream protocol
> length exceeds maximum value". pdu_length will never exceed
> MAX_STREAM_PROT_LEN(0x00FFFFFF).
>
> Thanks!
> >
> >
> >>                 pr_err("Transform message is too small (%u)\n",
> >>                        pdu_length);
> >>                 return -ECONNABORTED;
> >>         }
> >>
> >> -       if (pdu_length + 4 <
> >> -           le32_to_cpu(tr_hdr->OriginalMessageSize) + sizeof(struct
> >> smb2_transform_hdr)) {
> >> +       if (buf_data_size < le32_to_cpu(tr_hdr->OriginalMessageSize)) {
> >>                 pr_err("Transform message is broken\n");
> >>                 return -ECONNABORTED;
> >>         }
> >> --
> >> 2.25.1
> >>
> >
> >
> > --
> > Thanks,
> > Hyunchul
> >



--
Thanks,
Hyunchul

Re: [PATCH 1/3] ksmbd: use buf_data_size instead of recalculation in smb3_decrypt_req()

[Namjae Jeon]

2021-10-04 20:14 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> 2021년 10월 4일 (월) 오후 5:58, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
>>
>> 2021-10-04 17:38 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
>> > 2021년 10월 3일 (일) 오후 1:31, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>> >>
>> >> Tom suggested to use buf_data_size that is already calculated, to
>> >> verify
>> >> these offsets.
>> >>
>> >> Cc: Tom Talpey <tom@talpey.com>
>> >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> >> Cc: Ralph Böhme <slow@samba.org>
>> >> Cc: Steve French <smfrench@gmail.com>
>> >> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>> >> Cc: Hyunchul Lee <hyc.lee@gmail.com>
>> >> Suggested-by: Tom Talpey <tom@talpey.com>
>> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> >> ---
>> >>  fs/ksmbd/smb2pdu.c | 6 ++----
>> >>  1 file changed, 2 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> >> index b06361313889..4d1be224dd8e 100644
>> >> --- a/fs/ksmbd/smb2pdu.c
>> >> +++ b/fs/ksmbd/smb2pdu.c
>> >> @@ -8457,15 +8457,13 @@ int smb3_decrypt_req(struct ksmbd_work *work)
>> >>         struct smb2_transform_hdr *tr_hdr = (struct smb2_transform_hdr
>> >> *)buf;
>> >>         int rc = 0;
>> >>
>> >> -       if (pdu_length + 4 <
>> >> -           sizeof(struct smb2_transform_hdr) + sizeof(struct
>> >> smb2_hdr))
>> >> {
>> >> +       if (buf_data_size < sizeof(struct smb2_hdr)) {
>> >
>> > Could integer overflow occur when buf_data_size is initialized?
>> > buf_data_size is initialized with "pdu_length + 4 -
>> > sizeof(struct smb2_transform_hdr)".
>> overflow does not occur. See the comments below.
>> >
>
> Ah, I am worried that pdu_length + 4 is less than
> sizeof(struct smb2_transform_hdr). And I can't find the check
> that pdu size is enough before this function is called.
Got it, I will change data type of buf_data_size to signed on next version.

Thanks!
>
>
>> > There was the check that the pdu size is greater than at least
>> > __SMB2_HEADER_STRUCTURE_SIZE at ksmbd_conn_handler_loop(),
>> > But I can't find this check in the latest patch set.
>> Please check "ksmbd: add the check to vaildate if stream protocol
>> length exceeds maximum value". pdu_length will never exceed
>> MAX_STREAM_PROT_LEN(0x00FFFFFF).
>>
>> Thanks!
>> >
>> >
>> >>                 pr_err("Transform message is too small (%u)\n",
>> >>                        pdu_length);
>> >>                 return -ECONNABORTED;
>> >>         }
>> >>
>> >> -       if (pdu_length + 4 <
>> >> -           le32_to_cpu(tr_hdr->OriginalMessageSize) + sizeof(struct
>> >> smb2_transform_hdr)) {
>> >> +       if (buf_data_size < le32_to_cpu(tr_hdr->OriginalMessageSize))
>> >> {
>> >>                 pr_err("Transform message is broken\n");
>> >>                 return -ECONNABORTED;
>> >>         }
>> >> --
>> >> 2.25.1
>> >>
>> >
>> >
>> > --
>> > Thanks,
>> > Hyunchul
>> >
>
>
>
> --
> Thanks,
> Hyunchul
>

Re: [PATCH 3/3] ksmbd: fix oops from fuse driver

[Hyunchul Lee]

Looks good to me.
Acked-by: Hyunchul Lee <hyc.lee@gmail.com>

2021년 10월 3일 (일) 오후 1:31, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> Marios reported kernel oops from fuse driver when ksmbd call
> mark_inode_dirty(). This patch directly update ->i_ctime after removing
> mark_inode_ditry() and notify_change will put inode to dirty list.
>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Reported-by: Marios Makassikis <mmakassikis@freebox.fr>
> Tested-by: Marios Makassikis <mmakassikis@freebox.fr>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>  fs/ksmbd/smb2pdu.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 4d1be224dd8e..ed8324f9c2bd 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -5483,7 +5483,6 @@ static int set_file_basic_info(struct ksmbd_file *fp,
>                                struct ksmbd_share_config *share)
>  {
>         struct iattr attrs;
> -       struct timespec64 ctime;
>         struct file *filp;
>         struct inode *inode;
>         struct user_namespace *user_ns;
> @@ -5505,13 +5504,11 @@ static int set_file_basic_info(struct ksmbd_file *fp,
>                 attrs.ia_valid |= (ATTR_ATIME | ATTR_ATIME_SET);
>         }
>
> -       if (file_info->ChangeTime) {
> +       attrs.ia_valid |= ATTR_CTIME;
> +       if (file_info->ChangeTime)
>                 attrs.ia_ctime = ksmbd_NTtimeToUnix(file_info->ChangeTime);
> -               ctime = attrs.ia_ctime;
> -               attrs.ia_valid |= ATTR_CTIME;
> -       } else {
> -               ctime = inode->i_ctime;
> -       }
> +       else
> +               attrs.ia_ctime = inode->i_ctime;
>
>         if (file_info->LastWriteTime) {
>                 attrs.ia_mtime = ksmbd_NTtimeToUnix(file_info->LastWriteTime);
> @@ -5557,11 +5554,9 @@ static int set_file_basic_info(struct ksmbd_file *fp,
>                         return -EACCES;
>
>                 inode_lock(inode);
> +               inode->i_ctime = attrs.ia_ctime;
> +               attrs.ia_valid &= ~ATTR_CTIME;
>                 rc = notify_change(user_ns, dentry, &attrs, NULL);
> -               if (!rc) {
> -                       inode->i_ctime = ctime;
> -                       mark_inode_dirty(inode);
> -               }
>                 inode_unlock(inode);
>         }
>         return rc;
> --
> 2.25.1
>


-- 
Thanks,
Hyunchul