2021-09-24 0:13 GMT+09:00, Tom Talpey <tom@xxxxxxxxxx>:
> On 9/22/2021 11:48 PM, Namjae Jeon wrote:
>> Ronnie reported invalid request buffer access in chained command when
>> inserting garbage value to NextCommand of compound request.
>> This patch add validation check to avoid this issue.
>>
>> Cc: Tom Talpey <tom@xxxxxxxxxx>
>> Cc: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
>> Cc: Ralph Böhme <slow@xxxxxxxxx>
>> Cc: Steve French <smfrench@xxxxxxxxx>
>> Reported-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
>> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
>> ---
>> v2:
>> - fix integer overflow from work->next_smb2_rcv_hdr_off.
>> v3:
>> - check next command offset and at least header size of next pdu at
>> the same time.
>> v4:
>> - add next_cmd variable not to avoid repeat conversion.
>>
>> fs/ksmbd/smb2pdu.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index 90f867b9d560..301558a04298 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -459,13 +459,21 @@ static void init_chained_smb2_rsp(struct ksmbd_work
>> *work)
>> bool is_chained_smb2_message(struct ksmbd_work *work)
>> {
>> struct smb2_hdr *hdr = work->request_buf;
>> - unsigned int len;
>> + unsigned int len, next_cmd;
>>
>> if (hdr->ProtocolId != SMB2_PROTO_NUMBER)
>> return false;
>>
>> hdr = ksmbd_req_buf_next(work);
>> - if (le32_to_cpu(hdr->NextCommand) > 0) {
>> + next_cmd = le32_to_cpu(hdr->NextCommand);
>> + if (next_cmd > 0) {
>> + if ((u64)work->next_smb2_rcv_hdr_off + next_cmd + 64 >
>
> The "64" is somewhat arbitrary and mysterious. Is this the size
> of the next command smb2_hdr? Why not code that, at least with
> a #define?
Okay, Will use macro.
>
>> + get_rfc1002_len(work->request_buf)) {
>> + pr_err("next command(%u) offset exceeds smb msg size\n",
>> + next_cmd);
>> + return false;
>> + }
>
> Hmm, well the header fits in the reminder of the message. What
> about the rest of the nextcommand smb2 request? It seems very
> odd, and more than a little risky, to be validating only one
> aspect of the nextcommand here.
There is a loop to check the rest of the nextcommand.
Please see do { } while (is_chained_smb2_message(work)); in server.
>
>> +
>> ksmbd_debug(SMB, "got SMB2 chained command\n");
>
> This, to me, is entirely needless debug splat.
The reason is ?
>
> Tom.
>
>> init_chained_smb2_rsp(work);
>> return true;
>>
>
