2022-09-02 22:28 GMT+09:00, Tom Talpey <tom@xxxxxxxxxx>:
> 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")
>> move its struct to smbfs_common")
>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@xxxxxxxxxx>
>> Cc: <stable@xxxxxxxxxxxxxxx>
>> ---
>> 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; >
>
