by J. Bruce Fields

[permalink] [raw]

Subject: Re: [PATCH 03/27] SUNRPC: The transmitted message must lie in the RPCSEC window of validity

On Mon, Sep 03, 2018 at 11:29:12AM -0400, Trond Myklebust wrote:
> If a message has been encoded using RPCSEC_GSS, the server is
> maintaining a window of sequence numbers that it considers valid.
> The client should normally be tracking that window, and needs to
> verify that the sequence number used by the message being transmitted
> still lies inside the window of validity.
>
> So far, we've been able to assume this condition would be realised
> automatically, since the server has been encoding the message only

Do you mean "the client has been encoding..."?

--b.

> after taking the socket lock. Once we change that condition, we
> will need the explicit check.
>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> include/linux/sunrpc/auth.h | 2 ++
> include/linux/sunrpc/auth_gss.h | 1 +
> net/sunrpc/auth.c | 10 ++++++++
> net/sunrpc/auth_gss/auth_gss.c | 41 +++++++++++++++++++++++++++++++++
> net/sunrpc/clnt.c | 3 +++
> net/sunrpc/xprt.c | 7 ++++++
> 6 files changed, 64 insertions(+)
>
> diff --git a/include/linux/sunrpc/auth.h b/include/linux/sunrpc/auth.h
> index 58a6765c1c5e..2c97a3933ef9 100644
> --- a/include/linux/sunrpc/auth.h
> +++ b/include/linux/sunrpc/auth.h
> @@ -157,6 +157,7 @@ struct rpc_credops {
> int (*crkey_timeout)(struct rpc_cred *);
> bool (*crkey_to_expire)(struct rpc_cred *);
> char * (*crstringify_acceptor)(struct rpc_cred *);
> + bool (*crneed_reencode)(struct rpc_task *);
> };
>
> extern const struct rpc_authops authunix_ops;
> @@ -192,6 +193,7 @@ __be32 * rpcauth_marshcred(struct rpc_task *, __be32 *);
> __be32 * rpcauth_checkverf(struct rpc_task *, __be32 *);
> int rpcauth_wrap_req(struct rpc_task *task, kxdreproc_t encode, void *rqstp, __be32 *data, void *obj);
> int rpcauth_unwrap_resp(struct rpc_task *task, kxdrdproc_t decode, void *rqstp, __be32 *data, void *obj);
> +bool rpcauth_xmit_need_reencode(struct rpc_task *task);
> int rpcauth_refreshcred(struct rpc_task *);
> void rpcauth_invalcred(struct rpc_task *);
> int rpcauth_uptodatecred(struct rpc_task *);
> diff --git a/include/linux/sunrpc/auth_gss.h b/include/linux/sunrpc/auth_gss.h
> index 0c9eac351aab..30427b729070 100644
> --- a/include/linux/sunrpc/auth_gss.h
> +++ b/include/linux/sunrpc/auth_gss.h
> @@ -70,6 +70,7 @@ struct gss_cl_ctx {
> refcount_t count;
> enum rpc_gss_proc gc_proc;
> u32 gc_seq;
> + u32 gc_seq_xmit;
> spinlock_t gc_seq_lock;
> struct gss_ctx *gc_gss_ctx;
> struct xdr_netobj gc_wire_ctx;
> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index 305ecea92170..59df5cdba0ac 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -817,6 +817,16 @@ rpcauth_unwrap_resp(struct rpc_task *task, kxdrdproc_t decode, void *rqstp,
> return rpcauth_unwrap_req_decode(decode, rqstp, data, obj);
> }
>
> +bool
> +rpcauth_xmit_need_reencode(struct rpc_task *task)
> +{
> + struct rpc_cred *cred = task->tk_rqstp->rq_cred;
> +
> + if (!cred || !cred->cr_ops->crneed_reencode)
> + return false;
> + return cred->cr_ops->crneed_reencode(task);
> +}
> +
> int
> rpcauth_refreshcred(struct rpc_task *task)
> {
> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> index 21c0aa0a0d1d..c898a7c75e84 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -1984,6 +1984,46 @@ gss_unwrap_req_decode(kxdrdproc_t decode, struct rpc_rqst *rqstp,
> return decode(rqstp, &xdr, obj);
> }
>
> +static bool
> +gss_seq_is_newer(u32 new, u32 old)
> +{
> + return (s32)(new - old) > 0;
> +}
> +
> +static bool
> +gss_xmit_need_reencode(struct rpc_task *task)
> +{
> + struct rpc_rqst *req = task->tk_rqstp;
> + struct rpc_cred *cred = req->rq_cred;
> + struct gss_cl_ctx *ctx = gss_cred_get_ctx(cred);
> + u32 win, seq_xmit;
> + bool ret = true;
> +
> + if (!ctx)
> + return true;
> +
> + if (gss_seq_is_newer(req->rq_seqno, READ_ONCE(ctx->gc_seq)))
> + goto out;
> +
> + seq_xmit = READ_ONCE(ctx->gc_seq_xmit);
> + while (gss_seq_is_newer(req->rq_seqno, seq_xmit)) {
> + u32 tmp = seq_xmit;
> +
> + seq_xmit = cmpxchg(&ctx->gc_seq_xmit, tmp, req->rq_seqno);
> + if (seq_xmit == tmp) {
> + ret = false;
> + goto out;
> + }
> + }
> +
> + win = ctx->gc_win;
> + if (win > 0)
> + ret = !gss_seq_is_newer(req->rq_seqno, seq_xmit - win);
> +out:
> + gss_put_ctx(ctx);
> + return ret;
> +}
> +
> static int
> gss_unwrap_resp(struct rpc_task *task,
> kxdrdproc_t decode, void *rqstp, __be32 *p, void *obj)
> @@ -2052,6 +2092,7 @@ static const struct rpc_credops gss_credops = {
> .crunwrap_resp = gss_unwrap_resp,
> .crkey_timeout = gss_key_timeout,
> .crstringify_acceptor = gss_stringify_acceptor,
> + .crneed_reencode = gss_xmit_need_reencode,
> };
>
> static const struct rpc_credops gss_nullops = {
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 4f1ec8013332..d41b5ac1d4e8 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2184,6 +2184,9 @@ call_status(struct rpc_task *task)
> /* shutdown or soft timeout */
> rpc_exit(task, status);
> break;
> + case -EBADMSG:
> + task->tk_action = call_transmit;
> + break;
> default:
> if (clnt->cl_chatty)
> printk("%s: RPC call returned error %d\n",
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index 6aa09edc9567..3973e10ea2bd 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -1014,6 +1014,13 @@ void xprt_transmit(struct rpc_task *task)
> dprintk("RPC: %5u xprt_transmit(%u)\n", task->tk_pid, req->rq_slen);
>
> if (!req->rq_reply_bytes_recvd) {
> +
> + /* Verify that our message lies in the RPCSEC_GSS window */
> + if (!req->rq_bytes_sent && rpcauth_xmit_need_reencode(task)) {
> + task->tk_status = -EBADMSG;
> + return;
> + }
> +
> if (list_empty(&req->rq_list) && rpc_reply_expected(task)) {
> /*
> * Add to the list only if we're expecting a reply
> --
> 2.17.1