diff mbox series

[v6,3/5] ksmbd: Fix FSCTL_VALIDATE_NEGOTIATE_INFO message length check in smb2_ioctl()

Message ID 20220914021741.2672982-4-zhangxiaoxu5@huawei.com (mailing list archive)
State New, archived
Headers show
Series Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler | expand

Commit Message

Zhang Xiaoxu Sept. 14, 2022, 2:17 a.m. UTC
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.

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, 2 insertions(+), 1 deletion(-)

Comments

Tom Talpey Sept. 15, 2022, 6:45 p.m. UTC | #1
On 9/13/2022 7:17 PM, Zhang Xiaoxu wrote:
> 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.
> 
> 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, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index b56d7688ccf1..09ae601e64f9 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work)
>   			goto out;
>   		}
>   
> -		if (in_buf_len < sizeof(struct validate_negotiate_info_req)) {
> +		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
> +					  Dialects)) {
>   			ret = -EINVAL;
>   			goto out;
>   		}

Looks good, but see previous message regarding squashing this
with patch 2/5.

Reviewed-by: Tom Talpey <tom@talpey.com>
Namjae Jeon Sept. 16, 2022, 12:26 a.m. UTC | #2
2022-09-14 11:17 GMT+09:00, Zhang Xiaoxu <zhangxiaoxu5@huawei.com>:
> 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.
>
> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and
> move its struct to smbfs_common")
NACK. I am still thinking this tag is wrong.

> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> Cc: <stable@vger.kernel.org>
> ---
>  fs/ksmbd/smb2pdu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index b56d7688ccf1..09ae601e64f9 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work)
>  			goto out;
>  		}
>
> -		if (in_buf_len < sizeof(struct validate_negotiate_info_req)) {
> +		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
> +					  Dialects)) {
>  			ret = -EINVAL;
>  			goto out;
>  		}
> --
> 2.31.1
>
>
Zhang Xiaoxu Sept. 16, 2022, 11:20 a.m. UTC | #3
在 2022/9/16 8:26, Namjae Jeon 写道:
> 2022-09-14 11:17 GMT+09:00, Zhang Xiaoxu <zhangxiaoxu5@huawei.com>:
>> 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.
>>
>> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and
>> move its struct to smbfs_common")
> NACK. I am still thinking this tag is wrong.
Thanks for your comments.

In the v2, I remove the validation expression, and as your comments
in v2, duplicate check is unneed.

I add it back in v6 because tom's comments, we should ensure the req
has 'DialectCount', before validate the req has enough 'Dialects',
otherwise, dereferencing 'neg_req->DialectCount' will be OOB read.

Maybe the validation expression as below much better:
@@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work)
   ...
   if (in_buf_len < offsetof(struct validate_negotiate_info_req, Dialects[1]))

If even there's no one Dialects, 'the fsctl_validate_negotiate_info'
still return -EINVAL:

   fsctl_validate_negotiate_info
     ksmbd_lookup_dialect_by_id(neg_req->DialectCount=0)
       return BAD_PROT_ID
     ret = -EINVAL;

Same as before commit c7803b05f74b.

Could you give me more help about the fix tag.

Thanks.
Zhang Xiaoxu
> 
>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>>   fs/ksmbd/smb2pdu.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index b56d7688ccf1..09ae601e64f9 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work)
>>   			goto out;
>>   		}
>>
>> -		if (in_buf_len < sizeof(struct validate_negotiate_info_req)) {
>> +		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
>> +					  Dialects)) {
>>   			ret = -EINVAL;
>>   			goto out;
>>   		}
>> --
>> 2.31.1
>>
>>
Namjae Jeon Sept. 18, 2022, 11:45 p.m. UTC | #4
2022-09-16 20:20 GMT+09:00, zhangxiaoxu (A) <zhangxiaoxu5@huawei.com>:
>
>
> 在 2022/9/16 8:26, Namjae Jeon 写道:
>> 2022-09-14 11:17 GMT+09:00, Zhang Xiaoxu <zhangxiaoxu5@huawei.com>:
>>> 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.
>>>
>>> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and
>>> move its struct to smbfs_common")
>> NACK. I am still thinking this tag is wrong.
> Thanks for your comments.
>
> In the v2, I remove the validation expression, and as your comments
> in v2, duplicate check is unneed.
>
> I add it back in v6 because tom's comments, we should ensure the req
> has 'DialectCount', before validate the req has enough 'Dialects',
> otherwise, dereferencing 'neg_req->DialectCount' will be OOB read.
>
> Maybe the validation expression as below much better:
> @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work)
>    ...
>    if (in_buf_len < offsetof(struct validate_negotiate_info_req,
> Dialects[1]))
>
> If even there's no one Dialects, 'the fsctl_validate_negotiate_info'
> still return -EINVAL:
>
>    fsctl_validate_negotiate_info
>      ksmbd_lookup_dialect_by_id(neg_req->DialectCount=0)
>        return BAD_PROT_ID
>      ret = -EINVAL;
>
> Same as before commit c7803b05f74b.
Sorry, I don't understand what you say.
>
> Could you give me more help about the fix tag.
Please change a tag to commit f7db8fd03a4bc in this patch.

Thanks.
>
> Thanks.
> Zhang Xiaoxu
>>
>>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
>>> Cc: <stable@vger.kernel.org>
>>> ---
>>>   fs/ksmbd/smb2pdu.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>>> index b56d7688ccf1..09ae601e64f9 100644
>>> --- a/fs/ksmbd/smb2pdu.c
>>> +++ b/fs/ksmbd/smb2pdu.c
>>> @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work)
>>>   			goto out;
>>>   		}
>>>
>>> -		if (in_buf_len < sizeof(struct validate_negotiate_info_req)) {
>>> +		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
>>> +					  Dialects)) {
>>>   			ret = -EINVAL;
>>>   			goto out;
>>>   		}
>>> --
>>> 2.31.1
>>>
>>>
>
diff mbox series

Patch

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index b56d7688ccf1..09ae601e64f9 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7640,7 +7640,8 @@  int smb2_ioctl(struct ksmbd_work *work)
 			goto out;
 		}
 
-		if (in_buf_len < sizeof(struct validate_negotiate_info_req)) {
+		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
+					  Dialects)) {
 			ret = -EINVAL;
 			goto out;
 		}