2023-05-18 17:11 GMT+09:00, Hex Rabbit <h3xrabbit@xxxxxxxxx>:
> I have decided to leave the modifications within the function that handles
> the
> corresponding context. The reason for this is that I believe consolidating
> the
> checks together can improve readability, also, moving them out would
> require
> us to read the size of the flex-sized array again in the corresponding
> function.
>
> What do you think?
Looks okay. Please send the patch to test to the list.
Thanks.
>
> below is the modified patch:
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 972176bff..0285c3f9e 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -849,13 +849,13 @@ static void assemble_neg_contexts(struct ksmbd_conn
> *conn,
>
> static __le32 decode_preauth_ctxt(struct ksmbd_conn *conn,
> struct smb2_preauth_neg_context *pneg_ctxt,
> - int len_of_ctxts)
> + int ctxt_len)
> {
> /*
> * sizeof(smb2_preauth_neg_context) assumes SMB311_SALT_SIZE Salt,
> * which may not be present. Only check for used HashAlgorithms[1].
> */
> - if (len_of_ctxts < MIN_PREAUTH_CTXT_DATA_LEN)
> + if (ctxt_len < MIN_PREAUTH_CTXT_DATA_LEN)
> return STATUS_INVALID_PARAMETER;
>
> if (pneg_ctxt->HashAlgorithms != SMB2_PREAUTH_INTEGRITY_SHA512)
> @@ -867,15 +867,23 @@ static __le32 decode_preauth_ctxt(struct ksmbd_conn
> *conn,
>
> static void decode_encrypt_ctxt(struct ksmbd_conn *conn,
> struct smb2_encryption_neg_context *pneg_ctxt,
> - int len_of_ctxts)
> + int ctxt_len)
> {
> - int cph_cnt = le16_to_cpu(pneg_ctxt->CipherCount);
> - int i, cphs_size = cph_cnt * sizeof(__le16);
> + int cph_cnt;
> + int i, cphs_size;
> +
> + if (sizeof(struct smb2_encryption_neg_context) > ctxt_len) {
> + pr_err("Invalid SMB2_ENCRYPTION_CAPABILITIES context size\n");
> + return;
> + }
>
> conn->cipher_type = 0;
>
> + cph_cnt = le16_to_cpu(pneg_ctxt->CipherCount);
> + cphs_size = cph_cnt * sizeof(__le16);
> +
> if (sizeof(struct smb2_encryption_neg_context) + cphs_size >
> - len_of_ctxts) {
> + ctxt_len) {
> pr_err("Invalid cipher count(%d)\n", cph_cnt);
> return;
> }
> @@ -923,15 +931,22 @@ static void decode_compress_ctxt(struct ksmbd_conn
> *conn,
>
> static void decode_sign_cap_ctxt(struct ksmbd_conn *conn,
> struct smb2_signing_capabilities *pneg_ctxt,
> - int len_of_ctxts)
> + int ctxt_len)
> {
> - int sign_algo_cnt = le16_to_cpu(pneg_ctxt->SigningAlgorithmCount);
> - int i, sign_alos_size = sign_algo_cnt * sizeof(__le16);
> + int sign_algo_cnt;
> + int i, sign_alos_size;
> +
> + if (sizeof(struct smb2_signing_capabilities) > ctxt_len) {
> + pr_err("Invalid SMB2_SIGNING_CAPABILITIES context length\n");
> + return;
> + }
>
> conn->signing_negotiated = false;
> + sign_algo_cnt = le16_to_cpu(pneg_ctxt->SigningAlgorithmCount);
> + sign_alos_size = sign_algo_cnt * sizeof(__le16);
>
> if (sizeof(struct smb2_signing_capabilities) + sign_alos_size >
> - len_of_ctxts) {
> + ctxt_len) {
> pr_err("Invalid signing algorithm count(%d)\n", sign_algo_cnt);
> return;
> }
> @@ -969,18 +984,16 @@ static __le32 deassemble_neg_contexts(struct
> ksmbd_conn *conn,
> len_of_ctxts = len_of_smb - offset;
>
> while (i++ < neg_ctxt_cnt) {
> - int clen;
> -
> - /* check that offset is not beyond end of SMB */
> - if (len_of_ctxts == 0)
> - break;
> + int clen, ctxt_len;
>
> if (len_of_ctxts < sizeof(struct smb2_neg_context))
> break;
>
> pctx = (struct smb2_neg_context *)((char *)pctx + offset);
> clen = le16_to_cpu(pctx->DataLength);
> - if (clen + sizeof(struct smb2_neg_context) > len_of_ctxts)
> + ctxt_len = clen + sizeof(struct smb2_neg_context);
> +
> + if (ctxt_len > len_of_ctxts)
> break;
>
> if (pctx->ContextType == SMB2_PREAUTH_INTEGRITY_CAPABILITIES) {
> @@ -991,7 +1004,7 @@ static __le32 deassemble_neg_contexts(struct
> ksmbd_conn *conn,
>
> status = decode_preauth_ctxt(conn,
> (struct smb2_preauth_neg_context *)pctx,
> - len_of_ctxts);
> + ctxt_len);
> if (status != STATUS_SUCCESS)
> break;
> } else if (pctx->ContextType == SMB2_ENCRYPTION_CAPABILITIES) {
> @@ -1002,7 +1015,7 @@ static __le32 deassemble_neg_contexts(struct
> ksmbd_conn *conn,
>
> decode_encrypt_ctxt(conn,
> (struct smb2_encryption_neg_context *)pctx,
> - len_of_ctxts);
> + ctxt_len);
> } else if (pctx->ContextType == SMB2_COMPRESSION_CAPABILITIES) {
> ksmbd_debug(SMB,
> "deassemble SMB2_COMPRESSION_CAPABILITIES context\n");
> @@ -1021,9 +1034,10 @@ static __le32 deassemble_neg_contexts(struct
> ksmbd_conn *conn,
> } else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES) {
> ksmbd_debug(SMB,
> "deassemble SMB2_SIGNING_CAPABILITIES context\n");
> +
> decode_sign_cap_ctxt(conn,
> (struct smb2_signing_capabilities *)pctx,
> - len_of_ctxts);
> + ctxt_len);
> }
>
> /* offsets must be 8 byte aligned */
> ---
>
> Namjae Jeon <linkinjeon@xxxxxxxxxx> 於 2023年5月18日 週四 下午2:36寫道：
>>
>> 2023-05-18 15:30 GMT+09:00, Hex Rabbit <h3xrabbit@xxxxxxxxx>:
>> >> You need to consider Ciphers flex-array size to validate ctxt_len. we
>> >> can get its size using CipherCount in smb2_encryption_neg_context.
>> >
>> > I'm not checking the flex-array size since both
>> > `decode_sign_cap_ctxt()`
>> > and `decode_encrypt_ctxt()` have done it, or should I move it out?
>> Yes, We can move it out. Thanks.
>> >
>> > ```
>> > if (sizeof(struct smb2_encryption_neg_context) + cphs_size >
>> > len_of_ctxts) {
>> > pr_err("Invalid cipher count(%d)\n", cph_cnt);
>> > return;
>> > }
>> > ```
>> >
>> > ```
>> > if (sizeof(struct smb2_signing_capabilities) + sign_alos_size >
>> > len_of_ctxts) {
>> > pr_err("Invalid signing algorithm count(%d)\n", sign_algo_cnt);
>> > return;
>> > }
>> > ```
>> >
>
