Re: [PATCH v2 1/7] SUNRPC: Fix svcxdr_init_decode's end-of-buffer calculation

[Chuck Lever III <chuck.lever@oracle.com>]

> On Aug 29, 2022, at 8:48 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> 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.
>> 
>> 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 |   17 ++++++++++++++---
>> 1 file changed, 14 insertions(+), 3 deletions(-)
>> 
>> 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)
>> }
>> 
>> /**
>> - * 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" ?

Hm. I thought "upper layer header" is the exact terminology
that means "NFS or whatever". I understand what you mean by
"RPC program header" but I've never heard that term before.

But I'm open to other suggestions for clarity.


>>  */
>> 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);
>> +	/*
>> +	 * 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 = 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);
>> }
>> 
>> 
>> 
> 
> 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.

The plan is to move the call to svcxdr_init_decode() into
svc_process(), eventually, so that svc_getnl() and friends
can be replaced with xdr_stream helpers which intrinsically
manage the xdr_buf message and buffer length fields.

But that means XDR-related code in server-side RPCSEC GSS has
to be converted to xdr_stream too. That's not a weekend project.


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

Thanks!


--
Chuck Lever