2021-09-25 19:27 GMT+09:00, Hyunchul Lee <hyc.lee@xxxxxxxxx>:
> Looks good to me.
> Acked-by: Hyunchul Lee <hyc.lee@xxxxxxxxx>
>
> 2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@xxxxxxxxxx>님이 작성:
>>
>> When invalid data offset and data length in request,
>> ksmbd_smb2_check_message check strictly and doesn't allow to process such
>> requests.
>>
>> Cc: Tom Talpey <tom@xxxxxxxxxx>
>> Cc: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
>> Cc: Ralph Böhme <slow@xxxxxxxxx>
>> Cc: Steve French <smfrench@xxxxxxxxx>
>> Cc: Hyunchul Lee <hyc.lee@xxxxxxxxx>
>> Cc: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
>> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
>> ---
>> fs/ksmbd/smb2misc.c | 83 ++++++++++++++++++++-------------------------
>> 1 file changed, 37 insertions(+), 46 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
>> index 9aa46bb3e10d..697285e47522 100644
>> --- a/fs/ksmbd/smb2misc.c
>> +++ b/fs/ksmbd/smb2misc.c
>> @@ -83,15 +83,18 @@ static const bool
>> has_smb2_data_area[NUMBER_OF_SMB2_COMMANDS] = {
>> * Returns the pointer to the beginning of the data area. Length of the
>> data
>
> Do we need to change this comment about the return value?
Will update it on next version.
> Looks good to me except this.
> Acked-by: Hyunchul Lee <hyc.lee@xxxxxxxxx>
Thanks!
>
>
>> * area and the offset to it (from the beginning of the smb are also
>> returned.
>> */
>> -static char *smb2_get_data_area_len(int *off, int *len, struct smb2_hdr
>> *hdr)
>> +static int smb2_get_data_area_len(unsigned int *off, unsigned int *len,
>> + struct smb2_hdr *hdr)
>> {
>> + int ret = 0;
>> +
>> *off = 0;
>> *len = 0;
>>
>> /* error reqeusts do not have data area */
>> if (hdr->Status && hdr->Status != STATUS_MORE_PROCESSING_REQUIRED
>> &&
>> (((struct smb2_err_rsp *)hdr)->StructureSize) ==
>> SMB2_ERROR_STRUCTURE_SIZE2_LE)
>> - return NULL;
>> + return ret;
>>
>> /*
>> * Following commands have data areas so we have to get the
>> location
>> @@ -165,69 +168,52 @@ static char *smb2_get_data_area_len(int *off, int
>> *len, struct smb2_hdr *hdr)
>> case SMB2_IOCTL:
>> *off = le32_to_cpu(((struct smb2_ioctl_req
>> *)hdr)->InputOffset);
>> *len = le32_to_cpu(((struct smb2_ioctl_req
>> *)hdr)->InputCount);
>> -
>> break;
>> default:
>> ksmbd_debug(SMB, "no length check for command\n");
>> break;
>> }
>>
>> - /*
>> - * Invalid length or offset probably means data area is invalid,
>> but
>> - * we have little choice but to ignore the data area in this
>> case.
>> - */
>> if (*off > 4096) {
>> - ksmbd_debug(SMB, "offset %d too large, data area
>> ignored\n",
>> - *off);
>> - *len = 0;
>> - *off = 0;
>> - } else if (*off < 0) {
>> - ksmbd_debug(SMB,
>> - "negative offset %d to data invalid ignore
>> data area\n",
>> - *off);
>> - *off = 0;
>> - *len = 0;
>> - } else if (*len < 0) {
>> - ksmbd_debug(SMB,
>> - "negative data length %d invalid, data area
>> ignored\n",
>> - *len);
>> - *len = 0;
>> - } else if (*len > 128 * 1024) {
>> - ksmbd_debug(SMB, "data area larger than 128K: %d\n",
>> *len);
>> - *len = 0;
>> + ksmbd_debug(SMB, "offset %d too large\n", *off);
>> + ret = -EINVAL;
>> + } else if ((u64)*off + *len > MAX_STREAM_PROT_LEN) {
>> + ksmbd_debug(SMB, "Request is larger than maximum stream
>> protocol length(%u): %llu\n",
>> + MAX_STREAM_PROT_LEN, (u64)*off + *len);
>> + ret = -EINVAL;
>> }
>>
>> - /* return pointer to beginning of data area, ie offset from SMB
>> start */
>> - if ((*off != 0) && (*len != 0))
>> - return (char *)hdr + *off;
>> - else
>> - return NULL;
>> + return ret;
>> }
>>
>> /*
>> * Calculate the size of the SMB message based on the fixed header
>> * portion, the number of word parameters and the data portion of the
>> message.
>> */
>> -static unsigned int smb2_calc_size(void *buf)
>> +static int smb2_calc_size(void *buf, unsigned int *len)
>> {
>> struct smb2_pdu *pdu = (struct smb2_pdu *)buf;
>> struct smb2_hdr *hdr = &pdu->hdr;
>> - int offset; /* the offset from the beginning of SMB to data area
>> */
>> - int data_length; /* the length of the variable length data area
>> */
>> + unsigned int offset; /* the offset from the beginning of SMB to
>> data area */
>> + unsigned int data_length; /* the length of the variable length
>> data area */
>> + int ret;
>> +
>> /* Structure Size has already been checked to make sure it is 64
>> */
>> - int len = le16_to_cpu(hdr->StructureSize);
>> + *len = le16_to_cpu(hdr->StructureSize);
>>
>> /*
>> * StructureSize2, ie length of fixed parameter area has already
>> * been checked to make sure it is the correct length.
>> */
>> - len += le16_to_cpu(pdu->StructureSize2);
>> + *len += le16_to_cpu(pdu->StructureSize2);
>>
>> if (has_smb2_data_area[le16_to_cpu(hdr->Command)] == false)
>> goto calc_size_exit;
>>
>> - smb2_get_data_area_len(&offset, &data_length, hdr);
>> - ksmbd_debug(SMB, "SMB2 data length %d offset %d\n", data_length,
>> + ret = smb2_get_data_area_len(&offset, &data_length, hdr);
>> + if (ret)
>> + return ret;
>> + ksmbd_debug(SMB, "SMB2 data length %u offset %u\n", data_length,
>> offset);
>>
>> if (data_length > 0) {
>> @@ -237,16 +223,19 @@ static unsigned int smb2_calc_size(void *buf)
>> * for some commands, typically those with odd
>> StructureSize,
>> * so we must add one to the calculation.
>> */
>> - if (offset + 1 < len)
>> + if (offset + 1 < *len) {
>> ksmbd_debug(SMB,
>> - "data area offset %d overlaps SMB2
>> header %d\n",
>> - offset + 1, len);
>> - else
>> - len = offset + data_length;
>> + "data area offset %d overlaps SMB2
>> header %u\n",
>> + offset + 1, *len);
>> + return -EINVAL;
>> + }
>> +
>> + *len = offset + data_length;
>> }
>> +
>> calc_size_exit:
>> - ksmbd_debug(SMB, "SMB2 len %d\n", len);
>> - return len;
>> + ksmbd_debug(SMB, "SMB2 len %u\n", *len);
>> + return 0;
>> }
>>
>> static inline int smb2_query_info_req_len(struct smb2_query_info_req *h)
>> @@ -391,9 +380,11 @@ int ksmbd_smb2_check_message(struct ksmbd_work
>> *work)
>> return 1;
>> }
>>
>> - clc_len = smb2_calc_size(hdr);
>> + if (smb2_calc_size(hdr, &clc_len))
>> + return 1;
>> +
>> if (len != clc_len) {
>> - /* server can return one byte more due to implied bcc[0]
>> */
>> + /* client can return one byte more due to implied bcc[0]
>> */
>> if (clc_len == len + 1)
>> return 0;
>>
>> --
>> 2.25.1
>>
>
>
> --
> Thanks,
> Hyunchul
>
