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 21:28, Tom Talpey 写道:
> 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 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:
Yes, if no judgement the length before dereferencing 'DialectCount',
OOB read will occur.>
>    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
After precompile the smb2pdu.c:
   typedef unsigned short __u16;     # include/uapi/asm-generic/int-ll64.h
   typedef __u16 __le16;             # include/uapi/linux/types.h

   struct validate_negotiate_info_req {
    __le32 Capabilities;
    __u8 Guid[16];
    __le16 SecurityMode;
    __le16 DialectCount;
    __le16 Dialects[];
   } __attribute__((__packed__));

The DialectCount is unsigned, can't be negative.

Even the 'DialectCount' is big enough to USHRT_MAX(65535), according
integer promotions, it also can't be overflow to negative since the
sizeof(__le16) is only 2.

So, I think the expression is correct now.
> 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; >