I'd like this to be updated to reflect the fact that selecting aes128-gcm enables both aes128-gcm and aes128-ccm, preferring gcm, and selecting aes128-ccm only enables CCM.

You might consider using smb_mbc_put_align(&sr->reply, 8), which makes sure that there's enough space in the MBC (this is what smb uses for other alignment cases)

typo ('reserved')

A short comment (or #define?) indicating where the 64 comes from would be nice

Is there a reason all of this function's callers ignore the return code?

Typo ('Label')

A comment (or #define) indicating where '89' comes from would be helpful

Does this imply a possible bug in KCF?

There are a few other 3.1.1 changes that appear to be mandatory that don't seem to be in this changeset.

3.3.4.4 Sending an Error Response

        The format for returning an error changed in 3.1.1. Error context is used by smb2_qinfo_sec (and maybe other query_info functions). See: 3.3.5.20.3 for an example. The current one is implemented by smb2sr_put_error_data().

        'NULL errors' also changed (i.e. 3.3.5.20.1 returns an error with bytecount set to 8, instead of 0)

3.3.5.4 Receiving an SMB2 NEGOTIATE Request
	store the entire ClientDialects array on the Connection (smb_session_t) object.

3.3.5.15.12 Handling a Validate Negotiate Info Request
	If 3.1.1 was negotiated, disconnect.
	Otherwise, compare the ClientDialects array in the request to the stored array (instead of re-calculating the best dialect and comparing to the negotiated dialect)
    

Then there are a few other 3.1.1 changes that we might not want to implement faithfully, but are important to consider:

3.3.5.7 Receiving an SMB2 TREE_CONNECT Request

        "If Connection.Dialect is "3.1.1" and Session.IsAnonymous and Session.IsGuest are set to FALSE and the request is not signed or not encrypted, then the server MUST disconnect the connection."

        I'm not sure how strict we want this to be. You might consider enforcing this only if the server (or both client and server?) enables signing. (The spec no longer even conceptualizes a server or client that doesn't support signing, so it's not clear if any 3.1.1 clients are capable of disabling signing)

3.3.5.2.9 Verifying the Session
        The spec says that servers that support 3.1.1 are not supposed to allow 'optional' encryption: either it's required, or its disabled. I suppose that's not something we want to follow, though.

These types are a bit odd. Several members (salt, _resrvd, data) are unused, and they're all only used in this one function. The encrypt_caps ctx is only one member.

You might consider just using locals for these.

Unfortunately, Sessions and Connections are not one-to-one; Even without Multichannel, you can have more than one session per connection (and with multichannel, more than one connection per session). The Connection.preauth_hashval should only contain the negotiate request and response, and then Session.preauth_hashval should be initialized to Connection.preauth_hashval, and then updated with the Session Setup request/responses.

The smb31_preauth_salt member is unused

Probably -- unfortunately, the almost complete lack of documentation on KCF (and the relevant ARC cases never made it out) makes it hard to know which one is wrong (this has caused me no end of frustration whenever I've touched tht code).
For reading of a crypto_data_t, I think cd_offset is supposed to be the effective offset into the blob of data (which could be a single raw buffer, split across mblk_ts or iovec_ts) and cd_length is the total length of all the data summed across all the buffers the crypto_data_t contains.
I'm less sure how they should be interpreted for writing -- e.g. should cd_offset be the current write position and cd_length the total length of all the buffers, or should cd_length be the current amount written. Though for either case, I can't point at anything to support my guesses.

Because not all errors are 'sticky' in a compound, should these be reset to 0 at some point during compound processing? I imagine we only want to include the error contexts for the particular request/command that created them. Similiarly, I suppose it's possible for multiple requests in a compound to need to use error contexts...

This should not be restricted to 3.1.1; the requirement is for 'servers that implement' 3.1.1, rather than for connections that negotiate 3.1.1. Same for the changes at line 915.
This function shouldn't ever be called on 3.1.1 - its a protocol error (3.3.5.15.12 "If Connection.Dialect is "3.1.1", the server MUST terminate the transport connection and free the Connection object")

Is there a reason this isn't just a simple compare i.e.

for (int i = 0 ; i < num_dialects; i++)
    if (dialects[i] != s->cli_dialects[i])
        goto drop;


It's simpler, and has fewer loops.
I worry that the 'hash' approach could be manipulated to obtain a value of 0, even when the arrays differ. I.e. If the 'real' string is 0x300, 0x302, An attacker could provide the values 0x212, 0x210, and this particular check would 'pass'.

With the dialect array comparison check (without the >= 3.1.1 check), the 'best dialect' check is no longer needed.

I don't think we're ever supposed to increase max_bytes this way.  (Encoding more than max_bytes can lead to panics elsewhere.)

If we're at the limit of the space allowed for, we should error out.

In general, we should verify that the offset actually exists within the current request. The way most places do that is by using smb_mbc_decodef(..., "#.", ...) to skip to the listed offset (See http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/smbsrv/smb2_tree_connect.c#51 for an example)
In cases where we already know the offset is valid (like the above NEG_CTX_INFO_OFFSET, where we've already decoded that area once before), it's not necessary, but if we haven't verified it yet, we should.

smb_mbc_put_align() modifies the buffer (it zero's the skipped data), which we probably don't want to do for the command buffer.
You could either make a 'read' equivalent of smb_mbc_put_align() that uses smb_mbc_decodef(), or just use smb_mbc_decodef() directly.
You could calculate ctx_end_off to 'round up' to an 8-byte boundary, then use smb_mbc_decodef(..., "#.", ...) to get your desired alignment, and still make sure that we're within the boundary of the request.

neg_in_ctxs and neg_out_ctxs are no longer valid after this function returns; perhaps you should NULL nego2->neg_{in,out}_ctxs after the send_reply()?

Since the "C" format calls MBC_SETUP() on the passed mbc, if errdata == &sr->raw_data (as it is for all of our calls), then chain_offset will be 0 here.
I don't believe we'd currently end up with more than one error context, so it's not a problem 'right now'. However, a comment to that effect, and maybe an ASSERT(errdata != &sr->raw_data) before this ASSERT, should help future devs keep that restriction in mind.

If we don't check the return code at call sites, could we log errors in this function? It'd be really nice to know 'right away' that something is going wrong here, instead of having to diagnose it from a vague 'access_denied' or signature error later.

'mech' needs to be kmem_free()'d here; You might consider just setting rc = -1 and break'ing.

Ship It!

Shouldn't this be "wbblllC"?

(likewise the args...)

Ship It!