* [PATCH v2 0/2] Fix crash in trace_rpcgss_context due to 0-length acceptor @ 2021-01-21 16:20 Dave Wysochanski 2021-01-21 16:20 ` [PATCH v2 1/2] SUNRPC: Move simple_get_bytes and simple_get_netobj into xdr.h Dave Wysochanski 2021-01-21 16:20 ` [PATCH v2 2/2] SUNRPC: Handle 0 length opaque XDR object data properly Dave Wysochanski 0 siblings, 2 replies; 7+ messages in thread From: Dave Wysochanski @ 2021-01-21 16:20 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] SUNRPC: Move simple_get_bytes and simple_get_netobj into xdr.h 2021-01-21 16:20 [PATCH v2 0/2] Fix crash in trace_rpcgss_context due to 0-length acceptor Dave Wysochanski @ 2021-01-21 16:20 ` Dave Wysochanski 2021-01-21 17:05 ` Trond Myklebust 2021-01-21 16:20 ` [PATCH v2 2/2] SUNRPC: Handle 0 length opaque XDR object data properly Dave Wysochanski 1 sibling, 1 reply; 7+ messages in thread From: Dave Wysochanski @ 2021-01-21 16:20 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs 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 @@ #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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] SUNRPC: Move simple_get_bytes and simple_get_netobj into xdr.h 2021-01-21 16:20 ` [PATCH v2 1/2] SUNRPC: Move simple_get_bytes and simple_get_netobj into xdr.h Dave Wysochanski @ 2021-01-21 17:05 ` Trond Myklebust 2021-01-21 17:23 ` David Wysochanski 0 siblings, 1 reply; 7+ messages in thread From: Trond Myklebust @ 2021-01-21 17:05 UTC (permalink / raw) To: anna.schumaker, dwysocha; +Cc: linux-nfs 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 <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 @@ > #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 trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] SUNRPC: Move simple_get_bytes and simple_get_netobj into xdr.h 2021-01-21 17:05 ` Trond Myklebust @ 2021-01-21 17:23 ` David Wysochanski 2021-01-21 18:01 ` Trond Myklebust 0 siblings, 1 reply; 7+ messages in thread From: David Wysochanski @ 2021-01-21 17:23 UTC (permalink / raw) To: Trond Myklebust; +Cc: anna.schumaker, linux-nfs On Thu, Jan 21, 2021 at 12:05 PM Trond Myklebust <trondmy@hammerspace.com> 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 <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 @@ > > #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 > trond.myklebust@hammerspace.com > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] SUNRPC: Move simple_get_bytes and simple_get_netobj into xdr.h 2021-01-21 17:23 ` David Wysochanski @ 2021-01-21 18:01 ` Trond Myklebust 0 siblings, 0 replies; 7+ messages in thread From: Trond Myklebust @ 2021-01-21 18:01 UTC (permalink / raw) To: dwysocha; +Cc: linux-nfs, anna.schumaker On Thu, 2021-01-21 at 12:23 -0500, David Wysochanski wrote: > On Thu, Jan 21, 2021 at 12:05 PM Trond Myklebust > <trondmy@hammerspace.com> 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 <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 @@ > > > #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 trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] SUNRPC: Handle 0 length opaque XDR object data properly 2021-01-21 16:20 [PATCH v2 0/2] Fix crash in trace_rpcgss_context due to 0-length acceptor Dave Wysochanski 2021-01-21 16:20 ` [PATCH v2 1/2] SUNRPC: Move simple_get_bytes and simple_get_netobj into xdr.h Dave Wysochanski @ 2021-01-21 16:20 ` Dave Wysochanski 2021-01-21 18:19 ` J. Bruce Fields 1 sibling, 1 reply; 7+ messages in thread From: Dave Wysochanski @ 2021-01-21 16:20 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs 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 <dwysocha@redhat.com> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] SUNRPC: Handle 0 length opaque XDR object data properly 2021-01-21 16:20 ` [PATCH v2 2/2] SUNRPC: Handle 0 length opaque XDR object data properly Dave Wysochanski @ 2021-01-21 18:19 ` J. Bruce Fields 0 siblings, 0 replies; 7+ messages in thread From: J. Bruce Fields @ 2021-01-21 18:19 UTC (permalink / raw) To: Dave Wysochanski; +Cc: Trond Myklebust, Anna Schumaker, linux-nfs 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 <dwysocha@redhat.com> > --- > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-01-21 18:26 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-21 16:20 [PATCH v2 0/2] Fix crash in trace_rpcgss_context due to 0-length acceptor Dave Wysochanski 2021-01-21 16:20 ` [PATCH v2 1/2] SUNRPC: Move simple_get_bytes and simple_get_netobj into xdr.h Dave Wysochanski 2021-01-21 17:05 ` Trond Myklebust 2021-01-21 17:23 ` David Wysochanski 2021-01-21 18:01 ` Trond Myklebust 2021-01-21 16:20 ` [PATCH v2 2/2] SUNRPC: Handle 0 length opaque XDR object data properly Dave Wysochanski 2021-01-21 18:19 ` J. Bruce Fields
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.