Re: [PATCH v1 1/7] SUNRPC: Fix end-of-buffer calculation

[Trond Myklebust <trondmy@hammerspace.com>]

On Tue, 2022-08-23 at 17:00 -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.
> 
> 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).
> 
> 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.
> 
> Fixes: 5191955d6fc6 ("SUNRPC: Prepare for xdr_stream-style decoding
> on the server-side")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  include/linux/sunrpc/svc.h |   14 +++++++++++---
>  include/linux/sunrpc/xdr.h |   12 ++++++++++++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index daecb009c05b..494375313a6f 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -544,16 +544,24 @@ static inline void svc_reserve_auth(struct
> svc_rqst *rqstp, int space)
>  }
>  
>  /**
> - * 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.
>   */
>  static inline void svcxdr_init_decode(struct svc_rqst *rqstp)
>  {
>         struct xdr_stream *xdr = &rqstp->rq_arg_stream;
> -       struct kvec *argv = rqstp->rq_arg.head;
> +       struct xdr_buf *buf = &rqstp->rq_arg;
> +       struct kvec *argv = buf->head;
>  
> -       xdr_init_decode(xdr, &rqstp->rq_arg, argv->iov_base, NULL);
> +       /* Reset the argument buffer's length, now that the RPC
> header
> +        * has been decoded. */
> +       buf->len = xdr_buf_length(buf);
> +
> +       xdr_init_decode(xdr, buf, argv->iov_base, NULL);
>         xdr_set_scratch_page(xdr, rqstp->rq_scratch_page);
>  }
>  
> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> index 69175029abbb..f6bb215d4029 100644
> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h
> @@ -83,6 +83,18 @@ xdr_buf_init(struct xdr_buf *buf, void *start,
> size_t len)
>         buf->buflen = len;
>  }
>  
> +/**
> + * xdr_buf_length - Return the summed length of @buf's sub-buffers
> + * @buf: an instantiated xdr_buf
> + *
> + * Returns the sum of the lengths of the head kvec, the tail kvec,
> + * and the page array.
> + */
> +static inline unsigned int xdr_buf_length(const struct xdr_buf *buf)
> +{
> +       return buf->head->iov_len + buf->page_len + buf->tail-
> >iov_len;
> +}
> +

NACK. This function is neither needed nor wanted for the client code,
which already does the right thing w.r.t. maintaining an authoritative
buf->len.

If you need this helper, then stuff under a server-specific mattress
where developers looking for client functionality can't find it.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com