[PATCH] SUNRPC: fix sign error causing rpcsec_gss drops

[PATCH] SUNRPC: fix sign error causing rpcsec_gss drops

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

If sd_max is unsigned, then sd_max - GSS_SEQ_WIN is a very large number
whenever sd_max is less than GSS_SEQ_WIN, and the comparison:

	seq_num <= sd->sd_max - GSS_SEQ_WIN

in gss_check_seq_num is pretty much always true, even when that's
clearly not what was intended.

This was causing pynfs to hang when using krb5, because pynfs uses zero
as the initial gss sequence number.  That's perfectly legal, but this
logic error causes knfsd to drop the rpc in that case.  Out-of-order
sequence IDs in the first GSS_SEQ_WIN (128) calls will also cause this.

Fixes: 10b9d99a3dbb ("SUNRPC: Augment server-side rpcgss tracepoints")
Cc: stable@vger.kernel.org
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 7dba6a9c213a..b87565b64928 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -645,7 +645,7 @@ static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci,
 		}
 		__set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win);
 		goto ok;
-	} else if (seq_num <= sd->sd_max - GSS_SEQ_WIN) {
+	} else if (seq_num + GSS_SEQ_WIN <= sd->sd_max) {
 		goto toolow;
 	}
 	if (__test_and_set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win))
-- 
2.31.1

Re: [PATCH] SUNRPC: fix sign error causing rpcsec_gss drops

[Chuck Lever III]

> On Oct 1, 2021, at 9:59 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> If sd_max is unsigned, then sd_max - GSS_SEQ_WIN is a very large number
> whenever sd_max is less than GSS_SEQ_WIN, and the comparison:
> 
> 	seq_num <= sd->sd_max - GSS_SEQ_WIN
> 
> in gss_check_seq_num is pretty much always true, even when that's
> clearly not what was intended.
> 
> This was causing pynfs to hang when using krb5, because pynfs uses zero
> as the initial gss sequence number.  That's perfectly legal, but this
> logic error causes knfsd to drop the rpc in that case.  Out-of-order
> sequence IDs in the first GSS_SEQ_WIN (128) calls will also cause this.
> 
> Fixes: 10b9d99a3dbb ("SUNRPC: Augment server-side rpcgss tracepoints")
> Cc: stable@vger.kernel.org
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>

This will be included in the next NFSD 5.15-rc. Thanks!
See the for-next branch at

git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git


> ---
> net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 7dba6a9c213a..b87565b64928 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -645,7 +645,7 @@ static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci,
> 		}
> 		__set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win);
> 		goto ok;
> -	} else if (seq_num <= sd->sd_max - GSS_SEQ_WIN) {
> +	} else if (seq_num + GSS_SEQ_WIN <= sd->sd_max) {
> 		goto toolow;
> 	}
> 	if (__test_and_set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win))
> -- 
> 2.31.1
> 

--
Chuck Lever

Re: [PATCH] SUNRPC: fix sign error causing rpcsec_gss drops

[Daniel Kobras]

Hi Bruce!

Am 01.10.21 um 15:59 schrieb J. Bruce Fields:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> If sd_max is unsigned, then sd_max - GSS_SEQ_WIN is a very large number
> whenever sd_max is less than GSS_SEQ_WIN, and the comparison:
> 
> 	seq_num <= sd->sd_max - GSS_SEQ_WIN
> 
> in gss_check_seq_num is pretty much always true, even when that's
> clearly not what was intended.
> 
> This was causing pynfs to hang when using krb5, because pynfs uses zero
> as the initial gss sequence number.  That's perfectly legal, but this
> logic error causes knfsd to drop the rpc in that case.  Out-of-order
> sequence IDs in the first GSS_SEQ_WIN (128) calls will also cause this.
> 
> Fixes: 10b9d99a3dbb ("SUNRPC: Augment server-side rpcgss tracepoints")

I wonder about the Fixes tag: That changeset added tracepoints to the
exit path, but the buggy logic seems to have been present since the
pre-git ages. Or am I missing something about 10b9d99a3dbb? (This might
explain some reports of--as you stated elsewhere--"once in a blue moon
my krb5 mounts hang" we've investigated, albeit on kernels that predate
10b9d99a3dbb.)

Kind regards,

Daniel

> Cc: stable@vger.kernel.org
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 7dba6a9c213a..b87565b64928 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -645,7 +645,7 @@ static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci,
>  		}
>  		__set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win);
>  		goto ok;
> -	} else if (seq_num <= sd->sd_max - GSS_SEQ_WIN) {
> +	} else if (seq_num + GSS_SEQ_WIN <= sd->sd_max) {
>  		goto toolow;
>  	}
>  	if (__test_and_set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win))
> 


-- 
Daniel Kobras
Principal Architect
Puzzle ITC Deutschland
+49 7071 14316 0
www.puzzle-itc.de

-- 
Puzzle ITC Deutschland GmbH
Sitz der Gesellschaft: Eisenbahnstraße 1, 72072 
Tübingen

Eingetragen am Amtsgericht Stuttgart HRB 765802
Geschäftsführer: 
Lukas Kallies, Daniel Kobras, Mark Pröhl

Re: [PATCH] SUNRPC: fix sign error causing rpcsec_gss drops

[J. Bruce Fields]

On Fri, Oct 01, 2021 at 06:50:12PM +0200, Daniel Kobras wrote:
> Hi Bruce!
> 
> Am 01.10.21 um 15:59 schrieb J. Bruce Fields:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > If sd_max is unsigned, then sd_max - GSS_SEQ_WIN is a very large number
> > whenever sd_max is less than GSS_SEQ_WIN, and the comparison:
> > 
> > 	seq_num <= sd->sd_max - GSS_SEQ_WIN
> > 
> > in gss_check_seq_num is pretty much always true, even when that's
> > clearly not what was intended.
> > 
> > This was causing pynfs to hang when using krb5, because pynfs uses zero
> > as the initial gss sequence number.  That's perfectly legal, but this
> > logic error causes knfsd to drop the rpc in that case.  Out-of-order
> > sequence IDs in the first GSS_SEQ_WIN (128) calls will also cause this.
> > 
> > Fixes: 10b9d99a3dbb ("SUNRPC: Augment server-side rpcgss tracepoints")
> 
> I wonder about the Fixes tag: That changeset added tracepoints to the
> exit path, but the buggy logic seems to have been present since the
> pre-git ages. Or am I missing something about 10b9d99a3dbb?

The relevant parts of 10b9d99a3dbb were:

 struct gss_svc_seq_data {
        /* highest seq number seen so far: */
-       int                     sd_max;
+       u32                     sd_max;

and

-gss_check_seq_num(struct rsc *rsci, int seq_num)
+static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci,
+                             u32 seq_num)

Together, they mean the comparison

	seq_num <= sd->sd_max - GSS_SEQ_WIN

in the case sd_max is zero, effectively ends up being

	seq_num <= 4294967168

instead of what was intended,

	seq_num <= -128

.

> (This might explain some reports of--as you stated elsewhere--"once in
> a blue moon my krb5 mounts hang" we've investigated, albeit on kernels
> that predate 10b9d99a3dbb.)

Sounds like it was something else, I'm afraid!

--b.

Re: [PATCH] SUNRPC: fix sign error causing rpcsec_gss drops

[Daniel Kobras]

Hi Bruce!

Am 01.10.21 um 19:44 schrieb J. Bruce Fields:
> On Fri, Oct 01, 2021 at 06:50:12PM +0200, Daniel Kobras wrote:
>> Am 01.10.21 um 15:59 schrieb J. Bruce Fields:
>>> From: "J. Bruce Fields" <bfields@redhat.com>
>>>
>>> If sd_max is unsigned, then sd_max - GSS_SEQ_WIN is a very large number
>>> whenever sd_max is less than GSS_SEQ_WIN, and the comparison:
>>>
>>> 	seq_num <= sd->sd_max - GSS_SEQ_WIN
>>>
>>> in gss_check_seq_num is pretty much always true, even when that's
>>> clearly not what was intended.
>>>
>>> This was causing pynfs to hang when using krb5, because pynfs uses zero
>>> as the initial gss sequence number.  That's perfectly legal, but this
>>> logic error causes knfsd to drop the rpc in that case.  Out-of-order
>>> sequence IDs in the first GSS_SEQ_WIN (128) calls will also cause this.
>>>
>>> Fixes: 10b9d99a3dbb ("SUNRPC: Augment server-side rpcgss tracepoints")
>>
>> I wonder about the Fixes tag: That changeset added tracepoints to the
>> exit path, but the buggy logic seems to have been present since the
>> pre-git ages. Or am I missing something about 10b9d99a3dbb?
> 
> The relevant parts of 10b9d99a3dbb were:
> 
>  struct gss_svc_seq_data {
>         /* highest seq number seen so far: */
> -       int                     sd_max;
> +       u32                     sd_max;
> 
> and
> 
> -gss_check_seq_num(struct rsc *rsci, int seq_num)
> +static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci,
> +                             u32 seq_num)
> 
> Together, they mean the comparison
> 
> 	seq_num <= sd->sd_max - GSS_SEQ_WIN
> 
> in the case sd_max is zero, effectively ends up being
> 
> 	seq_num <= 4294967168
> 
> instead of what was intended,
> 
> 	seq_num <= -128
> 
> .

Sorry, indeed I had missed the change in signedness. Thanks for elaborating!

>> (This might explain some reports of--as you stated elsewhere--"once in
>> a blue moon my krb5 mounts hang" we've investigated, albeit on kernels
>> that predate 10b9d99a3dbb.)
> 
> Sounds like it was something else, I'm afraid!

Darn! ;-)

Kind regards,

Daniel
-- 
Daniel Kobras
Principal Architect
Puzzle ITC Deutschland
+49 7071 14316 0
www.puzzle-itc.de

-- 
Puzzle ITC Deutschland GmbH
Sitz der Gesellschaft: Eisenbahnstraße 1, 72072 
Tübingen

Eingetragen am Amtsgericht Stuttgart HRB 765802
Geschäftsführer: 
Lukas Kallies, Daniel Kobras, Mark Pröhl