Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH V3 1/2] SUNRPC: Fix buffer handling of GSS MIC without slack
@ 2019-09-16 11:59 Benjamin Coddington
  2019-09-16 11:59 ` [PATCH V3 2/2] SUNRPC: Rename xdr_buf_read_netobj to xdr_buf_read_mic Benjamin Coddington
  2019-09-16 14:43 ` [PATCH V3 1/2] SUNRPC: Fix buffer handling of GSS MIC without slack Chuck Lever
  0 siblings, 2 replies; 3+ messages in thread
From: Benjamin Coddington @ 2019-09-16 11:59 UTC (permalink / raw)
  To: trond.myklebust, anna.schumaker, tibbs, chuck.lever
  Cc: linux-nfs, bfields, km

The GSS Message Integrity Check data for krb5i may lie partially in the XDR
reply buffer's pages and tail.  If so, we try to copy the entire MIC into
free space in the tail.  But as the estimations of the slack space required
for authentication and verification have improved there may be less free
space in the tail to complete this copy -- see commit 2c94b8eca1a2
("SUNRPC: Use au_rslack when computing reply buffer size").  In fact, there
may only be room in the tail for a single copy of the MIC, and not part of
the MIC and then another complete copy.

The real world failure reported is that `ls` of a directory on NFS may
sometimes return -EIO, which can be traced back to xdr_buf_read_netobj()
failing to find available free space in the tail to copy the MIC.

Fix this by checking for the case of the MIC crossing the boundaries of
head, pages, and tail. If so, shift the buffer until the MIC is contained
completely within the pages or tail.  This allows the remainder of the
function to create a sub buffer that directly address the complete MIC.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
Cc: stable@vger.kernel.org # v5.1
---
 net/sunrpc/xdr.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 48c93b9e525e..b256806d69cd 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1237,16 +1237,29 @@ xdr_encode_word(struct xdr_buf *buf, unsigned int base, u32 obj)
 EXPORT_SYMBOL_GPL(xdr_encode_word);
 
 /* If the netobj starting offset bytes from the start of xdr_buf is contained
- * entirely in the head or the tail, set object to point to it; otherwise
- * try to find space for it at the end of the tail, copy it there, and
- * set obj to point to it. */
+ * entirely in the head, pages, or tail, set object to point to it; otherwise
+ * shift the buffer until it is contained entirely within the pages or tail.
+ */
 int xdr_buf_read_netobj(struct xdr_buf *buf, struct xdr_netobj *obj, unsigned int offset)
 {
 	struct xdr_buf subbuf;
+	unsigned int boundary;
 
 	if (xdr_decode_word(buf, offset, &obj->len))
 		return -EFAULT;
-	if (xdr_buf_subsegment(buf, &subbuf, offset + 4, obj->len))
+	offset += 4;
+
+	/* Is the obj partially in the head? */
+	boundary = buf->head[0].iov_len;
+	if (offset < boundary && (offset + obj->len) > boundary)
+		xdr_shift_buf(buf, boundary - offset);
+
+	/* Is the obj partially in the pages? */
+	boundary += buf->page_len;
+	if (offset < boundary && (offset + obj->len) > boundary)
+		xdr_shrink_pagelen(buf, boundary - offset);
+
+	if (xdr_buf_subsegment(buf, &subbuf, offset, obj->len))
 		return -EFAULT;
 
 	/* Is the obj contained entirely in the head? */
@@ -1258,11 +1271,7 @@ int xdr_buf_read_netobj(struct xdr_buf *buf, struct xdr_netobj *obj, unsigned in
 	if (subbuf.tail[0].iov_len == obj->len)
 		return 0;
 
-	/* use end of tail as storage for obj:
-	 * (We don't copy to the beginning because then we'd have
-	 * to worry about doing a potentially overlapping copy.
-	 * This assumes the object is at most half the length of the
-	 * tail.) */
+	/* Find a contiguous area in @buf to hold all of @obj */
 	if (obj->len > buf->buflen - buf->len)
 		return -ENOMEM;
 	if (buf->tail[0].iov_len != 0)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH V3 2/2] SUNRPC: Rename xdr_buf_read_netobj to xdr_buf_read_mic
  2019-09-16 11:59 [PATCH V3 1/2] SUNRPC: Fix buffer handling of GSS MIC without slack Benjamin Coddington
@ 2019-09-16 11:59 ` Benjamin Coddington
  2019-09-16 14:43 ` [PATCH V3 1/2] SUNRPC: Fix buffer handling of GSS MIC without slack Chuck Lever
  1 sibling, 0 replies; 3+ messages in thread
From: Benjamin Coddington @ 2019-09-16 11:59 UTC (permalink / raw)
  To: trond.myklebust, anna.schumaker, tibbs, chuck.lever
  Cc: linux-nfs, bfields, km

Let the name reflect the single use.  The function now assumes the GSS MIC
is the last object in the buffer.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 include/linux/sunrpc/xdr.h     |  2 +-
 net/sunrpc/auth_gss/auth_gss.c |  2 +-
 net/sunrpc/xdr.c               | 52 ++++++++++++++++++++--------------
 3 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index 8a87d8bcb197..f33e5013bdfb 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -186,7 +186,7 @@ xdr_adjust_iovec(struct kvec *iov, __be32 *p)
 extern void xdr_shift_buf(struct xdr_buf *, size_t);
 extern void xdr_buf_from_iov(struct kvec *, struct xdr_buf *);
 extern int xdr_buf_subsegment(struct xdr_buf *, struct xdr_buf *, unsigned int, unsigned int);
-extern int xdr_buf_read_netobj(struct xdr_buf *, struct xdr_netobj *, unsigned int);
+extern int xdr_buf_read_mic(struct xdr_buf *, struct xdr_netobj *, unsigned int);
 extern int read_bytes_from_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
 extern int write_bytes_to_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
 
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 4ce42c62458e..d75fddca44c9 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -1960,7 +1960,7 @@ gss_unwrap_resp_integ(struct rpc_task *task, struct rpc_cred *cred,
 
 	if (xdr_buf_subsegment(rcv_buf, &integ_buf, data_offset, integ_len))
 		goto unwrap_failed;
-	if (xdr_buf_read_netobj(rcv_buf, &mic, mic_offset))
+	if (xdr_buf_read_mic(rcv_buf, &mic, mic_offset))
 		goto unwrap_failed;
 	maj_stat = gss_verify_mic(ctx->gc_gss_ctx, &integ_buf, &mic);
 	if (maj_stat == GSS_S_CONTEXT_EXPIRED)
diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index b256806d69cd..74ae4c54b51a 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1236,52 +1236,60 @@ xdr_encode_word(struct xdr_buf *buf, unsigned int base, u32 obj)
 }
 EXPORT_SYMBOL_GPL(xdr_encode_word);
 
-/* If the netobj starting offset bytes from the start of xdr_buf is contained
- * entirely in the head, pages, or tail, set object to point to it; otherwise
- * shift the buffer until it is contained entirely within the pages or tail.
+/**
+ * xdr_buf_read_mic() - obtain the address of the GSS mic from xdr buf
+ * @buf: pointer to buffer containing a mic
+ * @mic: on success, returns the address of the mic
+ * @offset: the offset in buf where mic may be found
+ *
+ * This function may modify the xdr buf if the mic is found to be straddling
+ * a boundary between head, pages, and tail.  On success the mic can be read
+ * from the address returned.  There is no need to free the mic.
+ *
+ * Return: Success returns 0, otherwise an integer error.
  */
-int xdr_buf_read_netobj(struct xdr_buf *buf, struct xdr_netobj *obj, unsigned int offset)
+int xdr_buf_read_mic(struct xdr_buf *buf, struct xdr_netobj *mic, unsigned int offset)
 {
 	struct xdr_buf subbuf;
 	unsigned int boundary;
 
-	if (xdr_decode_word(buf, offset, &obj->len))
+	if (xdr_decode_word(buf, offset, &mic->len))
 		return -EFAULT;
 	offset += 4;
 
-	/* Is the obj partially in the head? */
+	/* Is the mic partially in the head? */
 	boundary = buf->head[0].iov_len;
-	if (offset < boundary && (offset + obj->len) > boundary)
+	if (offset < boundary && (offset + mic->len) > boundary)
 		xdr_shift_buf(buf, boundary - offset);
 
-	/* Is the obj partially in the pages? */
+	/* Is the mic partially in the pages? */
 	boundary += buf->page_len;
-	if (offset < boundary && (offset + obj->len) > boundary)
+	if (offset < boundary && (offset + mic->len) > boundary)
 		xdr_shrink_pagelen(buf, boundary - offset);
 
-	if (xdr_buf_subsegment(buf, &subbuf, offset, obj->len))
+	if (xdr_buf_subsegment(buf, &subbuf, offset, mic->len))
 		return -EFAULT;
 
-	/* Is the obj contained entirely in the head? */
-	obj->data = subbuf.head[0].iov_base;
-	if (subbuf.head[0].iov_len == obj->len)
+	/* Is the mic contained entirely in the head? */
+	mic->data = subbuf.head[0].iov_base;
+	if (subbuf.head[0].iov_len == mic->len)
 		return 0;
-	/* ..or is the obj contained entirely in the tail? */
-	obj->data = subbuf.tail[0].iov_base;
-	if (subbuf.tail[0].iov_len == obj->len)
+	/* ..or is the mic contained entirely in the tail? */
+	mic->data = subbuf.tail[0].iov_base;
+	if (subbuf.tail[0].iov_len == mic->len)
 		return 0;
 
-	/* Find a contiguous area in @buf to hold all of @obj */
-	if (obj->len > buf->buflen - buf->len)
+	/* Find a contiguous area in @buf to hold all of @mic */
+	if (mic->len > buf->buflen - buf->len)
 		return -ENOMEM;
 	if (buf->tail[0].iov_len != 0)
-		obj->data = buf->tail[0].iov_base + buf->tail[0].iov_len;
+		mic->data = buf->tail[0].iov_base + buf->tail[0].iov_len;
 	else
-		obj->data = buf->head[0].iov_base + buf->head[0].iov_len;
-	__read_bytes_from_xdr_buf(&subbuf, obj->data, obj->len);
+		mic->data = buf->head[0].iov_base + buf->head[0].iov_len;
+	__read_bytes_from_xdr_buf(&subbuf, mic->data, mic->len);
 	return 0;
 }
-EXPORT_SYMBOL_GPL(xdr_buf_read_netobj);
+EXPORT_SYMBOL_GPL(xdr_buf_read_mic);
 
 /* Returns 0 on success, or else a negative error code. */
 static int
-- 
2.20.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH V3 1/2] SUNRPC: Fix buffer handling of GSS MIC without slack
  2019-09-16 11:59 [PATCH V3 1/2] SUNRPC: Fix buffer handling of GSS MIC without slack Benjamin Coddington
  2019-09-16 11:59 ` [PATCH V3 2/2] SUNRPC: Rename xdr_buf_read_netobj to xdr_buf_read_mic Benjamin Coddington
@ 2019-09-16 14:43 ` Chuck Lever
  1 sibling, 0 replies; 3+ messages in thread
From: Chuck Lever @ 2019-09-16 14:43 UTC (permalink / raw)
  To: Benjamin Coddington
  Cc: trond.myklebust, Anna Schumaker, tibbs, Linux NFS Mailing List,
	Bruce Fields, km



> On Sep 16, 2019, at 7:59 AM, Benjamin Coddington <bcodding@redhat.com> wrote:
> 
> The GSS Message Integrity Check data for krb5i may lie partially in the XDR
> reply buffer's pages and tail.  If so, we try to copy the entire MIC into
> free space in the tail.  But as the estimations of the slack space required
> for authentication and verification have improved there may be less free
> space in the tail to complete this copy -- see commit 2c94b8eca1a2
> ("SUNRPC: Use au_rslack when computing reply buffer size").  In fact, there
> may only be room in the tail for a single copy of the MIC, and not part of
> the MIC and then another complete copy.
> 
> The real world failure reported is that `ls` of a directory on NFS may
> sometimes return -EIO, which can be traced back to xdr_buf_read_netobj()
> failing to find available free space in the tail to copy the MIC.
> 
> Fix this by checking for the case of the MIC crossing the boundaries of
> head, pages, and tail. If so, shift the buffer until the MIC is contained
> completely within the pages or tail.  This allows the remainder of the
> function to create a sub buffer that directly address the complete MIC.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> Cc: stable@vger.kernel.org # v5.1

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>


> ---
> net/sunrpc/xdr.c | 27 ++++++++++++++++++---------
> 1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 48c93b9e525e..b256806d69cd 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -1237,16 +1237,29 @@ xdr_encode_word(struct xdr_buf *buf, unsigned int base, u32 obj)
> EXPORT_SYMBOL_GPL(xdr_encode_word);
> 
> /* If the netobj starting offset bytes from the start of xdr_buf is contained
> - * entirely in the head or the tail, set object to point to it; otherwise
> - * try to find space for it at the end of the tail, copy it there, and
> - * set obj to point to it. */
> + * entirely in the head, pages, or tail, set object to point to it; otherwise
> + * shift the buffer until it is contained entirely within the pages or tail.
> + */
> int xdr_buf_read_netobj(struct xdr_buf *buf, struct xdr_netobj *obj, unsigned int offset)
> {
> 	struct xdr_buf subbuf;
> +	unsigned int boundary;
> 
> 	if (xdr_decode_word(buf, offset, &obj->len))
> 		return -EFAULT;
> -	if (xdr_buf_subsegment(buf, &subbuf, offset + 4, obj->len))
> +	offset += 4;
> +
> +	/* Is the obj partially in the head? */
> +	boundary = buf->head[0].iov_len;
> +	if (offset < boundary && (offset + obj->len) > boundary)
> +		xdr_shift_buf(buf, boundary - offset);
> +
> +	/* Is the obj partially in the pages? */
> +	boundary += buf->page_len;
> +	if (offset < boundary && (offset + obj->len) > boundary)
> +		xdr_shrink_pagelen(buf, boundary - offset);
> +
> +	if (xdr_buf_subsegment(buf, &subbuf, offset, obj->len))
> 		return -EFAULT;
> 
> 	/* Is the obj contained entirely in the head? */
> @@ -1258,11 +1271,7 @@ int xdr_buf_read_netobj(struct xdr_buf *buf, struct xdr_netobj *obj, unsigned in
> 	if (subbuf.tail[0].iov_len == obj->len)
> 		return 0;
> 
> -	/* use end of tail as storage for obj:
> -	 * (We don't copy to the beginning because then we'd have
> -	 * to worry about doing a potentially overlapping copy.
> -	 * This assumes the object is at most half the length of the
> -	 * tail.) */
> +	/* Find a contiguous area in @buf to hold all of @obj */
> 	if (obj->len > buf->buflen - buf->len)
> 		return -ENOMEM;
> 	if (buf->tail[0].iov_len != 0)
> -- 
> 2.20.1
> 

--
Chuck Lever




^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-16 11:59 [PATCH V3 1/2] SUNRPC: Fix buffer handling of GSS MIC without slack Benjamin Coddington
2019-09-16 11:59 ` [PATCH V3 2/2] SUNRPC: Rename xdr_buf_read_netobj to xdr_buf_read_mic Benjamin Coddington
2019-09-16 14:43 ` [PATCH V3 1/2] SUNRPC: Fix buffer handling of GSS MIC without slack Chuck Lever

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org
	public-inbox-index linux-nfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git