From e2ec16cf941b0aadfbd54fc2f52edd58b62e232d Mon Sep 17 00:00:00 2001 From: Mark Vitale Date: Fri, 15 Sep 2023 15:01:56 -0400 Subject: [PATCH] dir: Introduce struct DirEntryFlex The directory package as implemented in AFS-2 allocates space for each directory entry as a DirEntry struct followed by 0-8 contiguous DirXEntry structs, as needed. This is implemented by: - afs_dir_NameBlobs calculates the number of blocks needed - FindBlobs allocates and returns index of entry - afs_dir_GetBlob returns pointer to 1st DirEntry struct After this, we populate DirEntry (and any contiguous DirXEntry blocks) with open code. Most existing code writes the entry's name via a string copy operation to DirEntry->name, which is only 16 bytes long. Therefore, for dir entry names that are 16 bytes or longer, OpenAFS routinely does string copies that look like buffer overruns. This has not previously caused problems because the OpenAFS code has arranged for a sufficiently large amount of contiguous memory to be available. However, this remains undefined behavior in the C abstract virtual machine; thus compilers are not required to produce safe operation. Recent changes in the OpenAFS build chain have made this approach no longer viable: 1) Linux 6.5 commit df8fc4e934c12b 'kbuild: Enable -fstrict-flex-arrays=3' modified the hardening of several kernel string operations when running with CONFIG_FORTIFY_SOURCE=y. 2) gcc 13 commit 79a89108dd352cd9288f5de35481b1280c7588a5 '__builtin_dynamic_object_size: Recognize builtin' provides some enhancements to _builtin_object_size. The Linux commit above will now use these when the kernel is built with gcc 13. When OpenAFS is built under Linux 6.5 or higher and gcc 13 or higher, the hardened strlcpy will BUG for directory entry names longer than 16 characters. Since there are multiple places where OpenAFS writes directory names, there are several symptoms that may manifest. However, the first one is usually a kernel BUG at cache manager initialization if running with afsd -dynroot _and_ there are any cell names 15 characters or longer in the client CellServDB. (A 15-character cellname reaches the 16 character limit when -dyrnoot adds the RW mountpoint ".".) Address this by using flexible arrays (standardized with C99). A flexible array is a variable-length array that is declared with no size at all, e.g., name[]. Create an autoconf test to determine whether the compiler supports flexible arrays. Create a new struct DirEntryFlex. If the compiler supports flexible arrays, define name[]; otherwise retain the name[16] definition. Whenever we write a directory name, use DirEntryFlex so that any hardening will be satisfied that there is sufficient space for the name. However, the actual guarantee that this is true is still provided by the OpenAFS directory routines mentioned above - all of these remain unchanged. The DirEntry struct remains unchanged for continued use in OpenAFS, as well as for any out-of-tree users of the directory package. Change-Id: I6da5c6c295f051be90017084e5b3a3ef24d1271f Reviewed-on: https://gerrit.openafs.org/15573 Tested-by: BuildBot Reviewed-by: Cheyenne Wills Reviewed-by: Benjamin Kaduk Reviewed-by: Michael Meffie --- acinclude.m4 | 1 + src/afs/LINUX/osi_vnodeops.c | 4 ++-- src/afs/afs_dynroot.c | 4 ++-- src/cf/c-flexible-array.m4 | 16 ++++++++++++++++ src/dir/dir.c | 4 ++-- src/dir/dir.h | 26 ++++++++++++++++++++++++++ 6 files changed, 49 insertions(+), 6 deletions(-) create mode 100644 src/cf/c-flexible-array.m4 diff --git a/acinclude.m4 b/acinclude.m4 index 209fa34099..4df1bcde2e 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -56,6 +56,7 @@ OPENAFS_HCRYPTO OPENAFS_CURSES OPENAFS_C_ATTRIBUTE OPENAFS_C_PRAGMA +OPENAFS_C_FLEXIBLE_ARRAY OPENAFS_MORE_ROKEN_CHECKS OPENAFS_NETDB_CHECKS OPENAFS_ROKEN_HEADERS diff --git a/src/afs/LINUX/osi_vnodeops.c b/src/afs/LINUX/osi_vnodeops.c index 0f48417171..b56d467f81 100644 --- a/src/afs/LINUX/osi_vnodeops.c +++ b/src/afs/LINUX/osi_vnodeops.c @@ -426,7 +426,7 @@ afs_linux_readdir(struct file *fp, void *dirbuf, filldir_t filldir) int code; int offset; afs_int32 dirpos; - struct DirEntry *de; + struct DirEntryFlex *de; struct DirBuffer entry; ino_t ino; int len; @@ -531,7 +531,7 @@ afs_linux_readdir(struct file *fp, void *dirbuf, filldir_t filldir) goto unlock_out; } - de = (struct DirEntry *)entry.data; + de = entry.data; ino = afs_calc_inum (avc->f.fid.Cell, avc->f.fid.Fid.Volume, ntohl(de->fid.vnode)); len = strlen(de->name); diff --git a/src/afs/afs_dynroot.c b/src/afs/afs_dynroot.c index 042a50b6fb..f19343f649 100644 --- a/src/afs/afs_dynroot.c +++ b/src/afs/afs_dynroot.c @@ -228,7 +228,7 @@ afs_dynroot_addDirEnt(struct DirHeader *dirHeader, int *curPageP, { char *dirBase = (char *)dirHeader; struct PageHeader *pageHeader; - struct DirEntry *dirEntry; + struct DirEntryFlex *dirEntry; int sizeOfEntry, i, t1, t2; int curPage = *curPageP; int curChunk = *curChunkP; @@ -257,7 +257,7 @@ afs_dynroot_addDirEnt(struct DirHeader *dirHeader, int *curPageP, dirHeader->alloMap[curPage] = EPP - 1; } - dirEntry = (struct DirEntry *)(pageHeader + curChunk); + dirEntry = (struct DirEntryFlex *)(pageHeader + curChunk); dirEntry->flag = 1; dirEntry->length = 0; dirEntry->next = 0; diff --git a/src/cf/c-flexible-array.m4 b/src/cf/c-flexible-array.m4 new file mode 100644 index 0000000000..e281166f82 --- /dev/null +++ b/src/cf/c-flexible-array.m4 @@ -0,0 +1,16 @@ +AC_DEFUN([OPENAFS_C_FLEXIBLE_ARRAY],[ + dnl Check to see if the compiler support C99 flexible arrays, e.g., var[] + AC_MSG_CHECKING([for C99 flexible arrays]) + AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ + struct flexarraytest { + int flag; + int numbers[]; + }; + ]], [[]]) + ], + [AC_MSG_RESULT([yes]) + AC_DEFINE([HAVE_FLEXIBLE_ARRAY], [1], + [Define to 1 if your compiler supports C99 flexible arrays.]) + ],[AC_MSG_RESULT([no])] + ) +]) diff --git a/src/dir/dir.c b/src/dir/dir.c index 84a5e2cf49..1fb64a2e92 100644 --- a/src/dir/dir.c +++ b/src/dir/dir.c @@ -97,7 +97,7 @@ afs_dir_Create(dir_file_t dir, char *entry, void *voidfid) int blobs, firstelt; int i; struct DirBuffer entrybuf, prevbuf, headerbuf; - struct DirEntry *ep; + struct DirEntryFlex *ep; struct DirHeader *dhp; int code; size_t rlen; @@ -125,7 +125,7 @@ afs_dir_Create(dir_file_t dir, char *entry, void *voidfid) /* First, we fill in the directory entry. */ if (afs_dir_GetBlob(dir, firstelt, &entrybuf) != 0) return EIO; - ep = (struct DirEntry *)entrybuf.data; + ep = entrybuf.data; ep->flag = FFIRST; ep->fid.vnode = htonl(vfid[1]); diff --git a/src/dir/dir.h b/src/dir/dir.h index 39486895bb..25c450eb9f 100644 --- a/src/dir/dir.h +++ b/src/dir/dir.h @@ -51,6 +51,32 @@ struct DirHeader { unsigned short hashTable[NHASHENT]; }; +/* + * This struct is just a copy of DirEntry, but with name defined as a flexible + * array if possible. + * + * Using this helps us convince safety-minded string functions (e.g. + * _FORTIFY_SOURCE) that an OpenAFS directory entry name really does fit + * in the allotted space, and thus avoid undefined behavior. + */ +struct DirEntryFlex { + char flag; + char length; /* currently unused */ + unsigned short next; + struct MKFid fid; +#ifdef HAVE_FLEXIBLE_ARRAY + char name[]; +#else + char name[16]; +#endif +}; + +/* + * This struct was the original format for directory entries in very early + * versions of AFS. But now it just represents the minimum possible on-disk + * representation of a directory entry. The 16-character limit was relieved by + * the introduction of extension struct DirXEntry in AFS-2. +*/ struct DirEntry { /* A directory entry */ char flag;