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 | 36 ++++++++++++++++++++++++++++++++++--
net/sunrpc/auth_gss/auth_gss.c | 29 -----------------------------
net/sunrpc/auth_gss/gss_krb5_mech.c | 29 -----------------------------
3 files changed, 34 insertions(+), 60 deletions(-)
--
1.8.3.1
When handling an auth_gss downcall, it's possible to get 0-length
opaque object for the acceptor. In the case of a 0-length XDR
object, make sure simple_get_netobj() fills in dest->data = NULL,
and does not continue to kmemdup() which will set
dest->data = ZERO_SIZE_PTR for the acceptor.
The trace event code can handle NULL but not ZERO_SIZE_PTR for a
string, and so without this patch the rpcgss_context trace event
will crash the kernel 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 <[email protected]>
---
include/linux/sunrpc/xdr.h | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
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;
}
--
1.8.3.1
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 <[email protected]>
---
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 @@
#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 @@ struct gss_auth {
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 @@
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)
--
1.8.3.1
On Thu, 2021-01-21 at 11:20 -0500, Dave Wysochanski wrote:
> 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 <[email protected]>
> ---
> 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 @@
> #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;
> +}
> +
Hmm... I'm not too keen on having these in sunrpc/xdr.h. For one thing,
they do not describe objects that are XDR encoded. Secondly, their
naming isn't really consistent with any of the existing conventions for
xdr.
Can we please rather just create a new header file that is private in
net/sunrpc/auth_gss/ ?
> +
> /*
> * 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 @@ struct gss_auth {
> 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 @@
> 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)
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Thu, Jan 21, 2021 at 12:05 PM Trond Myklebust
<[email protected]> wrote:
>
> On Thu, 2021-01-21 at 11:20 -0500, Dave Wysochanski wrote:
> > 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 <[email protected]>
> > ---
> > 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 @@
> > #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;
> > +}
> > +
>
> Hmm... I'm not too keen on having these in sunrpc/xdr.h. For one thing,
> they do not describe objects that are XDR encoded. Secondly, their
> naming isn't really consistent with any of the existing conventions for
> xdr.
>
> Can we please rather just create a new header file that is private in
> net/sunrpc/auth_gss/ ?
>
Sure. Do you have any preference for the name?
internal.h
auth_gss.h
something else...
> > +
> > /*
> > * 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 @@ struct gss_auth {
> > 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 @@
> > 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)
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> [email protected]
>
>
On Thu, 2021-01-21 at 12:23 -0500, David Wysochanski wrote:
> On Thu, Jan 21, 2021 at 12:05 PM Trond Myklebust
> <[email protected]> wrote:
> >
> > On Thu, 2021-01-21 at 11:20 -0500, Dave Wysochanski wrote:
> > > 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 <[email protected]>
> > > ---
> > > 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 @@
> > > #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;
> > > +}
> > > +
> >
> > Hmm... I'm not too keen on having these in sunrpc/xdr.h. For one
> > thing,
> > they do not describe objects that are XDR encoded. Secondly, their
> > naming isn't really consistent with any of the existing conventions
> > for
> > xdr.
> >
> > Can we please rather just create a new header file that is private
> > in
> > net/sunrpc/auth_gss/ ?
> >
>
> Sure. Do you have any preference for the name?
> internal.h
> auth_gss.h
> something else...
>
It is unfortunate that we already have an auth_gss.h in
include/linux/sunrpc. How about auth_gss_internal.h instead?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Thu, Jan 21, 2021 at 11:20:16AM -0500, Dave Wysochanski wrote:
> When handling an auth_gss downcall, it's possible to get 0-length
> opaque object for the acceptor. In the case of a 0-length XDR
> object, make sure simple_get_netobj() fills in dest->data = NULL,
> and does not continue to kmemdup() which will set
> dest->data = ZERO_SIZE_PTR for the acceptor.
Thanks, sounds safe to me.--b.
>
> The trace event code can handle NULL but not ZERO_SIZE_PTR for a
> string, and so without this patch the rpcgss_context trace event
> will crash the kernel 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 <[email protected]>
> ---
> include/linux/sunrpc/xdr.h | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> 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;
> }
> --
> 1.8.3.1