Skip to contentSkip to conversation

NFSD: copy SETXATTR data from all XDR buffer segments

PATCH In review Aldo Ariel Panzardo 3 participants 3 messages Sep 15

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 NFSD: copy SETXATTR data from all XDR buffer segments

nfsd4_vbuf_from_vector() manually copies the head followed by whole pages starting at offset zero. This assumes that all remaining data starts at the beginning of the page array and that no data is stored in the tail.

However, nfsd4_decode_setxattr() passes an xdr_buf returned by xdr_stream_subsegment(). Such a sub-buffer can have a nonzero page_base and can span its page and tail segments. Ignoring that layout can copy the wrong bytes into the value passed to vfs_setxattr().

Use read_bytes_from_xdr_buf(), which observes the complete xdr_buf layout.

Fixes c1346a1216ab ("NFSD: Replace the internals of the READ_BUF() macro")
---
 fs/nfsd/nfs4xdr.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)
fs/nfsd/nfs4xdr.c +3 -16
index e17488a911..0f9f926cbd 100644
@@ -2288,10 +2288,8 @@ static __be32
 nfsd4_vbuf_from_vector(struct nfsd4_compoundargs *argp, struct xdr_buf *xdr,
 		       char **bufp, size_t buflen)
 {
-	struct page **pages = xdr->pages;
 	struct kvec *head = xdr->head;
-	char *tmp, *dp;
-	u32 len;
+	char *tmp;
 
 	if (buflen <= head->iov_len) {
 		/*
@@ -2306,19 +2304,8 @@ nfsd4_vbuf_from_vector(struct nfsd4_compoundargs *argp, struct xdr_buf *xdr,
 	if (tmp == NULL)
 		return nfserr_jukebox;
 
-	dp = tmp;
-	memcpy(dp, head->iov_base, head->iov_len);
-	buflen -= head->iov_len;
-	dp += head->iov_len;
-
-	while (buflen > 0) {
-		len = min_t(u32, buflen, PAGE_SIZE);
-		memcpy(dp, page_address(*pages), len);
-
-		buflen -= len;
-		dp += len;
-		pages++;
-	}
+	if (read_bytes_from_xdr_buf(xdr, 0, tmp, buflen))
+		return nfserr_bad_xdr;
 
 	*bufp = tmp;
 	return 0;
Signature · 1 line
2.43.0
maintainer Reviewed-by on v1 1/1
#1 lore raw
Quoted 49 lines · Aldo Ariel Panzardo

nfsd4_vbuf_from_vector() manually copies the head followed by whole pages starting at offset zero. This assumes that all remaining data starts at the beginning of the page array and that no data is stored in the tail.

However, nfsd4_decode_setxattr() passes an xdr_buf returned by xdr_stream_subsegment(). Such a sub-buffer can have a nonzero page_base and can span its page and tail segments. Ignoring that layout can copy the wrong bytes into the value passed to vfs_setxattr().

Use read_bytes_from_xdr_buf(), which observes the complete xdr_buf layout.

Fixes c1346a1216ab ("NFSD: Replace the internals of the READ_BUF() macro")
---
 fs/nfsd/nfs4xdr.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)
fs/nfsd/nfs4xdr.c +1 -11
index e17488a911..0f9f926cbd 100644
@@ -2288,10 +2288,8 @@ static __be32
 nfsd4_vbuf_from_vector(struct nfsd4_compoundargs *argp, struct xdr_buf *xdr,
 		       char **bufp, size_t buflen)
 {
-	struct page **pages = xdr->pages;
 	struct kvec *head = xdr->head;
-	char *tmp, *dp;
-	u32 len;
+	char *tmp;
 
 	if (buflen <= head->iov_len) {
 		/*
@@ -2306,19 +2304,8 @@ nfsd4_vbuf_from_vector(struct nfsd4_compoundargs *argp, struct xdr_buf *xdr,
 	if (tmp == NULL)
 		return nfserr_jukebox;
 
-	dp = tmp;
-	memcpy(dp, head->iov_base, head->iov_len);
-	buflen -= head->iov_len;
-	dp += head->iov_len;
-
-	while (buflen > 0) {
-		len = min_t(u32, buflen, PAGE_SIZE);
-		memcpy(dp, page_address(*pages), len);

fs/nfsd/nfs4xdr.c nfsd4_vbuf_from_vector(struct nfsd4_compoundargs *argp, struct xdr_buf *xdr, · line 2311

Quoted 10 lines
fs/nfsd/nfs4xdr.c +2 -5
-
-		buflen -= len;
-		dp += len;
-		pages++;
-	}
+	if (read_bytes_from_xdr_buf(xdr, 0, tmp, buflen))
+		return nfserr_bad_xdr;
 
 	*bufp = tmp;
 	return 0;
maintainer
#2 lore raw
In reply to Aldo Ariel Panzardo
Quoted 5 lines · Aldo Ariel Panzardo

nfsd4_vbuf_from_vector() manually copies the head followed by whole pages starting at offset zero. This assumes that all remaining data starts at the beginning of the page array and that no data is stored in the tail.

Quoted 7 lines

However, nfsd4_decode_setxattr() passes an xdr_buf returned by xdr_stream_subsegment(). Such a sub-buffer can have a nonzero page_base and can span its page and tail segments. Ignoring that layout can copy the wrong bytes into the value passed to vfs_setxattr().

[...]

Applied to nfsd-testing, thanks!

[1/1] NFSD: copy SETXATTR data from all XDR buffer segments

      commit: e551e7dde7b82be0d3e6b848c9c5fef728d020dd

--
Chuck Lever