From 9d7b94493c3d0230c15417436885a4211caeb411 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 20 Oct 2022 18:31:43 -0500 Subject: [PATCH] rx: Use atomics for rx_securityClass refcounts Currently, the refCount in struct rx_securityClass is not protected by any locks. Thus, if two threads create or destroy a connection using the same rx_securityClass at the same time (or call rxs_Release), the refCount can become inaccurate. If the refCount is undercounted, we can prematurely free it while it's still referenced by other connections or services, leading to segfaults, data corruption, etc. For client connections, this can happen between any threads that create and destroy a connection using the same security class struct. For server connections, only two threads can race in this way: the rx listener thread (which creates connections), and the rx event thread (which destroys idle connections in rxi_ReapConnections). To fix this, ideally we would change the refCount field to be an rx_atomic_t. However, struct rx_securityClass is declared in the public installed rx.h header, which cannot include rx_atomic.h. So instead, change refCount users to go through a few new functions: rxs_Ref(), rxs_DecRef(), and rxs_SetRefs(). These functions interpret the refCount as an rx_atomic_t, and so allows callers to use safe refcounting without needing to call rx_atomic_* functions directly. Rename the existing refCount field to refCount_data, and declare it as a char[8]. This gives us enough space to use it as an rx_atomic_t, but avoids using rx_atomic_t in a public header, and discourages callers from manipulating the refCount directly. Thanks to mvitale@sinenomine.net for helping investigate the relevant issue. Change-Id: I55094218c79e8bc5498a6d2c1daa5620b1fceaff Reviewed-on: https://gerrit.openafs.org/15158 Reviewed-by: Cheyenne Wills Reviewed-by: Mark Vitale Tested-by: BuildBot Reviewed-by: Michael Meffie --- src/rx/rx.c | 68 ++++++++++++++++++++++++++++++++++------ src/rx/rx.h | 8 ++++- src/rx/rx_null.c | 2 +- src/rx/rx_prototypes.h | 3 ++ src/rxgk/rxgk_client.c | 16 ++-------- src/rxgk/rxgk_server.c | 16 ++-------- src/rxkad/rxkad_client.c | 2 +- src/rxkad/rxkad_common.c | 11 +++---- src/rxkad/rxkad_server.c | 2 +- 9 files changed, 82 insertions(+), 46 deletions(-) diff --git a/src/rx/rx.c b/src/rx/rx.c index 5071287e14..34906fa6d2 100644 --- a/src/rx/rx.c +++ b/src/rx/rx.c @@ -57,6 +57,7 @@ extern afs_int32 afs_termState; # include "sys/lock_def.h" # endif /* AFS_AIX41_ENV */ # include "afs/rxgen_consts.h" +# include #else /* KERNEL */ # include @@ -7392,21 +7393,70 @@ rxi_ReapConnections(struct rxevent *unused, void *unused1, void *unused2, } -/* rxs_Release - This isn't strictly necessary but, since the macro name from - * rx.h is sort of strange this is better. This is called with a security - * object before it is discarded. Each connection using a security object has - * its own refcount to the object so it won't actually be freed until the last - * connection is destroyed. - * - * This is the only rxs module call. A hold could also be written but no one - * needs it. */ - +/* + * rxs_Release - This is called with a security object before it is discarded. + * Each connection using a security object has its own refcount to the object + * so it won't actually be freed until the last connection is destroyed. + */ int rxs_Release(struct rx_securityClass *aobj) { return RXS_Close(aobj); } +static_inline rx_atomic_t * +sc2refCount(struct rx_securityClass *aobj) +{ + opr_StaticAssert(sizeof(rx_atomic_t) <= sizeof(aobj->refCount_data)); + return (rx_atomic_t *)&aobj->refCount_data; +} + +/** + * Get a reference to a security object. + * + * @param[in] aobj The security object. + * @returns The given security object. + */ +struct rx_securityClass * +rxs_Ref(struct rx_securityClass *aobj) +{ + rx_atomic_t *refCount = sc2refCount(aobj); + rx_atomic_inc(refCount); + return aobj; +} + +/** + * Puts a reference to a security object. + * + * This should only be used by securityClass implementations; users of a + * security class should use rxs_Release to release a reference to a security + * object. + * + * @param[in] aobj The security object. + * @returns The refcount of the security object (post-decrement). + */ +int +rxs_DecRef(struct rx_securityClass *aobj) +{ + rx_atomic_t *refCount = sc2refCount(aobj); + return rx_atomic_dec_and_read(refCount); +} + +/** + * Set the reference count on a security object. + * + * This should only be used by securityClass implementations. + * + * @param[in] aobj The security object. + * @param[in] refs The refCount to set. + */ +void +rxs_SetRefs(struct rx_securityClass *aobj, int refs) +{ + rx_atomic_t *refCount = sc2refCount(aobj); + rx_atomic_set(refCount, refs); +} + void rxi_DebugInit(void) { diff --git a/src/rx/rx.h b/src/rx/rx.h index bb5ea49b11..dd41a5dece 100644 --- a/src/rx/rx.h +++ b/src/rx/rx.h @@ -604,7 +604,13 @@ struct rx_securityClass { int (*op_Spare3) (void); } *ops; void *privateData; - int refCount; + + /* + * Don't use this field directly. To manipulate the refcount, callers + * should use rxs_Ref()/rxs_Release(), and (for security class + * implementations only) rxs_DecRef()/rxs_SetRefs(). + */ + char refCount_data[8]; }; #define RXS_OP(obj,op,args) ((obj && (obj->ops->op_ ## op)) ? \ diff --git a/src/rx/rx_null.c b/src/rx/rx_null.c index 809fc67b4b..cda3aa8cf5 100644 --- a/src/rx/rx_null.c +++ b/src/rx/rx_null.c @@ -25,7 +25,7 @@ /* The null security object. No authentication, no nothing. */ static struct rx_securityOps null_ops; -static struct rx_securityClass null_object = { &null_ops, 0, 0 }; +static struct rx_securityClass null_object = { &null_ops, 0, {0} }; struct rx_securityClass * rxnull_NewServerSecurityObject(void) diff --git a/src/rx/rx_prototypes.h b/src/rx/rx_prototypes.h index 00ccd07619..00bf879616 100644 --- a/src/rx/rx_prototypes.h +++ b/src/rx/rx_prototypes.h @@ -88,6 +88,9 @@ extern void rxi_CallError(struct rx_call *call, afs_int32 error); extern void rx_SetConnSecondsUntilNatPing(struct rx_connection *conn, afs_int32 seconds); extern int rxs_Release(struct rx_securityClass *aobj); +extern struct rx_securityClass *rxs_Ref(struct rx_securityClass *aobj); +extern int rxs_DecRef(struct rx_securityClass *aobj); +extern void rxs_SetRefs(struct rx_securityClass *aobj, int refs); #ifndef KERNEL extern void rx_PrintTheseStats(FILE * file, struct rx_statistics *s, int size, afs_int32 freePackets, char version); diff --git a/src/rxgk/rxgk_client.c b/src/rxgk/rxgk_client.c index a737839463..449f1f4fae 100644 --- a/src/rxgk/rxgk_client.c +++ b/src/rxgk/rxgk_client.c @@ -59,15 +59,6 @@ cprivate_destroy(struct rxgk_cprivate *cp) rxi_Free(cp, sizeof(*cp)); } -/* - * Increment the reference count on the security object secobj. - */ -static_inline void -obj_ref(struct rx_securityClass *secobj) -{ - secobj->refCount++; -} - /* * Decrement the reference count on the security object secobj. * If the reference count falls to zero, release the underlying storage. @@ -77,8 +68,7 @@ obj_rele(struct rx_securityClass *secobj) { struct rxgk_cprivate *cp; - secobj->refCount--; - if (secobj->refCount > 0) { + if (rxs_DecRef(secobj) > 0) { /* still in use */ return; } @@ -116,7 +106,7 @@ rxgk_NewClientConnection(struct rx_securityClass *aobj, if (rxgk_security_overhead(aconn, cp->level, cp->k0) != 0) goto error; rx_SetSecurityData(aconn, cc); - obj_ref(aobj); + rxs_Ref(aobj); return 0; error: @@ -502,7 +492,7 @@ rxgk_NewClientSecurityObject(RXGK_Level level, afs_int32 enctype, rxgk_key k0, if (cp == NULL) goto error; sc->ops = &rxgk_client_ops; - sc->refCount = 1; + rxs_SetRefs(sc, 1); sc->privateData = cp; /* Now get the client-private data. */ diff --git a/src/rxgk/rxgk_server.c b/src/rxgk/rxgk_server.c index 06bd46f8cb..5415f2ea85 100644 --- a/src/rxgk/rxgk_server.c +++ b/src/rxgk/rxgk_server.c @@ -47,15 +47,6 @@ #include "rxgk_private.h" -/* - * Increment the reference count on the security object secobj. - */ -static_inline void -obj_ref(struct rx_securityClass *secobj) -{ - secobj->refCount++; -} - /* * Decrement the reference count on the security object secobj. * If the reference count falls to zero, release the underlying storage. @@ -65,8 +56,7 @@ obj_rele(struct rx_securityClass *secobj) { struct rxgk_sprivate *sp; - secobj->refCount--; - if (secobj->refCount > 0) { + if (rxs_DecRef(secobj) > 0) { /* still in use */ return; } @@ -124,7 +114,7 @@ rxgk_NewServerConnection(struct rx_securityClass *aobj, sconn_set_noauth(sc); rx_SetSecurityData(aconn, sc); - obj_ref(aobj); + rxs_Ref(aobj); return 0; error: @@ -753,7 +743,7 @@ rxgk_NewServerSecurityObject(void *getkey_rock, rxgk_getkey_func getkey) return NULL; } sc->ops = &rxgk_server_ops; - sc->refCount = 1; + rxs_SetRefs(sc, 1); sc->privateData = sp; /* Now set the server-private data. */ diff --git a/src/rxkad/rxkad_client.c b/src/rxkad/rxkad_client.c index 630488374a..a363191918 100644 --- a/src/rxkad/rxkad_client.c +++ b/src/rxkad/rxkad_client.c @@ -87,7 +87,7 @@ rxkad_NewClientSecurityObject(rxkad_level level, size = sizeof(struct rx_securityClass); tsc = rxi_Alloc(size); memset((void *)tsc, 0, size); - tsc->refCount = 1; /* caller gets one for free */ + rxs_SetRefs(tsc, 1); /* caller gets one for free */ tsc->ops = &rxkad_client_ops; psize = PDATA_SIZE(ticketLen); diff --git a/src/rxkad/rxkad_common.c b/src/rxkad/rxkad_common.c index 634fa4e285..2489e989cc 100644 --- a/src/rxkad/rxkad_common.c +++ b/src/rxkad/rxkad_common.c @@ -313,8 +313,6 @@ FreeObject(struct rx_securityClass *aobj) { struct rxkad_cprivate *tcp; /* both structs start w/ type field */ - if (aobj->refCount > 0) - return 0; /* still in use */ tcp = (struct rxkad_cprivate *)aobj->privateData; rxi_Free(aobj, sizeof(struct rx_securityClass)); if (tcp->type & rxkad_client) { @@ -335,10 +333,9 @@ FreeObject(struct rx_securityClass *aobj) int rxkad_Close(struct rx_securityClass *aobj) { - afs_int32 code; - aobj->refCount--; - code = FreeObject(aobj); - return code; + if (rxs_DecRef(aobj) > 0) + return 0; /* still in use */ + return FreeObject(aobj); } /* either: called to (re)create a new connection. */ @@ -371,7 +368,7 @@ rxkad_NewConnection(struct rx_securityClass *aobj, INC_RXKAD_STATS(connections[rxkad_LevelIndex(tcp->level)]); } - aobj->refCount++; /* attached connection */ + rxs_Ref(aobj); /* attached connection */ return 0; } diff --git a/src/rxkad/rxkad_server.c b/src/rxkad/rxkad_server.c index 07e806bcdf..d163085096 100644 --- a/src/rxkad/rxkad_server.c +++ b/src/rxkad/rxkad_server.c @@ -147,7 +147,7 @@ rxkad_NewServerSecurityObject(rxkad_level level, void *get_key_rock, size = sizeof(struct rx_securityClass); tsc = rxi_Alloc(size); memset(tsc, 0, size); - tsc->refCount = 1; /* caller has one reference */ + rxs_SetRefs(tsc, 1); /* caller has one reference */ tsc->ops = &rxkad_server_ops; size = sizeof(struct rxkad_sprivate); tsp = rxi_Alloc(size);