On Tue, Dec 24, 2024 at 6:16 AM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote: > > On 12/23/24 6:22 PM, Rick Macklem wrote: > > On Mon, Dec 23, 2024 at 10:07 AM <cel@xxxxxxxxxx> wrote: > >> > >> From: Chuck Lever <chuck.lever@xxxxxxxxxx> > >> > >> 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
