Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3d6242195f2ea33fc10d8e7dafadb9e5ad65b1be.camel@kernel.org>
Subject: Re: [PATCH v2 1/7] SUNRPC: Fix svcxdr_init_decode's end-of-buffer
 calculation
From:   Jeff Layton <jlayton@kernel.org>
To:     Chuck Lever <chuck.lever@oracle.com>, linux-nfs@vger.kernel.org
Date:   Mon, 29 Aug 2022 08:48:41 -0400
In-Reply-To: <166171262197.21449.2261873517844800915.stgit@manet.1015granger.net>
References: <166171174172.21449.5036120183381273656.stgit@manet.1015granger.net>
         <166171262197.21449.2261873517844800915.stgit@manet.1015granger.net>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Sun, 2022-08-28 at 14:50 -0400, Chuck Lever wrote:
> Ensure that stream-based argument decoding can't go past the actual
> end of the receive buffer. xdr_init_decode's calculation of the
> value of xdr->end over-estimates the end of the buffer because the
> Linux kernel RPC server code does not remove the size of the RPC
> header from rqstp->rq_arg before calling the upper layer's
> dispatcher.
>=20
> The server-side still uses the svc_getnl() macros to decode the
> RPC call header. These macros reduce the length of the head iov
> but do not update the total length of the message in the buffer
> (buf->len).
>=20
> A proper fix for this would be to replace the use of svc_getnl() and
> friends in the RPC header decoder, but that would be a large and
> invasive change that would be difficult to backport.
>=20
> Fixes: 5191955d6fc6 ("SUNRPC: Prepare for xdr_stream-style decoding on th=
e server-side")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  include/linux/sunrpc/svc.h |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>=20
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index daecb009c05b..5a830b66f059 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -544,16 +544,27 @@ static inline void svc_reserve_auth(struct svc_rqst=
 *rqstp, int space)
>  }
> =20
>  /**
> - * svcxdr_init_decode - Prepare an xdr_stream for svc Call decoding
> + * svcxdr_init_decode - Prepare an xdr_stream for Call decoding
>   * @rqstp: controlling server RPC transaction context
>   *
> + * This function currently assumes the RPC header in rq_arg has
> + * already been decoded. Upon return, xdr->p points to the
> + * location of the upper layer header.

nit: "upper layer header" is a bit nebulous here. Maybe "points to the
start of the RPC program header" ?

>   */
>  static inline void svcxdr_init_decode(struct svc_rqst *rqstp)
>  {
>  	struct xdr_stream *xdr =3D &rqstp->rq_arg_stream;
> -	struct kvec *argv =3D rqstp->rq_arg.head;
> +	struct xdr_buf *buf =3D &rqstp->rq_arg;
> +	struct kvec *argv =3D buf->head;
> =20
> -	xdr_init_decode(xdr, &rqstp->rq_arg, argv->iov_base, NULL);
> +	/*
> +	 * svc_getnl() and friends do not keep the xdr_buf's ::len
> +	 * field up to date. Refresh that field before initializing
> +	 * the argument decoding stream.
> +	 */
> +	buf->len =3D buf->head->iov_len + buf->page_len + buf->tail->iov_len;
> +
> +	xdr_init_decode(xdr, buf, argv->iov_base, NULL);
>  	xdr_set_scratch_page(xdr, rqstp->rq_scratch_page);
>  }
> =20
>=20
>=20

Patch looks fine. I do wish this code were less confusing with length
handing though I'm not sure how to approach cleaning that up.

Reviewed-by: Jeff Layton <jlayton@kernel.org>