| Message ID | 166171262197.21449.2261873517844800915.stgit@manet.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for server-side xdr_stream overhaul | expand |
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" ? > */ > 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. Reviewed-by: Jeff Layton <jlayton@kernel.org>
> 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
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. */ 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); }
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(-)