> On Aug 23, 2022, at 5:52 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
>> ---
>> 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.
See patch 7/7 in this series, which updates two functions that are part
of client-side call chains.
I'm reading into your objection a little, but I think your long term
goal is to have the XDR layer manage ::len opaquely to RPC consumers
in both the client and the server implementation.
I agree that's a good goal, and yes, eventually I will pull the chain
and replace the use of svc_getnl() and friends with xdr_stream-based
decoding. Just not today.
> If you need this helper, then stuff under a server-specific mattress
> where developers looking for client functionality can't find it.
I don't have to have this helper, it was meant as nothing more than
code de-duplication. I'll remove it and drop 7/7.
Thanks for taking a look!
--
Chuck Lever
