Re: [PATCH v4 2/5] ksmbd: Remove the wrong message length check of FSCTL_VALIDATE_NEGOTIATE_INFO

["zhangxiaoxu (A)" <zhangxiaoxu5@huawei.com>]

在 2022/9/2 22:47, Namjae Jeon 写道:
> 2022-09-02 22:28 GMT+09:00, Tom Talpey <tom@talpey.com>:
>> On 9/1/2022 10:24 AM, Zhang Xiaoxu wrote:
>>> 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
> I think the fixes tag is wrong. Isn't the below correct?
> Fixes: f7db8fd03a4bc ("ksmbd: add validation in smb2_ioctl")

Before commit c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and
move its struct to smbfs_common"), the struct defined in fs/ksmbd/smb2pdu.h:

   struct validate_negotiate_info_req {
          __le32 Capabilities;
          __u8   Guid[SMB2_CLIENT_GUID_SIZE];
          __le16 SecurityMode;
          __le16 DialectCount;
          __le16 Dialects[1]; /* dialect (someday maybe list) client asked for */
   } __packed;

After this commit, the struct defined in fs/smbfs_common/smb2pdu.h:
   struct validate_negotiate_info_req {
          __le32 Capabilities;
          __u8   Guid[SMB2_CLIENT_GUID_SIZE];
          __le16 SecurityMode;
          __le16 DialectCount;
          __le16 Dialects[4]; /* BB expand this if autonegotiate > 4 dialects */
   } __packed;

The 'Dialects' array length from 1 change to 4.

Before commit c7803b05f74b, the message should contain at lease 1 dialects,
but after this commit, it changed to should contain at lease 4 dialects.

So the fixes tag should be c7803b05f74b.

> 
>>> 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;
>>> -
>>
>> This actually fixes a different bug, which the comment needs to mention.
>> The structure size includes 4 dialect slots, but the protocol does not
>> require the client to send all 4. So this allows the negotiation to not
>> fail. Which is good.
>>
>> But there are two other issues now.
>>
>> 1) The code no longer checks that a complete validate negotiate header
>> is present before dereferencing neg_req->DialectCount. This code will
>> fetch the DialectCount potentially beyond the end of an invalid short
>> packet:
>>
>>     fsctl_validate_negotiate_info():
>>
>>           if (in_buf_len < offsetof(struct validate_negotiate_info_req,
>> Dialects) +
>>                           le16_to_cpu(neg_req->DialectCount) *
>> sizeof(__le16))
>>                   return -EINVAL;
>>
>>           dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects,
>>                                                neg_req->DialectCount);
>>
>> 2) The DialectCount is an le16, which can be negative. A small invalid
>> negative value may pass this test and proceed to process the array,
>> which would be bad. This is an existing issue, but should be fixed
>> since you now need to correct the test anyway.
>>
>> Tom.
>>
>>
>>>    		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
>>>    			return -EINVAL; >
>>