Re: [PATCH v2 0/2] Fix XDR encoding near page boundaries

Rick Macklem
2024-12-24T15:50:42Z

On Tue, Dec 24, 2024 at 6:16 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 12/23/24 6:22 PM, Rick Macklem wrote:
> > On Mon, Dec 23, 2024 at 10:07 AM <cel@kernel.org> wrote:
> >>
> >> From: Chuck Lever <chuck.lever@oracle.com>
> >>
> >> Build out the patch series to address the longstanding bug pointed
> >> out by J David and Rick Macklem.
> >>
> >> At least during NFSv4 COMPOUND encoding, using
> >> write_bytes_to_xdr_buf() seems less brittle than saving a pointer
> >> into the XDR encoding buffer.
> >>
> >> I have one more patch to add (not yet included) that addresses the
> >> issue in the NFSv4 READ and READ_PLUS encoders.
> > It also looks like there is a similar situation in nfsd4_encode_fattr4().
> > It uses attrlen_p (only a 4byte xdr_reserve_space(), so safe for now.
> >
> > You might just regret choosing to not wire down the "safe to use
> > xdr_reserve_space() for 4 bytes" semantic, but it is obviously up to you.
>
> I'm 100% behind the idea of making it hard or impossible to code
> things the wrong way.
Righto, sounds good to me.

>
> But I'm not sure what you mean by "wire down". I can't think of a way
> to enforce the "only 4 octets or less" rule -- just having a helper
> function with that name documents it but doesn't enforce it.
Agreed. By "wired down" I was simply thinking of defining it as
required behaviour. Documented by a comment update or similar.

I was being a little "tongue in cheek" when I made the comment,
referring to all the work involved.

Good luck with it and have a good holiday, rick

>
> My plan is to replace the obviously unsafe call sites immediately,
> document the desired long-term semantics, then replace the other
> "safe for now" call sites over time.
>
> I've found a few other potentially unsafe server-side callers:
> * gss_wrap_req_integ
> * gss_wrap_req_priv
> * unx_marshal
>
> I consider these "safe for now" because the reserved space is guaranteed
> to be in the XDR buffer's head iovec, far away from page boundaries.
>
> I'm hoping that no-one introduces new unsafe call sites in the meantime.
> We should be able to catch that in review.
>
> That also buys some time to come up with something better.
>
>
> > rick
> >
> >>
> >> Changes since RFC:
> >> - Document the guarantees around pointer returned by xdr_reserve_space()
> >> - Use write_bytes_to_xdr_buf() instead
> >>
> >> Chuck Lever (2):
> >> NFSD: Encode COMPOUND operation status on page boundaries
> >> SUNRPC: Document validity guarantees of the pointer returned by
> >> reserve_space
> >>
> >> fs/nfsd/nfs4xdr.c | 20 ++++++++++----------
> >> net/sunrpc/xdr.c | 3 +++
> >> 2 files changed, 13 insertions(+), 10 deletions(-)
> >>
> >> --
> >> 2.47.0
> >>
>
>
> --
> Chuck Lever