2008-05-19 18:20:04

by J. Bruce Fields

Subject: Re: [PATCH 01/05] svcrdma: Verify read-list fits within RPCSVC_MAXPAGES

On Sun, May 18, 2008 at 07:13:17PM -0500, Tom Tucker wrote:
> A RDMA read-list cannot contain more elements than RPCSVC_MAXPAGES or
> it will overflow the DTO context. Verify this when processing the
> protocol header.
>
> Signed-off-by: Tom Tucker <[email protected]>
>
> ---
> net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> index 6b16d8c..06ab484 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> @@ -306,6 +306,8 @@ static int rdma_read_xdr(struct svcxprt_rdma *xprt,
> ch_sge_ary = (struct chunk_sge *)tmp_ch_ctxt->sge;
>
> svc_rdma_rcl_chunk_counts(ch, &ch_count, &byte_count);
> + if (ch_count > RPCSVC_MAXPAGES)
> + return -EINVAL;
> sge_count = rdma_rcl_to_sge(xprt, rqstp, hdr_ctxt, rmsgp,
> sge, ch_sge_ary,
> ch_count, byte_count);

If the ch_count is just the total number of bytes to be read into this
request, then don't we also need to know at what offset they're going to
be inserted? (Shouldn't there be some check like ch->rc_position +
ch_count > RPCSVC_MAXPAGES ?)

Also, do we verify somewhere (before calling
svc_rdma_rcl_chunk_counts()) that rc_discrim is set on the last chunk?

--b.

2008-05-20 01:04:21

by Tom Tucker

Subject: Re: [PATCH 01/05] svcrdma: Verify read-list fits within RPCSVC_MAXPAGES

On Mon, 2008-05-19 at 14:20 -0400, J. Bruce Fields wrote:
> On Sun, May 18, 2008 at 07:13:17PM -0500, Tom Tucker wrote:
> > A RDMA read-list cannot contain more elements than RPCSVC_MAXPAGES or
> > it will overflow the DTO context. Verify this when processing the
> > protocol header.
> >
> > Signed-off-by: Tom Tucker <[email protected]>
> >
> > ---
> > net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> > index 6b16d8c..06ab484 100644
> > --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> > +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> > @@ -306,6 +306,8 @@ static int rdma_read_xdr(struct svcxprt_rdma *xprt,
> > ch_sge_ary = (struct chunk_sge *)tmp_ch_ctxt->sge;
> >
> > svc_rdma_rcl_chunk_counts(ch, &ch_count, &byte_count);
> > + if (ch_count > RPCSVC_MAXPAGES)
> > + return -EINVAL;
> > sge_count = rdma_rcl_to_sge(xprt, rqstp, hdr_ctxt, rmsgp,
> > sge, ch_sge_ary,
> > ch_count, byte_count);
>
> If the ch_count is just the total number of bytes to be read into this
> request, then don't we also need to know at what offset they're going to
> be inserted? (Shouldn't there be some check like ch->rc_position +
> ch_count > RPCSVC_MAXPAGES ?)
>

The ch_count is the number of RPCRDMA chunk elements in the read-list.
It's not a byte count, but a scatter-gather-list length.

I think the local read-list buffer limits should be clamped by
svc_rdma_rcl_chunk_counts, however, see below...

> Also, do we verify somewhere (before calling
> svc_rdma_rcl_chunk_counts()) that rc_discrim is set on the last chunk?
>

No we don't and a Byzantine client could crash us. The computed
byte_count should also be clamped here. I'll add this to the list --
nice catch.

This kind of check along with a bunch of others should go in
svc_rdma_xdr_decode_req. I have these things planned for the 2.6.27
time-frame (along with Fast NSMR support).

Do you think it's more urgent?

Tom

> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html