From: Theo Buehler Subject: isakmpd: avoid undefined function pointer casts To: tech@openbsd.org Date: Thu, 17 Jul 2025 08:39:17 +0200 On Wed, Jul 16, 2025 at 09:46:22PM +0200, Theo Buehler wrote: > This is a bit of a stupid diff with lots of ugly boilerplate, but I > don't see a better way. The problem is the second sentence of > C99 6.3.2.3, 8: > > A pointer to a function of one type may be converted to a pointer to a > function of another type and back again; the result shall compare equal > to the original pointer. If a converted pointer is used to call a > function whose type is not compatible with the pointed-to type, the > behavior is undefined. > > The notion of compatibility for the functions here boils down to > compatible return type and compatible argument list, which in particular > means the same number of arguments. See 6.7.5.3, 15 for the gory details. > There's also a version understandable to a human being lacking the brain > damage necessary for successfully deciphering the C99 standard's language: > https://en.cppreference.com/w/c/language/type.html > > So calling 'void CKSUM_Final(CKSUM_CTX *);' through the final member of > struct hash_function is clearly UB since the numbers of parameters don't > match. If that was all, the fix would be trivial. But the same goes for > void MD5Final(MD5_CTX *, char *); because MD5_CTX * and void * aren't > compatible in this sense: by definition, two pointer types are compatible > if they point to compatible types. Or, if you prefer: void * has looser > alignment requirements than MD5_CTX *, so they aren't compatible. > > The below avoids this by adding wrappers that have the correct types > and that then propagate their parameters in a defined way. As I said, > it's stupid and ugly and the one upside apart from appeasing the > language lawyers I can see is that we no longer have function pointer > casts. > > PS: there is similar code in isakmpd/hash.c. Here's the corresponding change for isakmpd: Index: hash.c =================================================================== RCS file: /cvs/src/sbin/isakmpd/hash.c,v diff -u -p -r1.25 hash.c --- hash.c 21 Nov 2024 10:07:30 -0000 1.25 +++ hash.c 17 Jul 2025 06:31:42 -0000 @@ -43,7 +43,7 @@ void hmac_init(struct hash *, unsigned c void hmac_final(unsigned char *, struct hash *); /* Temporary hash contexts. */ -static union { +static union ANY_CTX { MD5_CTX md5ctx; SHA1_CTX sha1ctx; SHA2_CTX sha2ctx; @@ -54,45 +54,135 @@ static unsigned char digest[HASH_MAX]; /* Encapsulation of hash functions. */ +static void +md5_init(union ANY_CTX *ctx) +{ + MD5Init(&ctx->md5ctx); +} + +static void +md5_update(union ANY_CTX *ctx, const unsigned char *data, size_t len) +{ + MD5Update(&ctx->md5ctx, data, len); +} + +static void +md5_final(unsigned char *digest, union ANY_CTX *ctx) +{ + MD5Final(digest, &ctx->md5ctx); +} + +static void +sha1_init(union ANY_CTX *ctx) +{ + SHA1Init(&ctx->sha1ctx); +} + +static void +sha1_update(union ANY_CTX *ctx, const unsigned char *data, size_t len) +{ + SHA1Update(&ctx->sha1ctx, data, len); +} + +static void +sha1_final(unsigned char *digest, union ANY_CTX *ctx) +{ + SHA1Final(digest, &ctx->sha1ctx); +} + +static void +sha256_init(union ANY_CTX *ctx) +{ + SHA256Init(&ctx->sha2ctx); +} + +static void +sha256_update(union ANY_CTX *ctx, const unsigned char *data, size_t len) +{ + SHA256Update(&ctx->sha2ctx, data, len); +} + +static void +sha256_final(unsigned char *digest, union ANY_CTX *ctx) +{ + SHA256Final(digest, &ctx->sha2ctx); +} + +static void +sha384_init(union ANY_CTX *ctx) +{ + SHA384Init(&ctx->sha2ctx); +} + +static void +sha384_update(union ANY_CTX *ctx, const unsigned char *data, size_t len) +{ + SHA384Update(&ctx->sha2ctx, data, len); +} + +static void +sha384_final(unsigned char *digest, union ANY_CTX *ctx) +{ + SHA384Final(digest, &ctx->sha2ctx); +} + +static void +sha512_init(union ANY_CTX *ctx) +{ + SHA512Init(&ctx->sha2ctx); +} + +static void +sha512_update(union ANY_CTX *ctx, const unsigned char *data, size_t len) +{ + SHA512Update(&ctx->sha2ctx, data, len); +} + +static void +sha512_final(unsigned char *digest, union ANY_CTX *ctx) +{ + SHA512Final(digest, &ctx->sha2ctx); +} + static struct hash hashes[] = { { HASH_MD5, 5, MD5_SIZE, MD5_BLOCK_LENGTH, (void *)&Ctx.md5ctx, digest, sizeof(MD5_CTX), (void *)&Ctx2.md5ctx, - (void (*)(void *))MD5Init, - (void (*)(void *, unsigned char *, unsigned int))MD5Update, - (void (*)(unsigned char *, void *))MD5Final, + md5_init, + md5_update, + md5_final, hmac_init, hmac_final }, { HASH_SHA1, 6, SHA1_SIZE, SHA1_BLOCK_LENGTH, (void *)&Ctx.sha1ctx, digest, sizeof(SHA1_CTX), (void *)&Ctx2.sha1ctx, - (void (*)(void *))SHA1Init, - (void (*)(void *, unsigned char *, unsigned int))SHA1Update, - (void (*)(unsigned char *, void *))SHA1Final, + sha1_init, + sha1_update, + sha1_final, hmac_init, hmac_final }, { HASH_SHA2_256, 7, SHA2_256_SIZE, SHA256_BLOCK_LENGTH, (void *)&Ctx.sha2ctx, digest, sizeof(SHA2_CTX), (void *)&Ctx2.sha2ctx, - (void (*)(void *))SHA256Init, - (void (*)(void *, unsigned char *, unsigned int))SHA256Update, - (void (*)(u_int8_t *, void *))SHA256Final, + sha256_init, + sha256_update, + sha256_final, hmac_init, hmac_final }, { HASH_SHA2_384, 8, SHA2_384_SIZE, SHA384_BLOCK_LENGTH, (void *)&Ctx.sha2ctx, digest, sizeof(SHA2_CTX), (void *)&Ctx2.sha2ctx, - (void (*)(void *))SHA384Init, - (void (*)(void *, unsigned char *, unsigned int))SHA384Update, - (void (*)(u_int8_t *, void *))SHA384Final, + sha384_init, + sha384_update, + sha384_final, hmac_init, hmac_final }, { HASH_SHA2_512, 9, SHA2_512_SIZE, SHA512_BLOCK_LENGTH, (void *)&Ctx.sha2ctx, digest, sizeof(SHA2_CTX), (void *)&Ctx2.sha2ctx, - (void (*)(void *))SHA512Init, - (void (*)(void *, unsigned char *, unsigned int))SHA512Update, - (void (*)(u_int8_t *, void *))SHA512Final, + sha512_init, + sha512_update, + sha512_final, hmac_init, hmac_final } Index: hash.h =================================================================== RCS file: /cvs/src/sbin/isakmpd/hash.h,v diff -u -p -r1.9 hash.h --- hash.h 21 Nov 2024 10:07:30 -0000 1.9 +++ hash.h 17 Jul 2025 06:34:50 -0000 @@ -49,6 +49,8 @@ enum hashes { HASH_SHA2_512 }; +union ANY_CTX; + struct hash { enum hashes type; int id; /* ISAKMP/Oakley ID */ @@ -58,9 +60,9 @@ struct hash { unsigned char *digest; /* Pointer to a digest */ int ctxsize; void *ctx2; /* Pointer to a 2nd context, for HMAC octx */ - void (*Init) (void *); - void (*Update) (void *, unsigned char *, unsigned int); - void (*Final) (unsigned char *, void *); + void (*Init) (union ANY_CTX *); + void (*Update) (union ANY_CTX *, const unsigned char *, size_t); + void (*Final) (unsigned char *, union ANY_CTX *); void (*HMACInit) (struct hash *, unsigned char *, unsigned int); void (*HMACFinal) (unsigned char *, struct hash *); };