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

[Chuck Lever <chuck.lever@oracle.com>]

> 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