[v3,6/8] cifs: deprecate 'enable_negotiate_signing' module param

Deprecate enable_negotiate_signing module parameter as it's irrelevant
since the requested dialect and server support will dictate which
algorithm will actually be used.

Send a negotiate signing context on every SMB 3.1.1 negotiation now.
AES-CMAC will still be used instead, iff, SMB 3.0.x negotiated or
SMB 3.1.1 negotiated, but server doesn't support AES-GMAC.

Warn the user if, for whatever reason, the module was loaded with
'enable_negotiate_signing=0'.

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
 fs/cifs/cifsfs.c   | 14 +++++++++++---
 fs/cifs/cifsglob.h |  3 ++-
 fs/cifs/smb2pdu.c  | 11 ++++-------
 3 files changed, 17 insertions(+), 11 deletions(-)

could you fix these minor checkpatch warnings?

ERROR: code indent should use tabs where possible
#74: FILE: fs/cifs/cifsglob.h:2035:
+^I^I^I^I         XXX: deprecated, remove it at some point */$

WARNING: Block comments use * on subsequent lines
#74: FILE: fs/cifs/cifsglob.h:2035:
+extern bool enable_negotiate_signing; /* request use of faster (GMAC)
signing if available
+          XXX: deprecated, remove it at some point */

WARNING: Block comments use a trailing */ on a separate line
#74: FILE: fs/cifs/cifsglob.h:2035:
+          XXX: deprecated, remove it at some point */

total: 1 errors, 5 warnings, 58 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

NOTE: Whitespace errors detected.
      You may wish to use scripts/cleanpatch or scripts/cleanfile

0006-cifs-deprecate-enable_negotiate_signing-module-param.patch has
style problems, please review.

On Wed, Sep 28, 2022 at 8:57 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> Deprecate enable_negotiate_signing module parameter as it's irrelevant
> since the requested dialect and server support will dictate which
> algorithm will actually be used.
>
> Send a negotiate signing context on every SMB 3.1.1 negotiation now.
> AES-CMAC will still be used instead, iff, SMB 3.0.x negotiated or
> SMB 3.1.1 negotiated, but server doesn't support AES-GMAC.
>
> Warn the user if, for whatever reason, the module was loaded with
> 'enable_negotiate_signing=0'.
>
> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> ---
>  fs/cifs/cifsfs.c   | 14 +++++++++++---
>  fs/cifs/cifsglob.h |  3 ++-
>  fs/cifs/smb2pdu.c  | 11 ++++-------
>  3 files changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 8042d7280dec..c46dc9edf6ec 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -65,7 +65,7 @@ bool lookupCacheEnabled = true;
>  bool disable_legacy_dialects; /* false by default */
>  bool enable_gcm_256 = true;
>  bool require_gcm_256; /* false by default */
> -bool enable_negotiate_signing; /* false by default */
> +bool enable_negotiate_signing = true; /* deprecated -- always true now */
>  unsigned int global_secflags = CIFSSEC_DEF;
>  /* unsigned int ntlmv2_support = 0; */
>  unsigned int sign_CIFS_PDUs = 1;
> @@ -133,8 +133,12 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable requesting strongest (256 bit) GCM encr
>  module_param(require_gcm_256, bool, 0644);
>  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM encryption. Default: n/N/0");
>
> -module_param(enable_negotiate_signing, bool, 0644);
> -MODULE_PARM_DESC(enable_negotiate_signing, "Enable negotiating packet signing algorithm with server. Default: n/N/0");
> +/* XXX: remove this at some point */
> +module_param(enable_negotiate_signing, bool, 0);
> +MODULE_PARM_DESC(enable_negotiate_signing,
> +                "(deprecated) Enable negotiating packet signing algorithm with the server. "
> +                "This parameter is ignored as cifs.ko will always try to negotiate the signing "
> +                "algorithm on SMB 3.1.1 mounts.");
>
>  module_param(disable_legacy_dialects, bool, 0644);
>  MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
> @@ -1712,6 +1716,10 @@ init_cifs(void)
>                 goto out_init_cifs_idmap;
>         }
>
> +       if (!enable_negotiate_signing)
> +               pr_warn_once("ignoring deprecated module parameter 'enable_negotiate_signing=0', "
> +                            "will try to negotiate signing capabilities anyway...\n");
> +
>         return 0;
>
>  out_init_cifs_idmap:
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 81a8eff06467..cadde6c451e5 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -2031,7 +2031,8 @@ extern unsigned int global_secflags;      /* if on, session setup sent
>  extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
>  extern bool enable_gcm_256; /* allow optional negotiate of strongest signing (aes-gcm-256) */
>  extern bool require_gcm_256; /* require use of strongest signing (aes-gcm-256) */
> -extern bool enable_negotiate_signing; /* request use of faster (GMAC) signing if available */
> +extern bool enable_negotiate_signing; /* request use of faster (GMAC) signing if available
> +                                        XXX: deprecated, remove it at some point */
>  extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
>  extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
>  extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 2c2bf28382bc..6c1d58492b18 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -609,13 +609,10 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
>                 neg_context_count++;
>         }
>
> -       if (enable_negotiate_signing) {
> -               ctxt_len = build_signing_ctxt((struct smb2_signing_capabilities *)
> -                               pneg_ctxt);
> -               *total_len += ctxt_len;
> -               pneg_ctxt += ctxt_len;
> -               neg_context_count++;
> -       }
> +       ctxt_len = build_signing_ctxt((struct smb2_signing_capabilities *)pneg_ctxt);
> +       *total_len += ctxt_len;
> +       pneg_ctxt += ctxt_len;
> +       neg_context_count++;
>
>         /* check for and add transport_capabilities and signing capabilities */
>         req->NegotiateContextCount = cpu_to_le16(neg_context_count);
> --
> 2.35.3
>

On 09/29, Steve French wrote:
>could you fix these minor checkpatch warnings?

Ugh, I ran checkpatch but forgot to fix things, sorry about that. Will
resubmit.


Cheers,

Enzo