[PATCH v2 0/3] Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler

[PATCH v2 0/3] Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler

[Zhang Xiaoxu]

v1->v2: fix some bug in ksmbd when handle FSCTL_VALIDATE_NEGOTIATE_INFO
	message

Zhang Xiaoxu (3):
  cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message
  ksmbd: Remove the wrong message length check of
    FSCTL_VALIDATE_NEGOTIATE_INFO
  ksmbd: Fix wrong return value in smb2_ioctl() when wrong out_buf_len

 fs/cifs/smb2pdu.c  | 4 ++--
 fs/ksmbd/smb2pdu.c | 9 ++++-----
 2 files changed, 6 insertions(+), 7 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message

[Zhang Xiaoxu]

Commit d5c7076b772a ("smb3: add smb3.1.1 to default dialect list")
extend the dialects from 3 to 4, but forget to decrease the extended
length when specific the dialect, then the message length is larger
than expected.

This maybe leak some info through network because not initialize the
message body.

After apply this patch, the VALIDATE_NEGOTIATE_INFO message length is
reduced from 28 bytes to 26 bytes.

Fixes: d5c7076b772a ("smb3: add smb3.1.1 to default dialect list")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
Cc: <stable@vger.kernel.org>
---
 fs/cifs/smb2pdu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 128e44e57528..37f422eb3876 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1167,9 +1167,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 		pneg_inbuf->Dialects[0] =
 			cpu_to_le16(server->vals->protocol_id);
 		pneg_inbuf->DialectCount = cpu_to_le16(1);
-		/* structure is big enough for 3 dialects, sending only 1 */
+		/* structure is big enough for 4 dialects, sending only 1 */
 		inbuflen = sizeof(*pneg_inbuf) -
-				sizeof(pneg_inbuf->Dialects[0]) * 2;
+				sizeof(pneg_inbuf->Dialects[0]) * 3;
 	}
 
 	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
-- 
2.31.1

[PATCH v2 2/3] ksmbd: Remove the wrong message length check of FSCTL_VALIDATE_NEGOTIATE_INFO

[Zhang Xiaoxu]

The struct validate_negotiate_info_req change from variable-length array
to reguler array, but the message length check is unchanged.

The fsctl_validate_negotiate_info() already check the message length, so
remove it from smb2_ioctl().

Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and move its struct to smbfs_common")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
Cc: <stable@vger.kernel.org>
---
 fs/ksmbd/smb2pdu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index c49f65146ab3..c9f400bbb814 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7640,9 +7640,6 @@ int smb2_ioctl(struct ksmbd_work *work)
 			goto out;
 		}
 
-		if (in_buf_len < sizeof(struct validate_negotiate_info_req))
-			return -EINVAL;
-
 		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
 			return -EINVAL;
 
-- 
2.31.1

[PATCH v2 3/3] ksmbd: Fix wrong return value in smb2_ioctl() when wrong out_buf_len

[Zhang Xiaoxu]

When the out_buf_len is less than the size of struct
validate_negotiate_info_rsp, should goto out to initialize the
status in the response header.

Fixes: f7db8fd03a4bc ("ksmbd: add validation in smb2_ioctl")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
Cc: <stable@vger.kernel.org>
---
 fs/ksmbd/smb2pdu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index c9f400bbb814..7da0ec466887 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7640,8 +7640,10 @@ int smb2_ioctl(struct ksmbd_work *work)
 			goto out;
 		}
 
-		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
-			return -EINVAL;
+		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp)) {
+			ret = -EINVAL;
+			goto out;
+		}
 
 		ret = fsctl_validate_negotiate_info(conn,
 			(struct validate_negotiate_info_req *)&req->Buffer[0],
-- 
2.31.1

Re: [PATCH v2 0/3] Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler

[Tom Talpey]

On 8/31/2022 3:52 AM, Zhang Xiaoxu wrote:
> v1->v2: fix some bug in ksmbd when handle FSCTL_VALIDATE_NEGOTIATE_INFO
> 	message
> 
> Zhang Xiaoxu (3):
>    cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message
>    ksmbd: Remove the wrong message length check of
>      FSCTL_VALIDATE_NEGOTIATE_INFO
>    ksmbd: Fix wrong return value in smb2_ioctl() when wrong out_buf_len
> 
>   fs/cifs/smb2pdu.c  | 4 ++--
>   fs/ksmbd/smb2pdu.c | 9 ++++-----
>   2 files changed, 6 insertions(+), 7 deletions(-)
> 

Sorry but these are a NAK from me - they don't actually change
the definition to a variable-length array, they just attempt
to undo the broken "4", in several places. The real fix begins
in smbpdu.h in this line:
         __le16 Dialects[4]; --> Dialects[]

Also, the change to ksmbd is incorrect, it is critical to check
that the inbound buffer holds at least enough data to be able
to dereference the DialectCount, followed by a second check
that all the counted array elements are present. Also that
the outbound buffer is large enough to return a single dialect.

Tom.

Re: [PATCH v2 0/3] Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler

[zhangxiaoxu (A)]

在 2022/8/31 22:50, Tom Talpey 写道:
> On 8/31/2022 3:52 AM, Zhang Xiaoxu wrote:
>> v1->v2: fix some bug in ksmbd when handle FSCTL_VALIDATE_NEGOTIATE_INFO
>>     message
>>
>> Zhang Xiaoxu (3):
>>    cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message
>>    ksmbd: Remove the wrong message length check of
>>      FSCTL_VALIDATE_NEGOTIATE_INFO
>>    ksmbd: Fix wrong return value in smb2_ioctl() when wrong out_buf_len
>>
>>   fs/cifs/smb2pdu.c  | 4 ++--
>>   fs/ksmbd/smb2pdu.c | 9 ++++-----
>>   2 files changed, 6 insertions(+), 7 deletions(-)
>>
> 
> Sorry but these are a NAK from me - they don't actually change
> the definition to a variable-length array, they just attempt
> to undo the broken "4", in several places. The real fix begins
> in smbpdu.h in this line:
>          __le16 Dialects[4]; --> Dialects[]
This patchset just for fix the problems, the patches for refactor to
variable-length array is on the way.
> 
> Also, the change to ksmbd is incorrect, it is critical to check
> that the inbound buffer holds at least enough data to be able
> to dereference the DialectCount, followed by a second check
> that all the counted array elements are present. Also that
> the outbound buffer is large enough to return a single dialect.
The 'fsctl_validate_negotiate_info' function already check the inbound
buffer length, so remove the wrong inbound check in 'smb2_ioctl',
do you mean move the inbound check from 'fsctl_validate_negotiate_info'
to 'smb2_ioctl'?

7387 static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn,                                                                                                                                                                         │  ksmbd_spnego_negtokeninit.as
7388                                          struct validate_negotiate_info_req *neg_req,                                                                                                                                                     │  ksmbd_spnego_negtokeninit.as
7389                                          struct validate_negotiate_info_rsp *neg_rsp,                                                                                                                                                     │  ksmbd_spnego_negtokentarg.as
7390                                          unsigned int in_buf_len)                                                                                                                                                                         │  ksmbd_spnego_negtokentarg.as
7391 {                                                                                                                                                                                                                                         │  ksmbd_spnego_negtokentarg.as
7392         int ret = 0;                                                                                                                                                                                                                      │  ksmbd_spnego_negtokentarg.as
7393         int dialect;                                                                                                                                                                                                                      │  ksmbd_work.c
7394                                                                                                                                                                                                                                           │  ksmbd_work.h
7395         if (in_buf_len < offsetof(struct validate_negotiate_info_req, Dialects) +                                                                                                                                                         │  ksmbd_work.o
7396                         le16_to_cpu(neg_req->DialectCount) * sizeof(__le16))                                                                                                                                                              │  Makefile
7397                 return -EINVAL;
> 
> Tom.