All of lore.kernel.org
 help / color / mirror / Atom feed
From: Namjae Jeon <linkinjeon@kernel.org>
To: Hyunchul Lee <hyc.lee@gmail.com>
Cc: linux-cifs <linux-cifs@vger.kernel.org>,
	"Tom Talpey" <tom@talpey.com>,
	"Ronnie Sahlberg" <ronniesahlberg@gmail.com>,
	"Ralph Böhme" <slow@samba.org>,
	"Steve French" <smfrench@gmail.com>,
	"Sergey Senozhatsky" <senozhatsky@chromium.org>
Subject: Re: [PATCH 4/7] ksmbd: check strictly data area in ksmbd_smb2_check_message()
Date: Sat, 25 Sep 2021 19:46:07 +0900	[thread overview]
Message-ID: <CAKYAXd_Mjy44CTe=hPGuUUJd9T9u4UiJT6+Y-2n15pp4nnf=6Q@mail.gmail.com> (raw)
In-Reply-To: <CANFS6bYOJE12aA+UZpr6ajeu555a23N7F3OS1mgVZKX48AtwFg@mail.gmail.com>

2021-09-25 19:27 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> Looks good to me.
> Acked-by: Hyunchul Lee <hyc.lee@gmail.com>
>
> 2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>>
>> 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@talpey.com>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Cc: Hyunchul Lee <hyc.lee@gmail.com>
>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>  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@gmail.com>
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
>

  reply	other threads:[~2021-09-25 10:46 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-24  2:12 [PATCH 0/7] a bunch of patches that have not yet been reviewed Namjae Jeon
2021-09-24  2:12 ` [PATCH 1/7] ksmbd: add validation in smb2_ioctl Namjae Jeon
2021-09-25 10:16   ` Hyunchul Lee
2021-09-25 10:44     ` Namjae Jeon
2021-09-24  2:12 ` [PATCH 2/7] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
2021-09-25  8:13   ` Hyunchul Lee
2021-09-25  9:19     ` Namjae Jeon
2021-09-24  2:12 ` [PATCH 3/7] ksmbd: remove RFC1002 check in smb2 request Namjae Jeon
2021-09-25  8:18   ` Hyunchul Lee
2021-09-24  2:12 ` [PATCH 4/7] ksmbd: check strictly data area in ksmbd_smb2_check_message() Namjae Jeon
2021-09-25 10:27   ` Hyunchul Lee
2021-09-25 10:46     ` Namjae Jeon [this message]
2021-09-24  2:12 ` [PATCH 5/7] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value Namjae Jeon
2021-09-25  8:41   ` Hyunchul Lee
2021-09-25  9:24     ` Namjae Jeon
2021-09-24  2:12 ` [PATCH 6/7] ksmbd: fix invalid request buffer access in compound Namjae Jeon
2021-09-25  9:41   ` Hyunchul Lee
2021-09-24  2:12 ` [PATCH 7/7] ksmbd: add validation in smb2 negotiate Namjae Jeon
2021-09-24  4:58   ` Namjae Jeon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKYAXd_Mjy44CTe=hPGuUUJd9T9u4UiJT6+Y-2n15pp4nnf=6Q@mail.gmail.com' \
    --to=linkinjeon@kernel.org \
    --cc=hyc.lee@gmail.com \
    --cc=linux-cifs@vger.kernel.org \
    --cc=ronniesahlberg@gmail.com \
    --cc=senozhatsky@chromium.org \
    --cc=slow@samba.org \
    --cc=smfrench@gmail.com \
    --cc=tom@talpey.com \
    --subject='Re: [PATCH 4/7] ksmbd: check strictly data area in ksmbd_smb2_check_message()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.