diff mbox series

[1/1] SUNRPC: fix krb5p mount to provide large enough buffer in rq_rcvsize

Message ID 20200325210136.2826-1-olga.kornievskaia@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/1] SUNRPC: fix krb5p mount to provide large enough buffer in rq_rcvsize | expand

Commit Message

Olga Kornievskaia March 25, 2020, 9:01 p.m. UTC
From: Olga Kornievskaia <kolga@netapp.com>

Ever since commit 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing
reply buffer size"). It changed how "req->rq_rcvsize" is calculated. It
used to use au_cslack value which was nice and large and changed it to
au_rslack value which turns out to be too small.

Since 5.1, v3 mount with sec=krb5p fails against an Ontap server
because client's receive buffer it too small.

For gss krb5p, we need to account for the mic token in the verifier,
and the wrap token in the wrap token.

RFC 4121 defines:
mic token
Octet no   Name        Description
         --------------------------------------------------------------
         0..1     TOK_ID     Identification field.  Tokens emitted by
                             GSS_GetMIC() contain the hex value 04 04
                             expressed in big-endian order in this
                             field.
         2        Flags      Attributes field, as described in section
                             4.2.2.
         3..7     Filler     Contains five octets of hex value FF.
         8..15    SND_SEQ    Sequence number field in clear text,
                             expressed in big-endian order.
         16..last SGN_CKSUM  Checksum of the "to-be-signed" data and
                             octet 0..15, as described in section 4.2.4.

that's 16bytes (GSS_KRB5_TOK_HDR_LEN) + chksum

wrap token
Octet no   Name        Description
         --------------------------------------------------------------
          0..1     TOK_ID    Identification field.  Tokens emitted by
                             GSS_Wrap() contain the hex value 05 04
                             expressed in big-endian order in this
                             field.
          2        Flags     Attributes field, as described in section
                             4.2.2.
          3        Filler    Contains the hex value FF.
          4..5     EC        Contains the "extra count" field, in big-
                             endian order as described in section 4.2.3.
          6..7     RRC       Contains the "right rotation count" in big-
                             endian order, as described in section
                             4.2.5.
          8..15    SND_SEQ   Sequence number field in clear text,
                             expressed in big-endian order.
          16..last Data      Encrypted data for Wrap tokens with
                             confidentiality, or plaintext data followed
                             by the checksum for Wrap tokens without
                             confidentiality, as described in section
                             4.2.4.

Also 16bytes of header (GSS_KRB5_TOK_HDR_LEN), encrypted data, and cksum
(other things like padding)

RFC 3961 defines known cksum sizes:
Checksum type              sumtype        checksum         section or
                                value            size         reference
   ---------------------------------------------------------------------
   CRC32                            1               4           6.1.3
   rsa-md4                          2              16           6.1.2
   rsa-md4-des                      3              24           6.2.5
   des-mac                          4              16           6.2.7
   des-mac-k                        5               8           6.2.8
   rsa-md4-des-k                    6              16           6.2.6
   rsa-md5                          7              16           6.1.1
   rsa-md5-des                      8              24           6.2.4
   rsa-md5-des3                     9              24             ??
   sha1 (unkeyed)                  10              20             ??
   hmac-sha1-des3-kd               12              20            6.3
   hmac-sha1-des3                  13              20             ??
   sha1 (unkeyed)                  14              20             ??
   hmac-sha1-96-aes128             15              20         [KRB5-AES]
   hmac-sha1-96-aes256             16              20         [KRB5-AES]
   [reserved]                  0x8003               ?         [GSS-KRB5]

Linux kernel now mainly supports type 15,16 so max cksum size is 20bytes.
(GSS_KRB5_MAX_CKSUM_LEN)

Re-use already existing define of GSS_KRB5_MAX_SLACK_NEEDED that's used
for encoding the gss_wrap tokens (same tokens are used in reply).

Fixes: 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing reply buffer size")
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 net/sunrpc/auth_gss/auth_gss.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Trond Myklebust March 25, 2020, 9:33 p.m. UTC | #1
Hi Olga,

On Wed, 2020-03-25 at 17:01 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> Ever since commit 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing
> reply buffer size"). It changed how "req->rq_rcvsize" is calculated.
> It
> used to use au_cslack value which was nice and large and changed it
> to
> au_rslack value which turns out to be too small.
> 
> Since 5.1, v3 mount with sec=krb5p fails against an Ontap server
> because client's receive buffer it too small.
> 
> For gss krb5p, we need to account for the mic token in the verifier,
> and the wrap token in the wrap token.
> 
> RFC 4121 defines:
> mic token
> Octet no   Name        Description
>          ----------------------------------------------------------
> ----
>          0..1     TOK_ID     Identification field.  Tokens emitted by
>                              GSS_GetMIC() contain the hex value 04 04
>                              expressed in big-endian order in this
>                              field.
>          2        Flags      Attributes field, as described in
> section
>                              4.2.2.
>          3..7     Filler     Contains five octets of hex value FF.
>          8..15    SND_SEQ    Sequence number field in clear text,
>                              expressed in big-endian order.
>          16..last SGN_CKSUM  Checksum of the "to-be-signed" data and
>                              octet 0..15, as described in section
> 4.2.4.
> 
> that's 16bytes (GSS_KRB5_TOK_HDR_LEN) + chksum
> 
> wrap token
> Octet no   Name        Description
>          ----------------------------------------------------------
> ----
>           0..1     TOK_ID    Identification field.  Tokens emitted by
>                              GSS_Wrap() contain the hex value 05 04
>                              expressed in big-endian order in this
>                              field.
>           2        Flags     Attributes field, as described in
> section
>                              4.2.2.
>           3        Filler    Contains the hex value FF.
>           4..5     EC        Contains the "extra count" field, in
> big-
>                              endian order as described in section
> 4.2.3.
>           6..7     RRC       Contains the "right rotation count" in
> big-
>                              endian order, as described in section
>                              4.2.5.
>           8..15    SND_SEQ   Sequence number field in clear text,
>                              expressed in big-endian order.
>           16..last Data      Encrypted data for Wrap tokens with
>                              confidentiality, or plaintext data
> followed
>                              by the checksum for Wrap tokens without
>                              confidentiality, as described in section
>                              4.2.4.
> 
> Also 16bytes of header (GSS_KRB5_TOK_HDR_LEN), encrypted data, and
> cksum
> (other things like padding)
> 
> RFC 3961 defines known cksum sizes:
> Checksum type              sumtype        checksum         section or
>                                 value            size         referen
> ce
>    ----------------------------------------------------------------
> -----
>    CRC32                            1               4           6.1.3
>    rsa-md4                          2              16           6.1.2
>    rsa-md4-des                      3              24           6.2.5
>    des-mac                          4              16           6.2.7
>    des-mac-k                        5               8           6.2.8
>    rsa-md4-des-k                    6              16           6.2.6
>    rsa-md5                          7              16           6.1.1
>    rsa-md5-des                      8              24           6.2.4
>    rsa-md5-des3                     9              24             ??
>    sha1 (unkeyed)                  10              20             ??
>    hmac-sha1-des3-kd               12              20            6.3
>    hmac-sha1-des3                  13              20             ??
>    sha1 (unkeyed)                  14              20             ??
>    hmac-sha1-96-aes128             15              20         [KRB5-
> AES]
>    hmac-sha1-96-aes256             16              20         [KRB5-
> AES]
>    [reserved]                  0x8003               ?         [GSS-
> KRB5]
> 
> Linux kernel now mainly supports type 15,16 so max cksum size is
> 20bytes.
> (GSS_KRB5_MAX_CKSUM_LEN)
> 
> Re-use already existing define of GSS_KRB5_MAX_SLACK_NEEDED that's
> used
> for encoding the gss_wrap tokens (same tokens are used in reply).
> 
> Fixes: 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing reply
> buffer size")
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  net/sunrpc/auth_gss/auth_gss.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/auth_gss/auth_gss.c
> b/net/sunrpc/auth_gss/auth_gss.c
> index 24ca861..5a733a6 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -20,6 +20,7 @@
>  #include <linux/sunrpc/clnt.h>
>  #include <linux/sunrpc/auth.h>
>  #include <linux/sunrpc/auth_gss.h>
> +#include <linux/sunrpc/gss_krb5.h>
>  #include <linux/sunrpc/svcauth_gss.h>
>  #include <linux/sunrpc/gss_err.h>
>  #include <linux/workqueue.h>
> @@ -51,6 +52,8 @@
>  /* length of a krb5 verifier (48), plus data added before arguments
> when
>   * using integrity (two 4-byte integers): */
>  #define GSS_VERF_SLACK		100
> +/* covers lengths of gss_unwrap() extra kerberos mic and wrap token
> */
> +#define GSS_RESP_SLACK		(GSS_KRB5_MAX_SLACK_NEEDED <<
> 2)
>  
>  static DEFINE_HASHTABLE(gss_auth_hash_table, 4);
>  static DEFINE_SPINLOCK(gss_auth_hash_lock);
> @@ -1050,7 +1053,7 @@ static void gss_pipe_free(struct gss_pipe *p)
>  		goto err_put_mech;
>  	auth = &gss_auth->rpc_auth;
>  	auth->au_cslack = GSS_CRED_SLACK >> 2;
> -	auth->au_rslack = GSS_VERF_SLACK >> 2;
> +	auth->au_rslack = GSS_RESP_SLACK >> 2;
>  	auth->au_verfsize = GSS_VERF_SLACK >> 2;
>  	auth->au_ralign = GSS_VERF_SLACK >> 2;
>  	auth->au_flags = 0;

Is this a sufficient fix, though? It looks to me as if the above is
just an initial value that gets adjusted on the fly in
gss_unwrap_resp_priv():

        auth->au_rslack = auth->au_verfsize + 2 +
                          XDR_QUADLEN(savedlen - rcv_buf->len);
        auth->au_ralign = auth->au_verfsize + 2 +
                          XDR_QUADLEN(savedlen - rcv_buf->len);

My questions would be

- Are we sure that the above calculation (in gss_unwrap_resp_priv()) is
correct?
- Are we sure that the above calculation always gives the same answer
over time? We probably should not store a value that keeps changing.
Chuck Lever March 25, 2020, 9:34 p.m. UTC | #2
> On Mar 25, 2020, at 5:01 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
> 
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> Ever since commit 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing
> reply buffer size"). It changed how "req->rq_rcvsize" is calculated. It
> used to use au_cslack value which was nice and large and changed it to
> au_rslack value which turns out to be too small.
> 
> Since 5.1, v3 mount with sec=krb5p fails against an Ontap server
> because client's receive buffer it too small.
> 
> For gss krb5p, we need to account for the mic token in the verifier,
> and the wrap token in the wrap token.
> 
> RFC 4121 defines:
> mic token
> Octet no   Name        Description
>         --------------------------------------------------------------
>         0..1     TOK_ID     Identification field.  Tokens emitted by
>                             GSS_GetMIC() contain the hex value 04 04
>                             expressed in big-endian order in this
>                             field.
>         2        Flags      Attributes field, as described in section
>                             4.2.2.
>         3..7     Filler     Contains five octets of hex value FF.
>         8..15    SND_SEQ    Sequence number field in clear text,
>                             expressed in big-endian order.
>         16..last SGN_CKSUM  Checksum of the "to-be-signed" data and
>                             octet 0..15, as described in section 4.2.4.
> 
> that's 16bytes (GSS_KRB5_TOK_HDR_LEN) + chksum
> 
> wrap token
> Octet no   Name        Description
>         --------------------------------------------------------------
>          0..1     TOK_ID    Identification field.  Tokens emitted by
>                             GSS_Wrap() contain the hex value 05 04
>                             expressed in big-endian order in this
>                             field.
>          2        Flags     Attributes field, as described in section
>                             4.2.2.
>          3        Filler    Contains the hex value FF.
>          4..5     EC        Contains the "extra count" field, in big-
>                             endian order as described in section 4.2.3.
>          6..7     RRC       Contains the "right rotation count" in big-
>                             endian order, as described in section
>                             4.2.5.
>          8..15    SND_SEQ   Sequence number field in clear text,
>                             expressed in big-endian order.
>          16..last Data      Encrypted data for Wrap tokens with
>                             confidentiality, or plaintext data followed
>                             by the checksum for Wrap tokens without
>                             confidentiality, as described in section
>                             4.2.4.
> 
> Also 16bytes of header (GSS_KRB5_TOK_HDR_LEN), encrypted data, and cksum
> (other things like padding)
> 
> RFC 3961 defines known cksum sizes:
> Checksum type              sumtype        checksum         section or
>                                value            size         reference
>   ---------------------------------------------------------------------
>   CRC32                            1               4           6.1.3
>   rsa-md4                          2              16           6.1.2
>   rsa-md4-des                      3              24           6.2.5
>   des-mac                          4              16           6.2.7
>   des-mac-k                        5               8           6.2.8
>   rsa-md4-des-k                    6              16           6.2.6
>   rsa-md5                          7              16           6.1.1
>   rsa-md5-des                      8              24           6.2.4
>   rsa-md5-des3                     9              24             ??
>   sha1 (unkeyed)                  10              20             ??
>   hmac-sha1-des3-kd               12              20            6.3
>   hmac-sha1-des3                  13              20             ??
>   sha1 (unkeyed)                  14              20             ??
>   hmac-sha1-96-aes128             15              20         [KRB5-AES]
>   hmac-sha1-96-aes256             16              20         [KRB5-AES]
>   [reserved]                  0x8003               ?         [GSS-KRB5]
> 
> Linux kernel now mainly supports type 15,16 so max cksum size is 20bytes.
> (GSS_KRB5_MAX_CKSUM_LEN)
> 
> Re-use already existing define of GSS_KRB5_MAX_SLACK_NEEDED that's used
> for encoding the gss_wrap tokens (same tokens are used in reply).
> 
> Fixes: 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing reply buffer size")
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
> net/sunrpc/auth_gss/auth_gss.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> index 24ca861..5a733a6 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -20,6 +20,7 @@
> #include <linux/sunrpc/clnt.h>
> #include <linux/sunrpc/auth.h>
> #include <linux/sunrpc/auth_gss.h>
> +#include <linux/sunrpc/gss_krb5.h>
> #include <linux/sunrpc/svcauth_gss.h>
> #include <linux/sunrpc/gss_err.h>
> #include <linux/workqueue.h>
> @@ -51,6 +52,8 @@
> /* length of a krb5 verifier (48), plus data added before arguments when
>  * using integrity (two 4-byte integers): */
> #define GSS_VERF_SLACK		100
> +/* covers lengths of gss_unwrap() extra kerberos mic and wrap token */
> +#define GSS_RESP_SLACK		(GSS_KRB5_MAX_SLACK_NEEDED << 2)

GSS_KRB5_MAX_SLACK_NEEDED is already in bytes. Shouldn't need the "<< 2" here.


> static DEFINE_HASHTABLE(gss_auth_hash_table, 4);
> static DEFINE_SPINLOCK(gss_auth_hash_lock);
> @@ -1050,7 +1053,7 @@ static void gss_pipe_free(struct gss_pipe *p)
> 		goto err_put_mech;
> 	auth = &gss_auth->rpc_auth;
> 	auth->au_cslack = GSS_CRED_SLACK >> 2;
> -	auth->au_rslack = GSS_VERF_SLACK >> 2;
> +	auth->au_rslack = GSS_RESP_SLACK >> 2;
> 	auth->au_verfsize = GSS_VERF_SLACK >> 2;
> 	auth->au_ralign = GSS_VERF_SLACK >> 2;
> 	auth->au_flags = 0;
> -- 
> 1.8.3.1
> 

--
Chuck Lever
Chuck Lever March 25, 2020, 9:43 p.m. UTC | #3
> On Mar 25, 2020, at 5:33 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> Hi Olga,
> 
> On Wed, 2020-03-25 at 17:01 -0400, Olga Kornievskaia wrote:
>> From: Olga Kornievskaia <kolga@netapp.com>
>> 
>> Ever since commit 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing
>> reply buffer size"). It changed how "req->rq_rcvsize" is calculated.
>> It
>> used to use au_cslack value which was nice and large and changed it
>> to
>> au_rslack value which turns out to be too small.
>> 
>> Since 5.1, v3 mount with sec=krb5p fails against an Ontap server
>> because client's receive buffer it too small.
>> 
>> For gss krb5p, we need to account for the mic token in the verifier,
>> and the wrap token in the wrap token.
>> 
>> RFC 4121 defines:
>> mic token
>> Octet no   Name        Description
>>         ----------------------------------------------------------
>> ----
>>         0..1     TOK_ID     Identification field.  Tokens emitted by
>>                             GSS_GetMIC() contain the hex value 04 04
>>                             expressed in big-endian order in this
>>                             field.
>>         2        Flags      Attributes field, as described in
>> section
>>                             4.2.2.
>>         3..7     Filler     Contains five octets of hex value FF.
>>         8..15    SND_SEQ    Sequence number field in clear text,
>>                             expressed in big-endian order.
>>         16..last SGN_CKSUM  Checksum of the "to-be-signed" data and
>>                             octet 0..15, as described in section
>> 4.2.4.
>> 
>> that's 16bytes (GSS_KRB5_TOK_HDR_LEN) + chksum
>> 
>> wrap token
>> Octet no   Name        Description
>>         ----------------------------------------------------------
>> ----
>>          0..1     TOK_ID    Identification field.  Tokens emitted by
>>                             GSS_Wrap() contain the hex value 05 04
>>                             expressed in big-endian order in this
>>                             field.
>>          2        Flags     Attributes field, as described in
>> section
>>                             4.2.2.
>>          3        Filler    Contains the hex value FF.
>>          4..5     EC        Contains the "extra count" field, in
>> big-
>>                             endian order as described in section
>> 4.2.3.
>>          6..7     RRC       Contains the "right rotation count" in
>> big-
>>                             endian order, as described in section
>>                             4.2.5.
>>          8..15    SND_SEQ   Sequence number field in clear text,
>>                             expressed in big-endian order.
>>          16..last Data      Encrypted data for Wrap tokens with
>>                             confidentiality, or plaintext data
>> followed
>>                             by the checksum for Wrap tokens without
>>                             confidentiality, as described in section
>>                             4.2.4.
>> 
>> Also 16bytes of header (GSS_KRB5_TOK_HDR_LEN), encrypted data, and
>> cksum
>> (other things like padding)
>> 
>> RFC 3961 defines known cksum sizes:
>> Checksum type              sumtype        checksum         section or
>>                                value            size         referen
>> ce
>>   ----------------------------------------------------------------
>> -----
>>   CRC32                            1               4           6.1.3
>>   rsa-md4                          2              16           6.1.2
>>   rsa-md4-des                      3              24           6.2.5
>>   des-mac                          4              16           6.2.7
>>   des-mac-k                        5               8           6.2.8
>>   rsa-md4-des-k                    6              16           6.2.6
>>   rsa-md5                          7              16           6.1.1
>>   rsa-md5-des                      8              24           6.2.4
>>   rsa-md5-des3                     9              24             ??
>>   sha1 (unkeyed)                  10              20             ??
>>   hmac-sha1-des3-kd               12              20            6.3
>>   hmac-sha1-des3                  13              20             ??
>>   sha1 (unkeyed)                  14              20             ??
>>   hmac-sha1-96-aes128             15              20         [KRB5-
>> AES]
>>   hmac-sha1-96-aes256             16              20         [KRB5-
>> AES]
>>   [reserved]                  0x8003               ?         [GSS-
>> KRB5]
>> 
>> Linux kernel now mainly supports type 15,16 so max cksum size is
>> 20bytes.
>> (GSS_KRB5_MAX_CKSUM_LEN)
>> 
>> Re-use already existing define of GSS_KRB5_MAX_SLACK_NEEDED that's
>> used
>> for encoding the gss_wrap tokens (same tokens are used in reply).
>> 
>> Fixes: 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing reply
>> buffer size")
>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> ---
>> net/sunrpc/auth_gss/auth_gss.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/sunrpc/auth_gss/auth_gss.c
>> b/net/sunrpc/auth_gss/auth_gss.c
>> index 24ca861..5a733a6 100644
>> --- a/net/sunrpc/auth_gss/auth_gss.c
>> +++ b/net/sunrpc/auth_gss/auth_gss.c
>> @@ -20,6 +20,7 @@
>> #include <linux/sunrpc/clnt.h>
>> #include <linux/sunrpc/auth.h>
>> #include <linux/sunrpc/auth_gss.h>
>> +#include <linux/sunrpc/gss_krb5.h>
>> #include <linux/sunrpc/svcauth_gss.h>
>> #include <linux/sunrpc/gss_err.h>
>> #include <linux/workqueue.h>
>> @@ -51,6 +52,8 @@
>> /* length of a krb5 verifier (48), plus data added before arguments
>> when
>>  * using integrity (two 4-byte integers): */
>> #define GSS_VERF_SLACK		100
>> +/* covers lengths of gss_unwrap() extra kerberos mic and wrap token
>> */
>> +#define GSS_RESP_SLACK		(GSS_KRB5_MAX_SLACK_NEEDED <<
>> 2)
>> 
>> static DEFINE_HASHTABLE(gss_auth_hash_table, 4);
>> static DEFINE_SPINLOCK(gss_auth_hash_lock);
>> @@ -1050,7 +1053,7 @@ static void gss_pipe_free(struct gss_pipe *p)
>> 		goto err_put_mech;
>> 	auth = &gss_auth->rpc_auth;
>> 	auth->au_cslack = GSS_CRED_SLACK >> 2;
>> -	auth->au_rslack = GSS_VERF_SLACK >> 2;
>> +	auth->au_rslack = GSS_RESP_SLACK >> 2;
>> 	auth->au_verfsize = GSS_VERF_SLACK >> 2;
>> 	auth->au_ralign = GSS_VERF_SLACK >> 2;
>> 	auth->au_flags = 0;
> 
> Is this a sufficient fix, though? It looks to me as if the above is
> just an initial value that gets adjusted on the fly in
> gss_unwrap_resp_priv():
> 
>        auth->au_rslack = auth->au_verfsize + 2 +
>                          XDR_QUADLEN(savedlen - rcv_buf->len);
>        auth->au_ralign = auth->au_verfsize + 2 +
>                          XDR_QUADLEN(savedlen - rcv_buf->len);

That's correct. The GSS_*_SLACK value is a _sz value that is
the largest possible expected size of the extra GSS data.


> My questions would be
> 
> - Are we sure that the above calculation (in gss_unwrap_resp_priv()) is
> correct?

Yes, this is the correct computation.

We know this because if the initial au_rslack value is large
enough, then subsequent Replies have the correct amount of buffer
space and always succeed.


> - Are we sure that the above calculation always gives the same answer
> over time? We probably should not store a value that keeps changing.

It does not change after the first Reply. au_rslack is typically
adjusted downwards from the initial value based on the size of the
first received Reply.

Not setting these variables after the first Reply has been received
would be a minor optimization that could be done after Olga's fix.

--
Chuck Lever
Trond Myklebust March 25, 2020, 9:52 p.m. UTC | #4
On Wed, 2020-03-25 at 17:43 -0400, Chuck Lever wrote:
> > On Mar 25, 2020, at 5:33 PM, Trond Myklebust <
> > trondmy@hammerspace.com> wrote:
> > 
> > Hi Olga,
> > 
> > On Wed, 2020-03-25 at 17:01 -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > > 
> > > Ever since commit 2c94b8eca1a2 ("SUNRPC: Use au_rslack when
> > > computing
> > > reply buffer size"). It changed how "req->rq_rcvsize" is
> > > calculated.
> > > It
> > > used to use au_cslack value which was nice and large and changed
> > > it
> > > to
> > > au_rslack value which turns out to be too small.
> > > 
> > > Since 5.1, v3 mount with sec=krb5p fails against an Ontap server
> > > because client's receive buffer it too small.
> > > 
> > > For gss krb5p, we need to account for the mic token in the
> > > verifier,
> > > and the wrap token in the wrap token.
> > > 
> > > RFC 4121 defines:
> > > mic token
> > > Octet no   Name        Description
> > >         ---------------------------------------------------------
> > > -
> > > ----
> > >         0..1     TOK_ID     Identification field.  Tokens emitted
> > > by
> > >                             GSS_GetMIC() contain the hex value 04
> > > 04
> > >                             expressed in big-endian order in this
> > >                             field.
> > >         2        Flags      Attributes field, as described in
> > > section
> > >                             4.2.2.
> > >         3..7     Filler     Contains five octets of hex value FF.
> > >         8..15    SND_SEQ    Sequence number field in clear text,
> > >                             expressed in big-endian order.
> > >         16..last SGN_CKSUM  Checksum of the "to-be-signed" data
> > > and
> > >                             octet 0..15, as described in section
> > > 4.2.4.
> > > 
> > > that's 16bytes (GSS_KRB5_TOK_HDR_LEN) + chksum
> > > 
> > > wrap token
> > > Octet no   Name        Description
> > >         ---------------------------------------------------------
> > > -
> > > ----
> > >          0..1     TOK_ID    Identification field.  Tokens emitted
> > > by
> > >                             GSS_Wrap() contain the hex value 05
> > > 04
> > >                             expressed in big-endian order in this
> > >                             field.
> > >          2        Flags     Attributes field, as described in
> > > section
> > >                             4.2.2.
> > >          3        Filler    Contains the hex value FF.
> > >          4..5     EC        Contains the "extra count" field, in
> > > big-
> > >                             endian order as described in section
> > > 4.2.3.
> > >          6..7     RRC       Contains the "right rotation count"
> > > in
> > > big-
> > >                             endian order, as described in section
> > >                             4.2.5.
> > >          8..15    SND_SEQ   Sequence number field in clear text,
> > >                             expressed in big-endian order.
> > >          16..last Data      Encrypted data for Wrap tokens with
> > >                             confidentiality, or plaintext data
> > > followed
> > >                             by the checksum for Wrap tokens
> > > without
> > >                             confidentiality, as described in
> > > section
> > >                             4.2.4.
> > > 
> > > Also 16bytes of header (GSS_KRB5_TOK_HDR_LEN), encrypted data,
> > > and
> > > cksum
> > > (other things like padding)
> > > 
> > > RFC 3961 defines known cksum sizes:
> > > Checksum
> > > type              sumtype        checksum         section or
> > >                                value            size         refe
> > > ren
> > > ce
> > >   -------------------------------------------------------------
> > > ---
> > > -----
> > >  
> > > CRC32                            1               4           6.1.
> > > 3
> > >   rsa-
> > > md4                          2              16           6.1.2
> > >   rsa-md4-
> > > des                      3              24           6.2.5
> > >   des-
> > > mac                          4              16           6.2.7
> > >   des-mac-
> > > k                        5               8           6.2.8
> > >   rsa-md4-des-
> > > k                    6              16           6.2.6
> > >   rsa-
> > > md5                          7              16           6.1.1
> > >   rsa-md5-
> > > des                      8              24           6.2.4
> > >   rsa-md5-
> > > des3                     9              24             ??
> > >   sha1
> > > (unkeyed)                  10              20             ??
> > >   hmac-sha1-des3-
> > > kd               12              20            6.3
> > >   hmac-sha1-
> > > des3                  13              20             ??
> > >   sha1
> > > (unkeyed)                  14              20             ??
> > >   hmac-sha1-96-
> > > aes128             15              20         [KRB5-
> > > AES]
> > >   hmac-sha1-96-
> > > aes256             16              20         [KRB5-
> > > AES]
> > >  
> > > [reserved]                  0x8003               ?         [GSS-
> > > KRB5]
> > > 
> > > Linux kernel now mainly supports type 15,16 so max cksum size is
> > > 20bytes.
> > > (GSS_KRB5_MAX_CKSUM_LEN)
> > > 
> > > Re-use already existing define of GSS_KRB5_MAX_SLACK_NEEDED
> > > that's
> > > used
> > > for encoding the gss_wrap tokens (same tokens are used in reply).
> > > 
> > > Fixes: 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing reply
> > > buffer size")
> > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > ---
> > > net/sunrpc/auth_gss/auth_gss.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/net/sunrpc/auth_gss/auth_gss.c
> > > b/net/sunrpc/auth_gss/auth_gss.c
> > > index 24ca861..5a733a6 100644
> > > --- a/net/sunrpc/auth_gss/auth_gss.c
> > > +++ b/net/sunrpc/auth_gss/auth_gss.c
> > > @@ -20,6 +20,7 @@
> > > #include <linux/sunrpc/clnt.h>
> > > #include <linux/sunrpc/auth.h>
> > > #include <linux/sunrpc/auth_gss.h>
> > > +#include <linux/sunrpc/gss_krb5.h>
> > > #include <linux/sunrpc/svcauth_gss.h>
> > > #include <linux/sunrpc/gss_err.h>
> > > #include <linux/workqueue.h>
> > > @@ -51,6 +52,8 @@
> > > /* length of a krb5 verifier (48), plus data added before
> > > arguments
> > > when
> > >  * using integrity (two 4-byte integers): */
> > > #define GSS_VERF_SLACK		100
> > > +/* covers lengths of gss_unwrap() extra kerberos mic and wrap
> > > token
> > > */
> > > +#define GSS_RESP_SLACK		(GSS_KRB5_MAX_SLACK_NEEDED <<
> > > 2)
> > > 
> > > static DEFINE_HASHTABLE(gss_auth_hash_table, 4);
> > > static DEFINE_SPINLOCK(gss_auth_hash_lock);
> > > @@ -1050,7 +1053,7 @@ static void gss_pipe_free(struct gss_pipe
> > > *p)
> > > 		goto err_put_mech;
> > > 	auth = &gss_auth->rpc_auth;
> > > 	auth->au_cslack = GSS_CRED_SLACK >> 2;
> > > -	auth->au_rslack = GSS_VERF_SLACK >> 2;
> > > +	auth->au_rslack = GSS_RESP_SLACK >> 2;
> > > 	auth->au_verfsize = GSS_VERF_SLACK >> 2;
> > > 	auth->au_ralign = GSS_VERF_SLACK >> 2;
> > > 	auth->au_flags = 0;
> > 
> > Is this a sufficient fix, though? It looks to me as if the above is
> > just an initial value that gets adjusted on the fly in
> > gss_unwrap_resp_priv():
> > 
> >        auth->au_rslack = auth->au_verfsize + 2 +
> >                          XDR_QUADLEN(savedlen - rcv_buf->len);
> >        auth->au_ralign = auth->au_verfsize + 2 +
> >                          XDR_QUADLEN(savedlen - rcv_buf->len);
> 
> That's correct. The GSS_*_SLACK value is a _sz value that is
> the largest possible expected size of the extra GSS data.
> 
> 
> > My questions would be
> > 
> > - Are we sure that the above calculation (in
> > gss_unwrap_resp_priv()) is
> > correct?
> 
> Yes, this is the correct computation.
> 
> We know this because if the initial au_rslack value is large
> enough, then subsequent Replies have the correct amount of buffer
> space and always succeed.
> 
> 
> > - Are we sure that the above calculation always gives the same
> > answer
> > over time? We probably should not store a value that keeps
> > changing.
> 
> It does not change after the first Reply. au_rslack is typically
> adjusted downwards from the initial value based on the size of the
> first received Reply.
> 
> Not setting these variables after the first Reply has been received
> would be a minor optimization that could be done after Olga's fix.
> 

OK. So you're both saying that as long as the initial value is correct,
we're good for the duration of the GSS session? Fair enough, I'll apply
this patch for 5.7 then.

Let's also fix up the above in a separate patch to not keep setting
auth->au_rslack / auth->au_ralign if their values are not changing.
That should prevent unnecessary cache line bouncing.
Chuck Lever March 25, 2020, 9:55 p.m. UTC | #5
> On Mar 25, 2020, at 5:52 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Wed, 2020-03-25 at 17:43 -0400, Chuck Lever wrote:
>>> On Mar 25, 2020, at 5:33 PM, Trond Myklebust <
>>> trondmy@hammerspace.com> wrote:
>>> 
>>> Hi Olga,
>>> 
>>> On Wed, 2020-03-25 at 17:01 -0400, Olga Kornievskaia wrote:
>>>> From: Olga Kornievskaia <kolga@netapp.com>
>>>> 
>>>> Ever since commit 2c94b8eca1a2 ("SUNRPC: Use au_rslack when
>>>> computing
>>>> reply buffer size"). It changed how "req->rq_rcvsize" is
>>>> calculated.
>>>> It
>>>> used to use au_cslack value which was nice and large and changed
>>>> it
>>>> to
>>>> au_rslack value which turns out to be too small.
>>>> 
>>>> Since 5.1, v3 mount with sec=krb5p fails against an Ontap server
>>>> because client's receive buffer it too small.
>>>> 
>>>> For gss krb5p, we need to account for the mic token in the
>>>> verifier,
>>>> and the wrap token in the wrap token.
>>>> 
>>>> RFC 4121 defines:
>>>> mic token
>>>> Octet no   Name        Description
>>>>        ---------------------------------------------------------
>>>> -
>>>> ----
>>>>        0..1     TOK_ID     Identification field.  Tokens emitted
>>>> by
>>>>                            GSS_GetMIC() contain the hex value 04
>>>> 04
>>>>                            expressed in big-endian order in this
>>>>                            field.
>>>>        2        Flags      Attributes field, as described in
>>>> section
>>>>                            4.2.2.
>>>>        3..7     Filler     Contains five octets of hex value FF.
>>>>        8..15    SND_SEQ    Sequence number field in clear text,
>>>>                            expressed in big-endian order.
>>>>        16..last SGN_CKSUM  Checksum of the "to-be-signed" data
>>>> and
>>>>                            octet 0..15, as described in section
>>>> 4.2.4.
>>>> 
>>>> that's 16bytes (GSS_KRB5_TOK_HDR_LEN) + chksum
>>>> 
>>>> wrap token
>>>> Octet no   Name        Description
>>>>        ---------------------------------------------------------
>>>> -
>>>> ----
>>>>         0..1     TOK_ID    Identification field.  Tokens emitted
>>>> by
>>>>                            GSS_Wrap() contain the hex value 05
>>>> 04
>>>>                            expressed in big-endian order in this
>>>>                            field.
>>>>         2        Flags     Attributes field, as described in
>>>> section
>>>>                            4.2.2.
>>>>         3        Filler    Contains the hex value FF.
>>>>         4..5     EC        Contains the "extra count" field, in
>>>> big-
>>>>                            endian order as described in section
>>>> 4.2.3.
>>>>         6..7     RRC       Contains the "right rotation count"
>>>> in
>>>> big-
>>>>                            endian order, as described in section
>>>>                            4.2.5.
>>>>         8..15    SND_SEQ   Sequence number field in clear text,
>>>>                            expressed in big-endian order.
>>>>         16..last Data      Encrypted data for Wrap tokens with
>>>>                            confidentiality, or plaintext data
>>>> followed
>>>>                            by the checksum for Wrap tokens
>>>> without
>>>>                            confidentiality, as described in
>>>> section
>>>>                            4.2.4.
>>>> 
>>>> Also 16bytes of header (GSS_KRB5_TOK_HDR_LEN), encrypted data,
>>>> and
>>>> cksum
>>>> (other things like padding)
>>>> 
>>>> RFC 3961 defines known cksum sizes:
>>>> Checksum
>>>> type              sumtype        checksum         section or
>>>>                               value            size         refe
>>>> ren
>>>> ce
>>>>  -------------------------------------------------------------
>>>> ---
>>>> -----
>>>> 
>>>> CRC32                            1               4           6.1.
>>>> 3
>>>>  rsa-
>>>> md4                          2              16           6.1.2
>>>>  rsa-md4-
>>>> des                      3              24           6.2.5
>>>>  des-
>>>> mac                          4              16           6.2.7
>>>>  des-mac-
>>>> k                        5               8           6.2.8
>>>>  rsa-md4-des-
>>>> k                    6              16           6.2.6
>>>>  rsa-
>>>> md5                          7              16           6.1.1
>>>>  rsa-md5-
>>>> des                      8              24           6.2.4
>>>>  rsa-md5-
>>>> des3                     9              24             ??
>>>>  sha1
>>>> (unkeyed)                  10              20             ??
>>>>  hmac-sha1-des3-
>>>> kd               12              20            6.3
>>>>  hmac-sha1-
>>>> des3                  13              20             ??
>>>>  sha1
>>>> (unkeyed)                  14              20             ??
>>>>  hmac-sha1-96-
>>>> aes128             15              20         [KRB5-
>>>> AES]
>>>>  hmac-sha1-96-
>>>> aes256             16              20         [KRB5-
>>>> AES]
>>>> 
>>>> [reserved]                  0x8003               ?         [GSS-
>>>> KRB5]
>>>> 
>>>> Linux kernel now mainly supports type 15,16 so max cksum size is
>>>> 20bytes.
>>>> (GSS_KRB5_MAX_CKSUM_LEN)
>>>> 
>>>> Re-use already existing define of GSS_KRB5_MAX_SLACK_NEEDED
>>>> that's
>>>> used
>>>> for encoding the gss_wrap tokens (same tokens are used in reply).
>>>> 
>>>> Fixes: 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing reply
>>>> buffer size")
>>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>>> ---
>>>> net/sunrpc/auth_gss/auth_gss.c | 5 ++++-
>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/net/sunrpc/auth_gss/auth_gss.c
>>>> b/net/sunrpc/auth_gss/auth_gss.c
>>>> index 24ca861..5a733a6 100644
>>>> --- a/net/sunrpc/auth_gss/auth_gss.c
>>>> +++ b/net/sunrpc/auth_gss/auth_gss.c
>>>> @@ -20,6 +20,7 @@
>>>> #include <linux/sunrpc/clnt.h>
>>>> #include <linux/sunrpc/auth.h>
>>>> #include <linux/sunrpc/auth_gss.h>
>>>> +#include <linux/sunrpc/gss_krb5.h>
>>>> #include <linux/sunrpc/svcauth_gss.h>
>>>> #include <linux/sunrpc/gss_err.h>
>>>> #include <linux/workqueue.h>
>>>> @@ -51,6 +52,8 @@
>>>> /* length of a krb5 verifier (48), plus data added before
>>>> arguments
>>>> when
>>>> * using integrity (two 4-byte integers): */
>>>> #define GSS_VERF_SLACK		100
>>>> +/* covers lengths of gss_unwrap() extra kerberos mic and wrap
>>>> token
>>>> */
>>>> +#define GSS_RESP_SLACK		(GSS_KRB5_MAX_SLACK_NEEDED <<
>>>> 2)
>>>> 
>>>> static DEFINE_HASHTABLE(gss_auth_hash_table, 4);
>>>> static DEFINE_SPINLOCK(gss_auth_hash_lock);
>>>> @@ -1050,7 +1053,7 @@ static void gss_pipe_free(struct gss_pipe
>>>> *p)
>>>> 		goto err_put_mech;
>>>> 	auth = &gss_auth->rpc_auth;
>>>> 	auth->au_cslack = GSS_CRED_SLACK >> 2;
>>>> -	auth->au_rslack = GSS_VERF_SLACK >> 2;
>>>> +	auth->au_rslack = GSS_RESP_SLACK >> 2;
>>>> 	auth->au_verfsize = GSS_VERF_SLACK >> 2;
>>>> 	auth->au_ralign = GSS_VERF_SLACK >> 2;
>>>> 	auth->au_flags = 0;
>>> 
>>> Is this a sufficient fix, though? It looks to me as if the above is
>>> just an initial value that gets adjusted on the fly in
>>> gss_unwrap_resp_priv():
>>> 
>>>       auth->au_rslack = auth->au_verfsize + 2 +
>>>                         XDR_QUADLEN(savedlen - rcv_buf->len);
>>>       auth->au_ralign = auth->au_verfsize + 2 +
>>>                         XDR_QUADLEN(savedlen - rcv_buf->len);
>> 
>> That's correct. The GSS_*_SLACK value is a _sz value that is
>> the largest possible expected size of the extra GSS data.
>> 
>> 
>>> My questions would be
>>> 
>>> - Are we sure that the above calculation (in
>>> gss_unwrap_resp_priv()) is
>>> correct?
>> 
>> Yes, this is the correct computation.
>> 
>> We know this because if the initial au_rslack value is large
>> enough, then subsequent Replies have the correct amount of buffer
>> space and always succeed.
>> 
>> 
>>> - Are we sure that the above calculation always gives the same
>>> answer
>>> over time? We probably should not store a value that keeps
>>> changing.
>> 
>> It does not change after the first Reply. au_rslack is typically
>> adjusted downwards from the initial value based on the size of the
>> first received Reply.
>> 
>> Not setting these variables after the first Reply has been received
>> would be a minor optimization that could be done after Olga's fix.
>> 
> 
> OK. So you're both saying that as long as the initial value is correct,
> we're good for the duration of the GSS session? Fair enough, I'll apply
> this patch for 5.7 then.

Thanks, please see my one-line correction. I think the definition of
GSS_RESP_SLACK should not include the "<< 2", I would like Olga to
confirm.


> Let's also fix up the above in a separate patch to not keep setting
> auth->au_rslack / auth->au_ralign if their values are not changing.
> That should prevent unnecessary cache line bouncing.

I volunteer, unless Olga wants to take it.


--
Chuck Lever
Olga Kornievskaia March 26, 2020, 12:04 p.m. UTC | #6
On Wed, Mar 25, 2020 at 5:34 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>
> > On Mar 25, 2020, at 5:01 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
> >
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > Ever since commit 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing
> > reply buffer size"). It changed how "req->rq_rcvsize" is calculated. It
> > used to use au_cslack value which was nice and large and changed it to
> > au_rslack value which turns out to be too small.
> >
> > Since 5.1, v3 mount with sec=krb5p fails against an Ontap server
> > because client's receive buffer it too small.
> >
> > For gss krb5p, we need to account for the mic token in the verifier,
> > and the wrap token in the wrap token.
> >
> > RFC 4121 defines:
> > mic token
> > Octet no   Name        Description
> >         --------------------------------------------------------------
> >         0..1     TOK_ID     Identification field.  Tokens emitted by
> >                             GSS_GetMIC() contain the hex value 04 04
> >                             expressed in big-endian order in this
> >                             field.
> >         2        Flags      Attributes field, as described in section
> >                             4.2.2.
> >         3..7     Filler     Contains five octets of hex value FF.
> >         8..15    SND_SEQ    Sequence number field in clear text,
> >                             expressed in big-endian order.
> >         16..last SGN_CKSUM  Checksum of the "to-be-signed" data and
> >                             octet 0..15, as described in section 4.2.4.
> >
> > that's 16bytes (GSS_KRB5_TOK_HDR_LEN) + chksum
> >
> > wrap token
> > Octet no   Name        Description
> >         --------------------------------------------------------------
> >          0..1     TOK_ID    Identification field.  Tokens emitted by
> >                             GSS_Wrap() contain the hex value 05 04
> >                             expressed in big-endian order in this
> >                             field.
> >          2        Flags     Attributes field, as described in section
> >                             4.2.2.
> >          3        Filler    Contains the hex value FF.
> >          4..5     EC        Contains the "extra count" field, in big-
> >                             endian order as described in section 4.2.3.
> >          6..7     RRC       Contains the "right rotation count" in big-
> >                             endian order, as described in section
> >                             4.2.5.
> >          8..15    SND_SEQ   Sequence number field in clear text,
> >                             expressed in big-endian order.
> >          16..last Data      Encrypted data for Wrap tokens with
> >                             confidentiality, or plaintext data followed
> >                             by the checksum for Wrap tokens without
> >                             confidentiality, as described in section
> >                             4.2.4.
> >
> > Also 16bytes of header (GSS_KRB5_TOK_HDR_LEN), encrypted data, and cksum
> > (other things like padding)
> >
> > RFC 3961 defines known cksum sizes:
> > Checksum type              sumtype        checksum         section or
> >                                value            size         reference
> >   ---------------------------------------------------------------------
> >   CRC32                            1               4           6.1.3
> >   rsa-md4                          2              16           6.1.2
> >   rsa-md4-des                      3              24           6.2.5
> >   des-mac                          4              16           6.2.7
> >   des-mac-k                        5               8           6.2.8
> >   rsa-md4-des-k                    6              16           6.2.6
> >   rsa-md5                          7              16           6.1.1
> >   rsa-md5-des                      8              24           6.2.4
> >   rsa-md5-des3                     9              24             ??
> >   sha1 (unkeyed)                  10              20             ??
> >   hmac-sha1-des3-kd               12              20            6.3
> >   hmac-sha1-des3                  13              20             ??
> >   sha1 (unkeyed)                  14              20             ??
> >   hmac-sha1-96-aes128             15              20         [KRB5-AES]
> >   hmac-sha1-96-aes256             16              20         [KRB5-AES]
> >   [reserved]                  0x8003               ?         [GSS-KRB5]
> >
> > Linux kernel now mainly supports type 15,16 so max cksum size is 20bytes.
> > (GSS_KRB5_MAX_CKSUM_LEN)
> >
> > Re-use already existing define of GSS_KRB5_MAX_SLACK_NEEDED that's used
> > for encoding the gss_wrap tokens (same tokens are used in reply).
> >
> > Fixes: 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing reply buffer size")
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> > net/sunrpc/auth_gss/auth_gss.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> > index 24ca861..5a733a6 100644
> > --- a/net/sunrpc/auth_gss/auth_gss.c
> > +++ b/net/sunrpc/auth_gss/auth_gss.c
> > @@ -20,6 +20,7 @@
> > #include <linux/sunrpc/clnt.h>
> > #include <linux/sunrpc/auth.h>
> > #include <linux/sunrpc/auth_gss.h>
> > +#include <linux/sunrpc/gss_krb5.h>
> > #include <linux/sunrpc/svcauth_gss.h>
> > #include <linux/sunrpc/gss_err.h>
> > #include <linux/workqueue.h>
> > @@ -51,6 +52,8 @@
> > /* length of a krb5 verifier (48), plus data added before arguments when
> >  * using integrity (two 4-byte integers): */
> > #define GSS_VERF_SLACK                100
> > +/* covers lengths of gss_unwrap() extra kerberos mic and wrap token */
> > +#define GSS_RESP_SLACK               (GSS_KRB5_MAX_SLACK_NEEDED << 2)
>
> GSS_KRB5_MAX_SLACK_NEEDED is already in bytes. Shouldn't need the "<< 2" here.


Ok yes just for my own understanding I convinced myself that indeed
"<<2" is not needed here because clnt.c will do rq_rcvsize is <<=2.

Now question: Do I even need to introduce GSS_RES_SLACK at all or
perhaps just use GSS_KRB5_MAX_SLACK_NEEDED to initialize?

> > static DEFINE_HASHTABLE(gss_auth_hash_table, 4);
> > static DEFINE_SPINLOCK(gss_auth_hash_lock);
> > @@ -1050,7 +1053,7 @@ static void gss_pipe_free(struct gss_pipe *p)
> >               goto err_put_mech;
> >       auth = &gss_auth->rpc_auth;
> >       auth->au_cslack = GSS_CRED_SLACK >> 2;
> > -     auth->au_rslack = GSS_VERF_SLACK >> 2;
> > +     auth->au_rslack = GSS_RESP_SLACK >> 2;
> >       auth->au_verfsize = GSS_VERF_SLACK >> 2;
> >       auth->au_ralign = GSS_VERF_SLACK >> 2;
> >       auth->au_flags = 0;
> > --
> > 1.8.3.1
> >
>
> --
> Chuck Lever
>
>
>
Olga Kornievskaia March 26, 2020, 12:10 p.m. UTC | #7
On Wed, Mar 25, 2020 at 5:55 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>
> > On Mar 25, 2020, at 5:52 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> >
> > On Wed, 2020-03-25 at 17:43 -0400, Chuck Lever wrote:
> >>> On Mar 25, 2020, at 5:33 PM, Trond Myklebust <
> >>> trondmy@hammerspace.com> wrote:
> >>>
> >>> Hi Olga,
> >>>
> >>> On Wed, 2020-03-25 at 17:01 -0400, Olga Kornievskaia wrote:
> >>>> From: Olga Kornievskaia <kolga@netapp.com>
> >>>>
> >>>> Ever since commit 2c94b8eca1a2 ("SUNRPC: Use au_rslack when
> >>>> computing
> >>>> reply buffer size"). It changed how "req->rq_rcvsize" is
> >>>> calculated.
> >>>> It
> >>>> used to use au_cslack value which was nice and large and changed
> >>>> it
> >>>> to
> >>>> au_rslack value which turns out to be too small.
> >>>>
> >>>> Since 5.1, v3 mount with sec=krb5p fails against an Ontap server
> >>>> because client's receive buffer it too small.
> >>>>
> >>>> For gss krb5p, we need to account for the mic token in the
> >>>> verifier,
> >>>> and the wrap token in the wrap token.
> >>>>
> >>>> RFC 4121 defines:
> >>>> mic token
> >>>> Octet no   Name        Description
> >>>>        ---------------------------------------------------------
> >>>> -
> >>>> ----
> >>>>        0..1     TOK_ID     Identification field.  Tokens emitted
> >>>> by
> >>>>                            GSS_GetMIC() contain the hex value 04
> >>>> 04
> >>>>                            expressed in big-endian order in this
> >>>>                            field.
> >>>>        2        Flags      Attributes field, as described in
> >>>> section
> >>>>                            4.2.2.
> >>>>        3..7     Filler     Contains five octets of hex value FF.
> >>>>        8..15    SND_SEQ    Sequence number field in clear text,
> >>>>                            expressed in big-endian order.
> >>>>        16..last SGN_CKSUM  Checksum of the "to-be-signed" data
> >>>> and
> >>>>                            octet 0..15, as described in section
> >>>> 4.2.4.
> >>>>
> >>>> that's 16bytes (GSS_KRB5_TOK_HDR_LEN) + chksum
> >>>>
> >>>> wrap token
> >>>> Octet no   Name        Description
> >>>>        ---------------------------------------------------------
> >>>> -
> >>>> ----
> >>>>         0..1     TOK_ID    Identification field.  Tokens emitted
> >>>> by
> >>>>                            GSS_Wrap() contain the hex value 05
> >>>> 04
> >>>>                            expressed in big-endian order in this
> >>>>                            field.
> >>>>         2        Flags     Attributes field, as described in
> >>>> section
> >>>>                            4.2.2.
> >>>>         3        Filler    Contains the hex value FF.
> >>>>         4..5     EC        Contains the "extra count" field, in
> >>>> big-
> >>>>                            endian order as described in section
> >>>> 4.2.3.
> >>>>         6..7     RRC       Contains the "right rotation count"
> >>>> in
> >>>> big-
> >>>>                            endian order, as described in section
> >>>>                            4.2.5.
> >>>>         8..15    SND_SEQ   Sequence number field in clear text,
> >>>>                            expressed in big-endian order.
> >>>>         16..last Data      Encrypted data for Wrap tokens with
> >>>>                            confidentiality, or plaintext data
> >>>> followed
> >>>>                            by the checksum for Wrap tokens
> >>>> without
> >>>>                            confidentiality, as described in
> >>>> section
> >>>>                            4.2.4.
> >>>>
> >>>> Also 16bytes of header (GSS_KRB5_TOK_HDR_LEN), encrypted data,
> >>>> and
> >>>> cksum
> >>>> (other things like padding)
> >>>>
> >>>> RFC 3961 defines known cksum sizes:
> >>>> Checksum
> >>>> type              sumtype        checksum         section or
> >>>>                               value            size         refe
> >>>> ren
> >>>> ce
> >>>>  -------------------------------------------------------------
> >>>> ---
> >>>> -----
> >>>>
> >>>> CRC32                            1               4           6.1.
> >>>> 3
> >>>>  rsa-
> >>>> md4                          2              16           6.1.2
> >>>>  rsa-md4-
> >>>> des                      3              24           6.2.5
> >>>>  des-
> >>>> mac                          4              16           6.2.7
> >>>>  des-mac-
> >>>> k                        5               8           6.2.8
> >>>>  rsa-md4-des-
> >>>> k                    6              16           6.2.6
> >>>>  rsa-
> >>>> md5                          7              16           6.1.1
> >>>>  rsa-md5-
> >>>> des                      8              24           6.2.4
> >>>>  rsa-md5-
> >>>> des3                     9              24             ??
> >>>>  sha1
> >>>> (unkeyed)                  10              20             ??
> >>>>  hmac-sha1-des3-
> >>>> kd               12              20            6.3
> >>>>  hmac-sha1-
> >>>> des3                  13              20             ??
> >>>>  sha1
> >>>> (unkeyed)                  14              20             ??
> >>>>  hmac-sha1-96-
> >>>> aes128             15              20         [KRB5-
> >>>> AES]
> >>>>  hmac-sha1-96-
> >>>> aes256             16              20         [KRB5-
> >>>> AES]
> >>>>
> >>>> [reserved]                  0x8003               ?         [GSS-
> >>>> KRB5]
> >>>>
> >>>> Linux kernel now mainly supports type 15,16 so max cksum size is
> >>>> 20bytes.
> >>>> (GSS_KRB5_MAX_CKSUM_LEN)
> >>>>
> >>>> Re-use already existing define of GSS_KRB5_MAX_SLACK_NEEDED
> >>>> that's
> >>>> used
> >>>> for encoding the gss_wrap tokens (same tokens are used in reply).
> >>>>
> >>>> Fixes: 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing reply
> >>>> buffer size")
> >>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >>>> ---
> >>>> net/sunrpc/auth_gss/auth_gss.c | 5 ++++-
> >>>> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/net/sunrpc/auth_gss/auth_gss.c
> >>>> b/net/sunrpc/auth_gss/auth_gss.c
> >>>> index 24ca861..5a733a6 100644
> >>>> --- a/net/sunrpc/auth_gss/auth_gss.c
> >>>> +++ b/net/sunrpc/auth_gss/auth_gss.c
> >>>> @@ -20,6 +20,7 @@
> >>>> #include <linux/sunrpc/clnt.h>
> >>>> #include <linux/sunrpc/auth.h>
> >>>> #include <linux/sunrpc/auth_gss.h>
> >>>> +#include <linux/sunrpc/gss_krb5.h>
> >>>> #include <linux/sunrpc/svcauth_gss.h>
> >>>> #include <linux/sunrpc/gss_err.h>
> >>>> #include <linux/workqueue.h>
> >>>> @@ -51,6 +52,8 @@
> >>>> /* length of a krb5 verifier (48), plus data added before
> >>>> arguments
> >>>> when
> >>>> * using integrity (two 4-byte integers): */
> >>>> #define GSS_VERF_SLACK             100
> >>>> +/* covers lengths of gss_unwrap() extra kerberos mic and wrap
> >>>> token
> >>>> */
> >>>> +#define GSS_RESP_SLACK            (GSS_KRB5_MAX_SLACK_NEEDED <<
> >>>> 2)
> >>>>
> >>>> static DEFINE_HASHTABLE(gss_auth_hash_table, 4);
> >>>> static DEFINE_SPINLOCK(gss_auth_hash_lock);
> >>>> @@ -1050,7 +1053,7 @@ static void gss_pipe_free(struct gss_pipe
> >>>> *p)
> >>>>            goto err_put_mech;
> >>>>    auth = &gss_auth->rpc_auth;
> >>>>    auth->au_cslack = GSS_CRED_SLACK >> 2;
> >>>> -  auth->au_rslack = GSS_VERF_SLACK >> 2;
> >>>> +  auth->au_rslack = GSS_RESP_SLACK >> 2;
> >>>>    auth->au_verfsize = GSS_VERF_SLACK >> 2;
> >>>>    auth->au_ralign = GSS_VERF_SLACK >> 2;
> >>>>    auth->au_flags = 0;
> >>>
> >>> Is this a sufficient fix, though? It looks to me as if the above is
> >>> just an initial value that gets adjusted on the fly in
> >>> gss_unwrap_resp_priv():
> >>>
> >>>       auth->au_rslack = auth->au_verfsize + 2 +
> >>>                         XDR_QUADLEN(savedlen - rcv_buf->len);
> >>>       auth->au_ralign = auth->au_verfsize + 2 +
> >>>                         XDR_QUADLEN(savedlen - rcv_buf->len);
> >>
> >> That's correct. The GSS_*_SLACK value is a _sz value that is
> >> the largest possible expected size of the extra GSS data.
> >>
> >>
> >>> My questions would be
> >>>
> >>> - Are we sure that the above calculation (in
> >>> gss_unwrap_resp_priv()) is
> >>> correct?
> >>
> >> Yes, this is the correct computation.
> >>
> >> We know this because if the initial au_rslack value is large
> >> enough, then subsequent Replies have the correct amount of buffer
> >> space and always succeed.
> >>
> >>
> >>> - Are we sure that the above calculation always gives the same
> >>> answer
> >>> over time? We probably should not store a value that keeps
> >>> changing.
> >>
> >> It does not change after the first Reply. au_rslack is typically
> >> adjusted downwards from the initial value based on the size of the
> >> first received Reply.
> >>
> >> Not setting these variables after the first Reply has been received
> >> would be a minor optimization that could be done after Olga's fix.
> >>
> >
> > OK. So you're both saying that as long as the initial value is correct,
> > we're good for the duration of the GSS session? Fair enough, I'll apply
> > this patch for 5.7 then.
>
> Thanks, please see my one-line correction. I think the definition of
> GSS_RESP_SLACK should not include the "<< 2", I would like Olga to
> confirm.

Yes I do.

> > Let's also fix up the above in a separate patch to not keep setting
> > auth->au_rslack / auth->au_ralign if their values are not changing.
> > That should prevent unnecessary cache line bouncing.
>
> I volunteer, unless Olga wants to take it.

Please do the fix (as because I'm not understanding what needs to be
fixed and I'd be bugging you folks to explain it first).

>
>
> --
> Chuck Lever
>
>
>
Chuck Lever March 26, 2020, 1:59 p.m. UTC | #8
> On Mar 26, 2020, at 8:04 AM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
> 
> On Wed, Mar 25, 2020 at 5:34 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>> 
>> 
>>> On Mar 25, 2020, at 5:01 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
>>> 
>>> From: Olga Kornievskaia <kolga@netapp.com>
>>> 
>>> Ever since commit 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing
>>> reply buffer size"). It changed how "req->rq_rcvsize" is calculated. It
>>> used to use au_cslack value which was nice and large and changed it to
>>> au_rslack value which turns out to be too small.
>>> 
>>> Since 5.1, v3 mount with sec=krb5p fails against an Ontap server
>>> because client's receive buffer it too small.
>>> 
>>> For gss krb5p, we need to account for the mic token in the verifier,
>>> and the wrap token in the wrap token.
>>> 
>>> RFC 4121 defines:
>>> mic token
>>> Octet no   Name        Description
>>>        --------------------------------------------------------------
>>>        0..1     TOK_ID     Identification field.  Tokens emitted by
>>>                            GSS_GetMIC() contain the hex value 04 04
>>>                            expressed in big-endian order in this
>>>                            field.
>>>        2        Flags      Attributes field, as described in section
>>>                            4.2.2.
>>>        3..7     Filler     Contains five octets of hex value FF.
>>>        8..15    SND_SEQ    Sequence number field in clear text,
>>>                            expressed in big-endian order.
>>>        16..last SGN_CKSUM  Checksum of the "to-be-signed" data and
>>>                            octet 0..15, as described in section 4.2.4.
>>> 
>>> that's 16bytes (GSS_KRB5_TOK_HDR_LEN) + chksum
>>> 
>>> wrap token
>>> Octet no   Name        Description
>>>        --------------------------------------------------------------
>>>         0..1     TOK_ID    Identification field.  Tokens emitted by
>>>                            GSS_Wrap() contain the hex value 05 04
>>>                            expressed in big-endian order in this
>>>                            field.
>>>         2        Flags     Attributes field, as described in section
>>>                            4.2.2.
>>>         3        Filler    Contains the hex value FF.
>>>         4..5     EC        Contains the "extra count" field, in big-
>>>                            endian order as described in section 4.2.3.
>>>         6..7     RRC       Contains the "right rotation count" in big-
>>>                            endian order, as described in section
>>>                            4.2.5.
>>>         8..15    SND_SEQ   Sequence number field in clear text,
>>>                            expressed in big-endian order.
>>>         16..last Data      Encrypted data for Wrap tokens with
>>>                            confidentiality, or plaintext data followed
>>>                            by the checksum for Wrap tokens without
>>>                            confidentiality, as described in section
>>>                            4.2.4.
>>> 
>>> Also 16bytes of header (GSS_KRB5_TOK_HDR_LEN), encrypted data, and cksum
>>> (other things like padding)
>>> 
>>> RFC 3961 defines known cksum sizes:
>>> Checksum type              sumtype        checksum         section or
>>>                               value            size         reference
>>>  ---------------------------------------------------------------------
>>>  CRC32                            1               4           6.1.3
>>>  rsa-md4                          2              16           6.1.2
>>>  rsa-md4-des                      3              24           6.2.5
>>>  des-mac                          4              16           6.2.7
>>>  des-mac-k                        5               8           6.2.8
>>>  rsa-md4-des-k                    6              16           6.2.6
>>>  rsa-md5                          7              16           6.1.1
>>>  rsa-md5-des                      8              24           6.2.4
>>>  rsa-md5-des3                     9              24             ??
>>>  sha1 (unkeyed)                  10              20             ??
>>>  hmac-sha1-des3-kd               12              20            6.3
>>>  hmac-sha1-des3                  13              20             ??
>>>  sha1 (unkeyed)                  14              20             ??
>>>  hmac-sha1-96-aes128             15              20         [KRB5-AES]
>>>  hmac-sha1-96-aes256             16              20         [KRB5-AES]
>>>  [reserved]                  0x8003               ?         [GSS-KRB5]
>>> 
>>> Linux kernel now mainly supports type 15,16 so max cksum size is 20bytes.
>>> (GSS_KRB5_MAX_CKSUM_LEN)
>>> 
>>> Re-use already existing define of GSS_KRB5_MAX_SLACK_NEEDED that's used
>>> for encoding the gss_wrap tokens (same tokens are used in reply).
>>> 
>>> Fixes: 2c94b8eca1a2 ("SUNRPC: Use au_rslack when computing reply buffer size")
>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>> ---
>>> net/sunrpc/auth_gss/auth_gss.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
>>> index 24ca861..5a733a6 100644
>>> --- a/net/sunrpc/auth_gss/auth_gss.c
>>> +++ b/net/sunrpc/auth_gss/auth_gss.c
>>> @@ -20,6 +20,7 @@
>>> #include <linux/sunrpc/clnt.h>
>>> #include <linux/sunrpc/auth.h>
>>> #include <linux/sunrpc/auth_gss.h>
>>> +#include <linux/sunrpc/gss_krb5.h>
>>> #include <linux/sunrpc/svcauth_gss.h>
>>> #include <linux/sunrpc/gss_err.h>
>>> #include <linux/workqueue.h>
>>> @@ -51,6 +52,8 @@
>>> /* length of a krb5 verifier (48), plus data added before arguments when
>>> * using integrity (two 4-byte integers): */
>>> #define GSS_VERF_SLACK                100
>>> +/* covers lengths of gss_unwrap() extra kerberos mic and wrap token */
>>> +#define GSS_RESP_SLACK               (GSS_KRB5_MAX_SLACK_NEEDED << 2)
>> 
>> GSS_KRB5_MAX_SLACK_NEEDED is already in bytes. Shouldn't need the "<< 2" here.
> 
> 
> Ok yes just for my own understanding I convinced myself that indeed
> "<<2" is not needed here because clnt.c will do rq_rcvsize is <<=2.
> 
> Now question: Do I even need to introduce GSS_RES_SLACK at all or
> perhaps just use GSS_KRB5_MAX_SLACK_NEEDED to initialize?

For the moment, Kerberos is the only supported security flavor,
so we're using that value without any other modification. I guess
the extra "#define GSS_RESP_SLACK" seems pointless in that case.

I'm OK with using the KRB5_MAX_SLACK macro directly if there's no
objection from others.


>>> static DEFINE_HASHTABLE(gss_auth_hash_table, 4);
>>> static DEFINE_SPINLOCK(gss_auth_hash_lock);
>>> @@ -1050,7 +1053,7 @@ static void gss_pipe_free(struct gss_pipe *p)
>>>              goto err_put_mech;
>>>      auth = &gss_auth->rpc_auth;
>>>      auth->au_cslack = GSS_CRED_SLACK >> 2;
>>> -     auth->au_rslack = GSS_VERF_SLACK >> 2;
>>> +     auth->au_rslack = GSS_RESP_SLACK >> 2;
>>>      auth->au_verfsize = GSS_VERF_SLACK >> 2;
>>>      auth->au_ralign = GSS_VERF_SLACK >> 2;
>>>      auth->au_flags = 0;
>>> --
>>> 1.8.3.1
>>> 
>> 
>> --
>> Chuck Lever

--
Chuck Lever
diff mbox series

Patch

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 24ca861..5a733a6 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -20,6 +20,7 @@ 
 #include <linux/sunrpc/clnt.h>
 #include <linux/sunrpc/auth.h>
 #include <linux/sunrpc/auth_gss.h>
+#include <linux/sunrpc/gss_krb5.h>
 #include <linux/sunrpc/svcauth_gss.h>
 #include <linux/sunrpc/gss_err.h>
 #include <linux/workqueue.h>
@@ -51,6 +52,8 @@ 
 /* length of a krb5 verifier (48), plus data added before arguments when
  * using integrity (two 4-byte integers): */
 #define GSS_VERF_SLACK		100
+/* covers lengths of gss_unwrap() extra kerberos mic and wrap token */
+#define GSS_RESP_SLACK		(GSS_KRB5_MAX_SLACK_NEEDED << 2)
 
 static DEFINE_HASHTABLE(gss_auth_hash_table, 4);
 static DEFINE_SPINLOCK(gss_auth_hash_lock);
@@ -1050,7 +1053,7 @@  static void gss_pipe_free(struct gss_pipe *p)
 		goto err_put_mech;
 	auth = &gss_auth->rpc_auth;
 	auth->au_cslack = GSS_CRED_SLACK >> 2;
-	auth->au_rslack = GSS_VERF_SLACK >> 2;
+	auth->au_rslack = GSS_RESP_SLACK >> 2;
 	auth->au_verfsize = GSS_VERF_SLACK >> 2;
 	auth->au_ralign = GSS_VERF_SLACK >> 2;
 	auth->au_flags = 0;