| Message ID | 20241223180724.1804-4-cel@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix XDR encoding near page boundaries | expand |
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. 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 >
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. 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. 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 >>
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
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. 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(-)