Looks good to me. Thanks! Reviewed-by: Pavel Shilovsky <pshilov@xxxxxxxxxxxxx> -- Best regards, Pavel Shilovsky чт, 10 дек. 2020 г. в 18:47, Steve French <smfrench@xxxxxxxxx>: > > updated patch > > https://git.samba.org/?p=sfrench/cifs-2.6.git;a=commit;h=ced4cd5388614b39b6ef1e6d6e70c9b6044e3b43 > > On Thu, Dec 10, 2020 at 8:32 AM Tom Talpey <tom@xxxxxxxxxx> wrote: > > > > tl;dr - the issue here goes deeper than Salt length > > > > On 12/9/2020 11:24 PM, Steve French wrote: > > > In the negotiate protocol preauth context, the server is not required > > > to populate the salt (although it is recommended, and done by most > > > > I don't think "recommended" is correct. The salt is optional, and that's > > because not all hashes use salt. Of course, the protocol currently > > only defines one hash algorithm, which does. But that could change. > > So delete "it is recommended,", and "most". > > > > > servers) so do not warn on mount if the salt is not 32 bytes, but > > > instead simply check that the preauth context is the minimum size > > > and that the salt would not overflow the buffer length. > > > > Suggest ending at "so do not warn.", see following. > > > > > CC: Stable <stable@xxxxxxxxxxxxxxx> > > > Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx> > > > --- > > > fs/cifs/smb2pdu.c | 7 +++++-- > > > fs/cifs/smb2pdu.h | 14 +++++++++++--- > > > 2 files changed, 16 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > > > index acb72705062d..8d572dcf330a 100644 > > > --- a/fs/cifs/smb2pdu.c > > > +++ b/fs/cifs/smb2pdu.c > > > @@ -427,8 +427,8 @@ build_preauth_ctxt(struct smb2_preauth_neg_context > > > *pneg_ctxt) > > > pneg_ctxt->ContextType = SMB2_PREAUTH_INTEGRITY_CAPABILITIES; > > > pneg_ctxt->DataLength = cpu_to_le16(38); > > > pneg_ctxt->HashAlgorithmCount = cpu_to_le16(1); > > > - pneg_ctxt->SaltLength = cpu_to_le16(SMB311_SALT_SIZE); > > > - get_random_bytes(pneg_ctxt->Salt, SMB311_SALT_SIZE); > > > + pneg_ctxt->SaltLength = cpu_to_le16(SMB311_CLIENT_SALT_SIZE); > > > + get_random_bytes(pneg_ctxt->Salt, SMB311_CLIENT_SALT_SIZE); > > > > Changing the salt size define is ok, but it's important to keep clear > > that "32" is not specified by the protocol, it just happens to be > > what Windows chose, for SHA512. I think it's a fine idea to do the > > same, since we're all using the same hash algorithm. > > > > Why not simply code a constant 32? Or, make the define something > > like LINUX_SMB3_SHA512_SALT_LENGTH_CHOICE? > > > > > pneg_ctxt->HashAlgorithms = SMB2_PREAUTH_INTEGRITY_SHA512; > > > } > > > > > > @@ -566,6 +566,9 @@ static void decode_preauth_context(struct > > > smb2_preauth_neg_context *ctxt) > > > if (len < MIN_PREAUTH_CTXT_DATA_LEN) { > > > pr_warn_once("server sent bad preauth context\n"); > > > return; > > > + } else if (len < MIN_PREAUTH_CTXT_DATA_LEN + le16_to_cpu(ctxt->SaltLength)) { > > > + pr_warn_once("server sent invalid SaltLength\n"); > > > + return; > > > } > > > if (le16_to_cpu(ctxt->HashAlgorithmCount) != 1) > > > pr_warn_once("Invalid SMB3 hash algorithm count\n"); > > > > This comment applies to all three pr_warn's. > > > > Why are these here? The server is busted, sure, but the client is being > > a protocol validation test suite. And the information is really not very > > useful. How is a sysadmin supposed to respond? Finally, why are they > > pr_warn_once? If this server is broken, what about all the other ones, > > for which it suppresses the next warning? > > > > I say, if the negotiate response is invalid, abort the negotiate and > > forget throwing the book at it. No pr_warn's, or a simple generic one. > > > > > diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h > > > index fa57b03ca98c..de3127a6fc34 100644 > > > --- a/fs/cifs/smb2pdu.h > > > +++ b/fs/cifs/smb2pdu.h > > > @@ -333,12 +333,20 @@ struct smb2_neg_context { > > > /* Followed by array of data */ > > > } __packed; > > > > > > -#define SMB311_SALT_SIZE 32 > > > +#define SMB311_CLIENT_SALT_SIZE 32 > > > /* Hash Algorithm Types */ > > > #define SMB2_PREAUTH_INTEGRITY_SHA512 cpu_to_le16(0x0001) > > > #define SMB2_PREAUTH_HASH_SIZE 64 > > > > > > -#define MIN_PREAUTH_CTXT_DATA_LEN (SMB311_SALT_SIZE + 6) > > > +/* > > > + * SaltLength that the server send can be zero, so the only three required > > > > It can be "any value", including zero. > > > > > + * fields (all __le16) end up six bytes total, so the minimum context data len > > > + * in the response is six. > > > + * The three required are: HashAlgorithmCount, SaltLength, and 1 HashAlgorithm > > > + * Although most servers send a SaltLength of 32 bytes, technically it is > > > + * optional. > > > > "Required" is ambiguous. All three of these fields are in the header, > > so they're all required. It's their value that's important. Obviously > > HashAlgorithmCount must be >0. SaltLength can be any value. Suggest > > removing this last sentence entirely. > > > > > + */ > > > +#define MIN_PREAUTH_CTXT_DATA_LEN 6 > > > struct smb2_preauth_neg_context { > > > __le16 ContextType; /* 1 */ > > > __le16 DataLength; > > > @@ -346,7 +354,7 @@ struct smb2_preauth_neg_context { > > > __le16 HashAlgorithmCount; /* 1 */ > > > __le16 SaltLength; > > > __le16 HashAlgorithms; /* HashAlgorithms[0] since only one defined */ > > > - __u8 Salt[SMB311_SALT_SIZE]; > > > + __u8 Salt[SMB311_CLIENT_SALT_SIZE]; > > > > Incorrect! The protocol does not define this value. 32 is only an > > implementation behavior. This field is not constant, and may be 0. > > > > Tom. > > > > > } __packed; > > > > > > /* Encryption Algorithms Ciphers */ > > > > > > > > > > -- > Thanks, > > Steve
