[bug report] ksmbd: add validation in smb2 negotiate

[bug report] ksmbd: add validation in smb2 negotiate

[Dan Carpenter]

Hello Namjae Jeon,

This is a semi-automatic email about new static checker warnings.

The patch 442ff9ebeb01: "ksmbd: add validation in smb2 negotiate" 
from Sep 29, 2021, leads to the following Smatch complaint:

    fs/ksmbd/smb2pdu.c:8330 smb3_preauth_hash_rsp()
    error: we previously assumed 'conn->preauth_info' could be null (see line 8310)

fs/ksmbd/smb2pdu.c
  8309		if (le16_to_cpu(req->Command) == SMB2_NEGOTIATE_HE &&
  8310		    conn->preauth_info)
                    ^^^^^^^^^^^^^^^^^^
The patch adds a new check for "conn->preauth_info"

  8311			ksmbd_gen_preauth_integrity_hash(conn, (char *)rsp,
  8312							 conn->preauth_info->Preauth_HashValue);
  8313	
  8314		if (le16_to_cpu(rsp->Command) == SMB2_SESSION_SETUP_HE && sess) {
  8315			__u8 *hash_value;
  8316	
  8317			if (conn->binding) {
  8318				struct preauth_session *preauth_sess;
  8319	
  8320				preauth_sess = ksmbd_preauth_session_lookup(conn, sess->id);
  8321				if (!preauth_sess)
  8322					return;
  8323				hash_value = preauth_sess->Preauth_HashValue;
  8324			} else {
  8325				hash_value = sess->Preauth_HashValue;
  8326				if (!hash_value)
  8327					return;
  8328			}
  8329			ksmbd_gen_preauth_integrity_hash(conn, (char *)rsp,

But it's not checked inside the ksmbd_gen_preauth_integrity_hash()
function.

  8330							 hash_value);
  8331		}
  8332	}

regards,
dan carpenter

Re: [bug report] ksmbd: add validation in smb2 negotiate

[Namjae Jeon]

2021-10-04 19:45 GMT+09:00, Dan Carpenter <dan.carpenter@oracle.com>:
> Hello Namjae Jeon,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 442ff9ebeb01: "ksmbd: add validation in smb2 negotiate"
> from Sep 29, 2021, leads to the following Smatch complaint:
>
>     fs/ksmbd/smb2pdu.c:8330 smb3_preauth_hash_rsp()
>     error: we previously assumed 'conn->preauth_info' could be null (see
> line 8310)
>
> fs/ksmbd/smb2pdu.c
>   8309		if (le16_to_cpu(req->Command) == SMB2_NEGOTIATE_HE &&
>   8310		    conn->preauth_info)
>                     ^^^^^^^^^^^^^^^^^^
> The patch adds a new check for "conn->preauth_info"
>
>   8311			ksmbd_gen_preauth_integrity_hash(conn, (char *)rsp,
>   8312							 conn->preauth_info->Preauth_HashValue);
>   8313	
>   8314		if (le16_to_cpu(rsp->Command) == SMB2_SESSION_SETUP_HE && sess) {
>   8315			__u8 *hash_value;
>   8316	
>   8317			if (conn->binding) {
>   8318				struct preauth_session *preauth_sess;
>   8319	
>   8320				preauth_sess = ksmbd_preauth_session_lookup(conn, sess->id);
>   8321				if (!preauth_sess)
>   8322					return;
>   8323				hash_value = preauth_sess->Preauth_HashValue;
>   8324			} else {
>   8325				hash_value = sess->Preauth_HashValue;
>   8326				if (!hash_value)
>   8327					return;
>   8328			}
>   8329			ksmbd_gen_preauth_integrity_hash(conn, (char *)rsp,
>
Hi Dan,

First, Thanks for your report:)
> But it's not checked inside the ksmbd_gen_preauth_integrity_hash()
> function.
conn->preauth_info can not be NULL on smb3.1.1 session setup stage.

smb2 negotate(smb3.1.1, allocate conn->preauth_info) ->  smb3.1.1
session setup -> smb3_preauth_hash_rsp().

This is the check not to call ksmbd_gen_preauth_integrity_hash().
if (le16_to_cpu(rsp->Command) == SMB2_SESSION_SETUP_HE && sess) {

Let me know if I am missing something:)

Thanks!
>
>   8330							 hash_value);
>   8331		}
>   8332	}
>
> regards,
> dan carpenter
>