[PATCH 0/2] Fix crash in trace_rpcgss_context due to 0-length acceptor

[PATCH 0/2] Fix crash in trace_rpcgss_context due to 0-length acceptor

[Dave Wysochanski]

This small patchset fixes a kernel crash when the rpcgss_context trace event is
enabled and IO is in flight when a kerberos ticket expires.  The crash occurs
because the acceptor name may be 0 bytes long and the gss_fill_context() function
does not handle it properly.  This causes the ctx->gc_acceptor.data to be
ZERO_SIZE_PTR which is not properly recognized by the tracepoint code.

The first patch is a simple refactor and eliminates duplicate helper functions
related to the crash.  The second patch is the actual fix inside one of the
helper functions due to the definition of an opaque XDR object.  This object
is defined in RFC 4506 (see section 4.10), where 'length' is an integer in a
range including 0.

Reproducer

# Enable the tracepoint and mount the share
trace-cmd start -e rpcgss:*
mount -osec=krb5 nfs-server:/export /mnt/nfs

# Obtain a kerberos ticket
# Set ticket lifetime to something small like 20 seconds
su test -c "kinit -l 20 test"

# Sleep for a portion of the ticket lifetime so we are writing while the ticket expires
sleep 10

# Now run some IO long enough that the ticket expires midway
dd if=/dev/urandom of=/mnt/nfs/file bs=1M count=100


Dave Wysochanski (2):
  SUNRPC: Move simple_get_bytes and simple_get_netobj into xdr.h
  SUNRPC: Handle 0 length opaque XDR object data properly

 include/linux/sunrpc/xdr.h          | 33 +++++++++++++++++++++++++++--
 net/sunrpc/auth_gss/auth_gss.c      | 29 -------------------------
 net/sunrpc/auth_gss/gss_krb5_mech.c | 29 -------------------------
 3 files changed, 31 insertions(+), 60 deletions(-)

-- 
2.25.2

[PATCH 1/2] SUNRPC: Move simple_get_bytes and simple_get_netobj into xdr.h

[Dave Wysochanski]

Remove duplicated helper functions to parse opaque XDR objects
and place inside the xdr.h file.  Also update comment which
is wrong since lockd is not the only user of xdr_netobj.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 include/linux/sunrpc/xdr.h          | 33 +++++++++++++++++++++++++++--
 net/sunrpc/auth_gss/auth_gss.c      | 29 -------------------------
 net/sunrpc/auth_gss/gss_krb5_mech.c | 29 -------------------------
 3 files changed, 31 insertions(+), 60 deletions(-)

diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index 19b6dea27367..8ef788ff80b9 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -25,8 +25,7 @@ struct rpc_rqst;
 #define XDR_QUADLEN(l)		(((l) + 3) >> 2)
 
 /*
- * Generic opaque `network object.' At the kernel level, this type
- * is used only by lockd.
+ * Generic opaque `network object.'
  */
 #define XDR_MAX_NETOBJ		1024
 struct xdr_netobj {
@@ -34,6 +33,36 @@ struct xdr_netobj {
 	u8 *			data;
 };
 
+static inline const void *
+simple_get_bytes(const void *p, const void *end, void *res, size_t len)
+{
+	const void *q = (const void *)((const char *)p + len);
+	if (unlikely(q > end || q < p))
+		return ERR_PTR(-EFAULT);
+	memcpy(res, p, len);
+	return q;
+}
+
+static inline const void *
+simple_get_netobj(const void *p, const void *end, struct xdr_netobj *dest)
+{
+	const void *q;
+	unsigned int len;
+
+	p = simple_get_bytes(p, end, &len, sizeof(len));
+	if (IS_ERR(p))
+		return p;
+	q = (const void *)((const char *)p + len);
+	if (unlikely(q > end || q < p))
+		return ERR_PTR(-EFAULT);
+	dest->data = kmemdup(p, len, GFP_NOFS);
+	if (unlikely(dest->data == NULL))
+		return ERR_PTR(-ENOMEM);
+	dest->len = len;
+	return q;
+}
+
+
 /*
  * Basic structure for transmission/reception of a client XDR message.
  * Features a header (for a linear buffer containing RPC headers
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 4ecc2a959567..228456b22b23 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -125,35 +125,6 @@ gss_cred_set_ctx(struct rpc_cred *cred, struct gss_cl_ctx *ctx)
 	clear_bit(RPCAUTH_CRED_NEW, &cred->cr_flags);
 }
 
-static const void *
-simple_get_bytes(const void *p, const void *end, void *res, size_t len)
-{
-	const void *q = (const void *)((const char *)p + len);
-	if (unlikely(q > end || q < p))
-		return ERR_PTR(-EFAULT);
-	memcpy(res, p, len);
-	return q;
-}
-
-static inline const void *
-simple_get_netobj(const void *p, const void *end, struct xdr_netobj *dest)
-{
-	const void *q;
-	unsigned int len;
-
-	p = simple_get_bytes(p, end, &len, sizeof(len));
-	if (IS_ERR(p))
-		return p;
-	q = (const void *)((const char *)p + len);
-	if (unlikely(q > end || q < p))
-		return ERR_PTR(-EFAULT);
-	dest->data = kmemdup(p, len, GFP_NOFS);
-	if (unlikely(dest->data == NULL))
-		return ERR_PTR(-ENOMEM);
-	dest->len = len;
-	return q;
-}
-
 static struct gss_cl_ctx *
 gss_cred_get_ctx(struct rpc_cred *cred)
 {
diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c b/net/sunrpc/auth_gss/gss_krb5_mech.c
index ae9acf3a7389..99ea36d5eefe 100644
--- a/net/sunrpc/auth_gss/gss_krb5_mech.c
+++ b/net/sunrpc/auth_gss/gss_krb5_mech.c
@@ -143,35 +143,6 @@ get_gss_krb5_enctype(int etype)
 	return NULL;
 }
 
-static const void *
-simple_get_bytes(const void *p, const void *end, void *res, int len)
-{
-	const void *q = (const void *)((const char *)p + len);
-	if (unlikely(q > end || q < p))
-		return ERR_PTR(-EFAULT);
-	memcpy(res, p, len);
-	return q;
-}
-
-static const void *
-simple_get_netobj(const void *p, const void *end, struct xdr_netobj *res)
-{
-	const void *q;
-	unsigned int len;
-
-	p = simple_get_bytes(p, end, &len, sizeof(len));
-	if (IS_ERR(p))
-		return p;
-	q = (const void *)((const char *)p + len);
-	if (unlikely(q > end || q < p))
-		return ERR_PTR(-EFAULT);
-	res->data = kmemdup(p, len, GFP_NOFS);
-	if (unlikely(res->data == NULL))
-		return ERR_PTR(-ENOMEM);
-	res->len = len;
-	return q;
-}
-
 static inline const void *
 get_key(const void *p, const void *end,
 	struct krb5_ctx *ctx, struct crypto_sync_skcipher **res)
-- 
2.25.2

[PATCH 2/2] SUNRPC: Handle 0 length opaque XDR object data properly

[Dave Wysochanski]

When handling an auth_gss downcall, it's possible to get 0-length
opaque object for the acceptor.  In this case we should make sure
simple_get_netobj() returns -EFAULT and does not continue to
kmemdup() which will return a special pointer value of
ZERO_SIZE_PTR.  If it just so happens that the rpcgss_context
trace event is enabled, the kernel will crash as follows:

[  162.887992] BUG: kernel NULL pointer dereference, address: 0000000000000010
[  162.898693] #PF: supervisor read access in kernel mode
[  162.900830] #PF: error_code(0x0000) - not-present page
[  162.902940] PGD 0 P4D 0
[  162.904027] Oops: 0000 [#1] SMP PTI
[  162.905493] CPU: 4 PID: 4321 Comm: rpc.gssd Kdump: loaded Not tainted 5.10.0 #133
[  162.908548] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
[  162.910978] RIP: 0010:strlen+0x0/0x20
[  162.912505] Code: 48 89 f9 74 09 48 83 c1 01 80 39 00 75 f7 31 d2 44 0f b6 04 16 44 88 04 11 48 83 c2 01 45 84 c0 75 ee c3 0f 1f 80 00 00 00 00 <80> 3f 00 74 10 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 31
[  162.920101] RSP: 0018:ffffaec900c77d90 EFLAGS: 00010202
[  162.922263] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00000000fffde697
[  162.925158] RDX: 000000000000002f RSI: 0000000000000080 RDI: 0000000000000010
[  162.928073] RBP: 0000000000000010 R08: 0000000000000e10 R09: 0000000000000000
[  162.930976] R10: ffff8e698a590cb8 R11: 0000000000000001 R12: 0000000000000e10
[  162.933883] R13: 00000000fffde697 R14: 000000010034d517 R15: 0000000000070028
[  162.936777] FS:  00007f1e1eb93700(0000) GS:ffff8e6ab7d00000(0000) knlGS:0000000000000000
[  162.940067] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  162.942417] CR2: 0000000000000010 CR3: 0000000104eba000 CR4: 00000000000406e0
[  162.945300] Call Trace:
[  162.946428]  trace_event_raw_event_rpcgss_context+0x84/0x140 [auth_rpcgss]
[  162.949308]  ? __kmalloc_track_caller+0x35/0x5a0
[  162.951224]  ? gss_pipe_downcall+0x3a3/0x6a0 [auth_rpcgss]
[  162.953484]  gss_pipe_downcall+0x585/0x6a0 [auth_rpcgss]
[  162.955953]  rpc_pipe_write+0x58/0x70 [sunrpc]
[  162.957849]  vfs_write+0xcb/0x2c0
[  162.959264]  ksys_write+0x68/0xe0
[  162.960706]  do_syscall_64+0x33/0x40
[  162.962238]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  162.964346] RIP: 0033:0x7f1e1f1e57df

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 include/linux/sunrpc/xdr.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index 8ef788ff80b9..aca38bdb0059 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -53,7 +53,7 @@ simple_get_netobj(const void *p, const void *end, struct xdr_netobj *dest)
 	if (IS_ERR(p))
 		return p;
 	q = (const void *)((const char *)p + len);
-	if (unlikely(q > end || q < p))
+	if (unlikely(q > end || q <= p))
 		return ERR_PTR(-EFAULT);
 	dest->data = kmemdup(p, len, GFP_NOFS);
 	if (unlikely(dest->data == NULL))
-- 
2.25.2

Re: [PATCH 2/2] SUNRPC: Handle 0 length opaque XDR object data properly

[David Wysochanski]

On Tue, Jan 5, 2021 at 5:12 PM Dave Wysochanski <dwysocha@redhat.com> wrote:
>
> When handling an auth_gss downcall, it's possible to get 0-length
> opaque object for the acceptor.  In this case we should make sure
> simple_get_netobj() returns -EFAULT and does not continue to
> kmemdup() which will return a special pointer value of
> ZERO_SIZE_PTR.  If it just so happens that the rpcgss_context
> trace event is enabled, the kernel will crash as follows:
>
> [  162.887992] BUG: kernel NULL pointer dereference, address: 0000000000000010
> [  162.898693] #PF: supervisor read access in kernel mode
> [  162.900830] #PF: error_code(0x0000) - not-present page
> [  162.902940] PGD 0 P4D 0
> [  162.904027] Oops: 0000 [#1] SMP PTI
> [  162.905493] CPU: 4 PID: 4321 Comm: rpc.gssd Kdump: loaded Not tainted 5.10.0 #133
> [  162.908548] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> [  162.910978] RIP: 0010:strlen+0x0/0x20
> [  162.912505] Code: 48 89 f9 74 09 48 83 c1 01 80 39 00 75 f7 31 d2 44 0f b6 04 16 44 88 04 11 48 83 c2 01 45 84 c0 75 ee c3 0f 1f 80 00 00 00 00 <80> 3f 00 74 10 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 31
> [  162.920101] RSP: 0018:ffffaec900c77d90 EFLAGS: 00010202
> [  162.922263] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00000000fffde697
> [  162.925158] RDX: 000000000000002f RSI: 0000000000000080 RDI: 0000000000000010
> [  162.928073] RBP: 0000000000000010 R08: 0000000000000e10 R09: 0000000000000000
> [  162.930976] R10: ffff8e698a590cb8 R11: 0000000000000001 R12: 0000000000000e10
> [  162.933883] R13: 00000000fffde697 R14: 000000010034d517 R15: 0000000000070028
> [  162.936777] FS:  00007f1e1eb93700(0000) GS:ffff8e6ab7d00000(0000) knlGS:0000000000000000
> [  162.940067] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  162.942417] CR2: 0000000000000010 CR3: 0000000104eba000 CR4: 00000000000406e0
> [  162.945300] Call Trace:
> [  162.946428]  trace_event_raw_event_rpcgss_context+0x84/0x140 [auth_rpcgss]
> [  162.949308]  ? __kmalloc_track_caller+0x35/0x5a0
> [  162.951224]  ? gss_pipe_downcall+0x3a3/0x6a0 [auth_rpcgss]
> [  162.953484]  gss_pipe_downcall+0x585/0x6a0 [auth_rpcgss]
> [  162.955953]  rpc_pipe_write+0x58/0x70 [sunrpc]
> [  162.957849]  vfs_write+0xcb/0x2c0
> [  162.959264]  ksys_write+0x68/0xe0
> [  162.960706]  do_syscall_64+0x33/0x40
> [  162.962238]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [  162.964346] RIP: 0033:0x7f1e1f1e57df
>
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
>  include/linux/sunrpc/xdr.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> index 8ef788ff80b9..aca38bdb0059 100644
> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h
> @@ -53,7 +53,7 @@ simple_get_netobj(const void *p, const void *end, struct xdr_netobj *dest)
>         if (IS_ERR(p))
>                 return p;
>         q = (const void *)((const char *)p + len);
> -       if (unlikely(q > end || q < p))
> +       if (unlikely(q > end || q <= p))
>                 return ERR_PTR(-EFAULT);
>         dest->data = kmemdup(p, len, GFP_NOFS);
>         if (unlikely(dest->data == NULL))
> --
> 2.25.2
>

Just following up on this as I got no response, but didn't see
it in anyone's tree yet.

After talking offline with Bruce about this, I think the above
approach is wrong and i'm going to do a v2 of this patch.

The short reason is, I don't think simple_get_netobj() should be
returning the -EFAULT in the case of a 0-length acceptor,
which I explain in the cover letter why that is valid as an
object.  Rather I think this patch should do a explicit check
for 0 length, and then fill in dest->data = NULL:

diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index 8ef788ff80b9..b4f5bf104405 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -55,9 +55,12 @@ struct xdr_netobj {
        q = (const void *)((const char *)p + len);
        if (unlikely(q > end || q < p))
                return ERR_PTR(-EFAULT);
-       dest->data = kmemdup(p, len, GFP_NOFS);
-       if (unlikely(dest->data == NULL))
-               return ERR_PTR(-ENOMEM);
+       if (len) {
+               dest->data = kmemdup(p, len, GFP_NOFS);
+               if (unlikely(dest->data == NULL))
+                       return ERR_PTR(-ENOMEM);
+       } else
+               dest->data = NULL;
        dest->len = len;
        return q;