clang-19 is flagging a warning/error in rxkad/test/stress.c as
unused-but-set-variables
stress.c:51:9: error: variable 'nonoauth' set but not used [-Werror,-Wunused-but-set-variable]
51 | int nonoauth = 0;
| ^
1 error generated.
The variable nonoauth is initialized and conditionally incremented, but
is not used elsewhere.
Remove the nonoauth variable.
Change-Id: I9a97a3980e5b6ff462f860d7bdae4ee8d7b22971
Reviewed-on: https://gerrit.openafs.org/15984
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Convert the prefpane logic for writing to config files to use
privhelper, so it can work with macOS 10.8+. With this commit, the
"CellServDB Editor" and "Parameter" tabs in the prefpane should now
work.
Specifically, define the privhelper task "write". Define a new variant
to TaskUtil's executePrivTask() called executePrivTaskWrite() to use the
new task type, and convert the prefpane code to use it.
Most files are straightforward, but converting writeAfsdOption() is a
little awkward due to the repeated checking of useAfsdConfVersion. Just
remove the writeAfsdOption() method and make the caller choose between
writeNewAfsdOption() and writeOldAfsdOption().
The new "write" task takes a new argument (data) to indicate what data
to write to the config file in question. Like the "backup" task, it also
takes a "filename" argument that is checked against our list of config
files.
Change-Id: Ib93da5a75a92e3507c15f9ee03eb8f7fbad7d088
Reviewed-on: https://gerrit.openafs.org/15960
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Convert the prefpane logic to backup various config files to privhelper,
so it can work with macOS 10.8+.
Specifically, define the new privhelper task "backup". Define a new
variant to TaskUtil's executePrivTask() called executePrivTaskBackup()
to use the new task type, and convert the prefpane code to use it.
Remove the backupFile() method, since it's now unused.
The new "backup" task has an argument (filename), so we don't need to
define separate tasks for each file we want to backup. But check the
given filename against a short list of files we know we deal with, just
to make sure we don't accidentally mess with some other system file,
since we're running as root.
Change-Id: I066d8e95a50a52f7509487f9228ba03dd027ea36
Reviewed-on: https://gerrit.openafs.org/15959
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Convert the logic to "start/stop OpenAFS" in the prefpane to use
privhelper, so it can work with macOS 10.8+.
Specifically, define these new privhelper tasks:
- afsd_start
- afsd_stop
And convert our start/stop-related code in the prefpane to use them.
Change-Id: I4cc5b500fb09b83a4007e2c7c5045e93fe1f4ced
Reviewed-on: https://gerrit.openafs.org/15958
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Convert the logic for the "start at login" option in the prefpane to use
privhelper, so it can work with macOS 10.8+.
Specifically, define these new privhelper tasks:
- startup_enable
- startup_disable
- startup_check
And convert our startup-related logic in the prefpane to use them.
Get rid of our now-unused method executeTaskWithAuth() and related
methods.
Change-Id: I2cb4c31f964529ab1af43ab7828c14eba7354af0
Reviewed-on: https://gerrit.openafs.org/15957
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
The prefpane for macOS runs as the logged-in user, but needs root access
for some operations: starting/stopping the client, editing various
configuration files like CellServDB, etc. We currently use functions
like AuthorizationExecuteWithPrivileges() to run commands with root
privileges directly, but this approach no longer works as of macOS 10.8
(Mountain Lion); the relevant functions have been removed.
Instead, a new approach exists as of macOS 10.6 (Snow Leopard). The
prefpane application itself cannot gain root privileges, but we can
provide another daemon process that runs as root, and the PrefPane sends
requests to that process to perform the privileged operations we need.
In this commit, create a separate helper program called PrivilegedHelper
(privhelper for short) that serves this purpose. Define the
executePrivTask() method in TaskUtil to handle communicating with
privhelper over XPC.
This commit does not define any of the tasks that privhelper will
actually perform; this just implements privhelper itself. Later commits
will add and use various privileged tasks in privhelper.
In order for privhelper to be able to run as root, both privhelper and
the prefpane itself must be code signed and the relevant apple team id
must be specified in their Info.plist when they are built, as well as
inside privhelper.c. Currently, we have
no way of specifying code signatures info during the build, since all code
signing is done when generating packages (via pkgbuild.sh) after
binaries are built. For now, just put a commented-out section in
src/platform/DARWIN/AFSPreference/Info.plist and
src/platform/DARWIN/PrivilegedHelper/privhelper-info.plist and a
placeholder in src/platform/DARWIN/PrivilegedHelper/privhelper.c to show
how to add this information. The package builder must add their own team
id to these before privhelper can work properly.
The privhelper tool checks that the calling user has authorization to
run commands as root (via AuthorizationCopyRights()), and that the
calling process is either our AFSBackgrounder menu bar or the prefpane.
We use xpc_connection_set_peer_code_signing_requirement() for this where
available, but fallback to using SecCodeCheckValidity() with
SecCodeCreateWithXPCMessage() or
xpc_dictionary_get_audit_token()/SecCodeCopyGuestWithAttributes() if
needed.
Change-Id: I724b6d486ee5397c89c79e589ddcb2a5987a895b
Reviewed-on: https://gerrit.openafs.org/15956
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
The default Xcode compiler options are insufficient to fail the build if
a method is not found. For example, a typo in the name of method
'componentsJoinedByString' results in the following build warning:
/TaskUtil.m:70:62: warning: instance method '-componentsJoinedBySting:'
not found (return type defaults to 'id') [-Wobjc-method-access]
NSLog(@"Task failed: %@ %@ status:%d.", taskName,
[args componentsJoinedBySting:@", "], status);
Because this is only flagged as a warning, the build completes
successfully. When this code runs, the AFSBackgrounder merely logs a
runtime exception every time it passes through the erroneous code. This
is a silent failure, unless you happen to know how to check for
AFSBackgrounder log messages:
$ log show --predicate 'process=="AFSBackgrounder"'
Add compiler flag '-Werror=objc-method-access' to all of our xcode-built
project files to treat this case as an error instead of a warning, so
the build will fail.
[adeason@sinenomine.net: Update all xcode project files.]
Change-Id: I69a9f45deb6710a50590bd79daf07466332a6ad1
Reviewed-on: https://gerrit.openafs.org/14586
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Commit 91378d93b9 (rx: Don't send packets to localhost if -rxbind set)
added the rxi_GetLocalAddr() function, but missed the prototype. Add the
function prototype to fix a GCC missing-prototype warning when building
the linux kernel module.
Fixes the build error when building the kernel module for Linux 6.8
or later when the tree was configured with --enable-checking:
.../src/libafs/MODLOAD-6.8.0-49-generic-SP/rx.c:9693:1: error: no
previous prototype for ‘rxi_GetLocalAddr’ [-Werror=missing-prototypes]
9693 | rxi_GetLocalAddr(struct sockaddr_in *sin)
| ^~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Change-Id: I43bd56fa28d258be509b1d1381e2f7d76ad5a532
Reviewed-on: https://gerrit.openafs.org/15978
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
The comments above VCHash claim that PFlush relies on the fid uniq not
being used in VCHash, but this doesn't appear to be true.
However, we do have some code that relies on VCHash not using uniq:
afs_GenFakeFid. Update the VCHash comment to point this out.
Change-Id: Ib1e0c1700c4cd13b95ac57522645ce73d2b24a08
Reviewed-on: https://gerrit.openafs.org/15042
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
For some platforms (SOLARIS, FBSD), shutting down the libafs client
currently involves sending a packet to localhost to wakeup the listener
thread. If rx is bound to a specific host address (the -rxbind option
was passed to afsd), this won't work, because rx won't receive packets
sent to localhost. This results in the client hanging forever when
trying to 'umount afs', until a packet is otherwise sent to the rx
socket.
To fix this, send the packet to the bound host address instead, if
-rxbind was given. Otherwise, send the packet to localhost, like before.
Introduce the small helper function rxi_GetLocalAddr() to consolidate
the logic of what address to use.
Change-Id: I26fa5194b726ed753779faa07142fec647228b44
Reviewed-on: https://gerrit.openafs.org/15906
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: Andrew Deason <adeason@sinenomine.net>
Remember what local address we're bound to (as passed to rx_InitHost(),
which is given when -rxbind is passed to various programs), like we do
for our local port with rx_port. Store this in a new internal global
variable, rx_host.
This commit just introduces the new variable, but does not use it.
Future commits will add code that make use of this information.
Change-Id: I5ac1a60b10f6c28df1e6620e30515bcdfd749311
Reviewed-on: https://gerrit.openafs.org/15905
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
The indexGreaterThanIndex function returns the closest index from the
index set that is greater than a specified index. This function is
typically used in a loop to iterate through a list of items until it
returns NSNotFound, indicating the end of the list.
In AFSCommanderPref.m, the following pattern is being used:
int index = 0;
do {
...
} while((index = [... indexGreaterThanIndex:index]) != NSNotFound);
The issue arises because indexGreaterThanIndex returns an NSUInteger,
while the loop uses an int for index. If NSNotFound is cast to an int,
it becomes -1, causing the loop to never terminate and leading to a
crash.
To fix this problem, change the type of index from int to NSUInteger to
ensure proper comparison and termination of the loop when NSNotFound is
returned.
Change-Id: I0b8b38d201b500c2d6d7778f0132ce17f7c04ac8
Reviewed-on: https://gerrit.openafs.org/15961
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Address potential memory issues by setting workIPArray to nil after
releasing it in commitModify. This prevents dangling pointers and
ensures consistency with rollbackModify, which already includes this
safeguard. Without this change, PrefPane can crash upon closing the
window used for setting IP addresses of cells in the CellServDB.
Change-Id: Iea56343d734f854b76b74c961d2df05bcf4a9332
Reviewed-on: https://gerrit.openafs.org/15962
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
On FreeBSD, we commonly generate some kernel header files in
src/libafs/kconf-GENERIC, ever since commit a9c1939eeb (FBSD: Use
GENERIC kernel headers by default). Ignore it.
Change-Id: I2ce14361b130f20861133a8358aaa4085073e897
Reviewed-on: https://gerrit.openafs.org/15902
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Ben Huntsman <ben@huntsmans.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Most of our binaries link with -lhcrypto or our internally-built
-lafshcrypto for heimdal-provided crypto code. But we also build an LWP
variant, libafshcrypto_lwp.a, which avoids any pthread calls and is to
be used when linking against LWP instead of pthreads.
Currently, a few binaries link against LWP, but also link against
$(LIB_hcrypto) (which gets expanded to -lhcrypto or -lafshcrypto). This
includes most binaries in src/tests, src/rx/test, and src/rxkad/test,
and has been the case since these commits:
236cb51b83 rx: Cleanup and build src/rx/test
1fe1dac4c5 rxkad: Cleanup and build src/rxkad/test
80c23d958c tests: Make src/tests buildable
On most platforms, this isn't so noticeable; the binaries just get built
for LWP, but have a pthreaded hcrypto and contain references to
pthreads. (hcrypto itself just uses a single global mutex that is
briefly held, and so tends to not cause cause any problems running with
LWP.)
On AIX, this causes a build error when building with --disable-shared,
because we build these programs without linking to the pthreads library:
/opt/IBM/xlC/16.1.0/bin/cc [...] -o test-setgroups test-setgroups.o [...]/lib/libopr.a -lafshcrypto -lrokenafs
ld: 0711-317 ERROR: Undefined symbol: .pthread_mutex_lock
ld: 0711-317 ERROR: Undefined symbol: .pthread_mutex_unlock
ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information.
make: *** [test-setgroups] Error 8
(Without --disable-shared, this succeeds, presumably because we link
against the libtool libafshcrypto.la library, which is smart enough to
know it depends on pthreads.)
To fix this, link against libafshcrypto_lwp.a for these LWP binaries
instead of $(LIB_hcrypto), like other LWP binaries do. We could maybe
change $(LIB_hcrypto) to expand to libafshcrypto_lwp.a for Makefile.lwp,
but that can be confusing, especially for directories that build both
LWP and pthreaded binaries.
Written in collaboration with ben@huntsmans.net, who noticed the issue
and provided testing.
Change-Id: Ic4eef01c40e3ecdc4a8dc999b21273d7da364d34
Reviewed-on: https://gerrit.openafs.org/15904
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Ben Huntsman <ben@huntsmans.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
scan-build identified these as unused operations that can be removed.
Change-Id: I6fe7c3c23cfcbecbe8fa539131aec779c42876c2
Reviewed-on: https://gerrit.openafs.org/13305
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Several static analysis tools have identified various problems:
- files left open from early returns/exits (infer)
- possible memory leaks (scan-build, cppcheck)
To resolve the above problems:
- close files before returning/exiting
- fix possible memory leaks by freeing memory
This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Change-Id: Id8d25bacd2d1e1dee32354248519e3821ffe726c
Reviewed-on: https://gerrit.openafs.org/14681
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Several static analysis tools have identified various problems:
- possible memory leaks (scan-build, cppcheck)
To resolve the above problems:
- fix possible memory leaks by freeing memory
This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Change-Id: Ica9e5a33bf33ae802fed0f1439fc3561d0406713
Reviewed-on: https://gerrit.openafs.org/14685
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: PatRiehecky <jcpunk@gmail.com>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Several static analysis tools have identified various problems:
- use of uninitlized variables (scan-build)
- missing checks to ensure *alloc was successful (infer)
- possible memory leaks (scan-build, cppcheck)
To resolve the above problems:
- ensure variables are initialized to safe defaults
- add checks to ensure *alloc was successful before using the memory
- fix possible memory leaks by freeing memory
This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Change-Id: Ic5e222e344df4d5a3e317fcbb2f30a09dce93793
Reviewed-on: https://gerrit.openafs.org/14679
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Several static analysis tools have identified various problems:
- missing checks to ensure *alloc was successful (infer)
To resolve the above problems:
- Add asserts to verify that the *alloc was successful before using the
memory.
This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Note: Most of the callers to cmd_AddParmAlias and cmd_OptionAsList do
not check the returned value, so asserts are used instead of returning
ENOMEM.
Change-Id: Ic44f6d34240c13c2626567f9d698d2480d2e8672
Reviewed-on: https://gerrit.openafs.org/14680
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Several static analysis tools have identified various problems:
- printf conversion specifiers (cppcheck)
- sscanf string overflows (cppcheck)
- files left open from early exits (infer)
- dereference of NULL pointer (scan-build)
- missing checks to ensure *alloc was successful (infer)
- possible memory leaks (scan-build, cppcheck)
To resolve the above problems:
- update printf conversion specifiers
- update sscanf format strings to include the size of the buffer
- close files before exiting
- ensure pointers are non-NULL prior to use
- add checks to ensure *alloc was successful before using the memory
- fix possible memory leaks by freeing memory
This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Change-Id: I864b2c6be0c7dec1e93b49231ad7876d38c20435
Reviewed-on: https://gerrit.openafs.org/14677
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Clang's scan-build static analysis tool flagged a 'Use of memory after
it is freed'
The function _DirUpdate can reallocate the storage that is pointed to
by the dirbuffer member of the DirHeader structure.
The code in afscp_DirLookup initializes a local variable to d->dirbuffer
before calling _DirUpdate.
Update afscp_dir.c to initialize the local variable 'h' after calling
_DirUpdate.
Change-Id: Ibfc8b25ac31998f8581185f896737ca4bc27da7e
Reviewed-on: https://gerrit.openafs.org/14713
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
In the project root .gitignore, fix a typo which was causing the
built products directory to not be properly ignored.
Change-Id: Iea83a135b6f5a4c60c76df7c8bfcc65df90e1b39
Reviewed-on: https://gerrit.openafs.org/15903
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Several static analysis tools have identified various problems:
- missing checks to ensure *alloc was successful (infer)
To resolve the above problems:
- add checks to ensure *alloc was successful before using the memory
This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Change-Id: I48e053e73131c3f86928a4658dbc69e56abd2b20
Reviewed-on: https://gerrit.openafs.org/14683
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Currently, if the dynamic vcaches feature is enabled, the cache manager
deallocates batches of vcaches (allocated on demand) in the background.
Since this procedure is executed once every 5 minutes (at most), the
number of vcaches can grow unlimitedly between executions, increasing
the cost of using and maintaining them during this timeframe.
To prevent this from happening, run afs_ShakeLooseVCaches() sooner if we
have way more vcaches than the configured threshold.
Change-Id: I10ce7487441c659463ec85b4c8390274c11ef43f
Reviewed-on: https://gerrit.openafs.org/15044
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
On some platforms (such as LINUX), unlinked vcaches are not destroyed
until the next execution of afs_ShakeLooseVCaches(). Since the hash for
a given vcache is computed from its volume id and vnode number, keeping
unlinked vcaches around can be a problem if files are repeatedly deleted
and recreated in the same volume. In this scenario, the same vnode
number will be reused several times, resulting in many vcaches with the
same volume id and vnode number (but different unique ids) in the same
bucket. Consequently, finding a given vcache in the bucket in question
can be time consuming (and cause extra cpu processing), as we would have
to traverse a long linked list.
To mitigate this problem, move vcaches associated with unlinked files to
the least recently used position in our VLRU, prioritizing their removal
the next time afs_ShakeLooseVCaches() is executed.
Introduce the QAddEnd() macro to make it easier to add a vcache to the
end of the VLRU.
Change-Id: Idc74da0c3c0b27bfdf2cd491b003bdd42c889a95
Reviewed-on: https://gerrit.openafs.org/14961
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Linux-6.12 commit 'fs: Convert aops->write_begin to take a folio'
(1da86618bdce3) changed the address_space_operations's members
write_begin and write_end to use a folio instead of a page.
Add configure check to test the signature for aop's write_begin and
write_end members to see if they take a folio parameter.
Update the afs_linux_write_begin and afs_linux_write_end functions to
use a folio instead of a page.
Change-Id: Idd38aa3a9871e188580aae86f5b22621a5511bb2
Reviewed-on: https://gerrit.openafs.org/15898
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
The function afs_linux_write_begin() has 2 preprocessor selected
implementations, one to handle the case where write_begin has a flag
parameter and the other where it doesn't.
Refactor the code to combine the 2 implementations using preprocessor
conditionals for the function declaration and within the body of the
function as needed.
There are no functional changes.
This refactoring is in preparation for additional changes that will be
made to the afs_linux_write_begin() function.
Change-Id: I5389ea562d834d847bc7413e6f58dc3be2e5aa31
Reviewed-on: https://gerrit.openafs.org/15897
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
The Linux 6.12 commit 'mm: remove PG_error' (09022bc196d23) removed
the PG_error page flag and the associated ClearPageError() and
SetPageError() functions (via removing the PAGEFLAG(Error, ...) macro).
The PG_error flag has not been used by core VFS/MM Linux code for some
time, possibly ever, and so our calls to these functions do not have any
practical effect, since we also do not check for the PG_error flag.
While we could simply remove these calls, play it safe and keep them
around until ClearPageError()/SetPageError() are removed.
The specific semantics of the PG_error flag are not completely well
defined in the Linux kernel, which appears to be one of the motivations
for its removal. Some Linux commits that mention some details on how the
flag is not useful for read errors include:
7edf1ec5b249 ceph: don't SetPageError on readpage errors
41a638a1b3fc affs: convert affs_symlink_read_folio() to use the folio
2b2553f12355 btrfs: stop setting PageError in the data I/O path
Add a configure test to see if ClearPageError()/SetPageError() are
available in the Linux kernel; if they are not, define
ClearPageError()/SetPageError() as no-ops.
Change-Id: I2d3235d81030aa0b30907336f58a7d9c5c7b8026
Reviewed-on: https://gerrit.openafs.org/15876
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Tested-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Commit 1f5e1ef9e3 (OPENAFS-SA-2024-003: Run xdr_free for retried RPCs)
added a couple of references to xdr_namelist, which currently causes a
build failure on AIX:
/bin/sh ../../libtool --quiet --mode=link --tag=CC xlc_r [...] -o pts pts.o ../../src/ptserver/liboafs_prot.la [...]
ld: 0711-317 ERROR: Undefined symbol: xdr_namelist
ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information.
make: 1254-004 The error code from the last command is 8.
To avoid this, add xdr_namelist to liboafs_prot.la.sym.
Change-Id: I8c9a7a64efdb9107e27a3b3502e5d96272f5341e
Reviewed-on: https://gerrit.openafs.org/15954
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Update version strings for the third 1.9.x development release.
Change-Id: I203a9d3487d1344882afb02ded094e01e7672753
Reviewed-on: https://gerrit.openafs.org/15926
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
Currently, there are a few callers of RPCs that specify some data for
an INOUT parameter, but do not initialize the memory for that data.
This can result in the uninitialized memory being sent to the peer
when the argument is processed as an IN argument. Simply clear the
relevant data before running the RPC to avoid this.
The relevant RPCs and arguments are:
- For RMTSYS_Pioctl, the 'OutData' argument.
- For BUDB_GetVolumes, the 'volumes' argument.
-- via DBLookupByVolume -> bcdb_LookupVolume -> ubik_BUDB_GetVolumes
-- and via bc_Restorer -> bcdb_FindVolumes -> ubik_BUDB_GetVolumes
- For KAA_Authenticate_old / KAA_Authenticate, this can happen with
the 'answer' argument in ka_Authenticate if KAA_AuthenticateV2 or
KAA_Authenticate return RXGEN_OPCODE, but the server manages to
populate oanswer.SeqLen with non-zero.
For all of these, make sure the memory is blanked before running the
relevant RPC. For ka_Authenticate, reset oanswer.SeqLen to 0 to avoid
sending any data, but still blank 'answer' and 'answer_old' just to be
safe.
FIXES 135043
Change-Id: I95ee91a1710d66e2e88c3086d57c279a376f7dc6
Reviewed-on: https://gerrit.openafs.org/15925
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
Here, 'OutData' only has OutData.rmtbulk_len bytes in it. We know that
OutData.rmtbulk_len is at most data->out_size, but it could be
smaller. So, only copy OutData.rmtbulk_len bytes, not data->out_size,
since data->out_size could be more than the number of bytes we have
allocated in OutData.
FIXES 135043
Change-Id: I6f87fc8cb5df0298061f419112200f6c7e1974ba
Reviewed-on: https://gerrit.openafs.org/15924
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
A few areas of code retry the same RPC, like so:
do {
code = VL_SomeRPC(rxconn, &array_out);
} while (some_condition);
xdr_free((xdrproc_t) xdr_foo, &array_out);
Or try a different version/variant of an RPC (e.g.
VLDB_ListAttributesN2 -> VLDB_ListAttributes).
If the first RPC call causes the output array to be allocated with
length N, then the subsequent RPC calls may fail if the server
responds with an array larger than N.
Furthermore, if the subsequent call responds with an array smaller
than N, then when we xdr_free the array, our length will be smaller
than the actual number of allocated elements. That results in two
potential issues:
- We'll fail to free the elements at the end of the array. This is
only a problem if each element in the array also uses
dynamically-allocated memory (e.g. each element contains a string or
another array). Fortunately, there are only a few such structures in
any of our RPC-L definitions: SysNameList and CredInfos. And neither
of those are used in such a retry loop, so this isn't a problem.
- We'll give the wrong length to osi_free when freeing the array
itself. This only matters for KERNEL, and only on some platforms
(such as Solaris), since the length given to osi_free is ignored
everywhere else.
To avoid these possible issues, change the relevant retry loops to
free our xdr-allocated arrays on every iteration of the loop, like
this:
do {
xdr_free((xdrproc_t) xdr_foo, &array_out);
code = VL_SomeRPC(rxconn, &array_out);
} while (some_condition);
xdr_free((xdrproc_t) xdr_foo, &array_out);
Or like this:
do {
code = VL_SomeRPC(rxconn, &array_out);
xdr_free((xdrproc_t) xdr_foo, &array_out);
} while (some_condition);
FIXES 135043
Change-Id: I4e058eb52d277e6d3817df8b703c5e5b894c0b5a
Reviewed-on: https://gerrit.openafs.org/15923
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
Currently, if a caller calls an RPC with a string output argument,
like so:
{
char *str = NULL;
code = RXAFS_SomeCall(&str);
/* do something with 'str' */
xdr_free((xdrproc_t) xdr_string, &str);
}
Normally, xdr_free causes xdr_string to call osi_free, specifying the
same size that we allocated for the string. However, since we only
have a char*, the amount of space allocated for the string is not
recorded separately, and so xdr_string calculates the size of the
buffer to free by using strlen().
This works for well-formed strings, but if we fail to decode the
payload of the string, or if our peer gave us a string with a NUL byte
in the middle of it, then strlen() may be significantly less than the
actual allocated size. And so in this case, the size given to osi_free
will be wrong.
The size given to osi_free is ignored in userspace, and for KERNEL on
many platforms like Linux and DARWIN. However, it is notably not
ignored for KERNEL on Solaris and some other less supported platforms
(HPUX, Irix, NetBSD). At least on Solaris, an incorrect size given to
osi_free can cause a system panic or possibly memory corruption.
To avoid this, change xdr_string during XDR_DECODE to make sure that
strlen() of the string always reflects the allocated size. If we fail
to decode the string's payload, replace the payload with non-NUL bytes
(fill it with 'z', an arbitrary choice). And if we do successfully
decode the payload, check if the strlen() is wrong (that is, if the
payload contains NUL '\0' bytes), and fail if so, also filling the
payload with 'z'. This is only strictly needed in KERNEL on certain
platforms, but do it everywhere so our behavior is consistent.
FIXES 135043
Change-Id: I90c419a7ef0ede247187172a182863dcb4250578
Reviewed-on: https://gerrit.openafs.org/15922
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
Various RPCs return a variable-length array in an OUT argument, but
are only supposed to return specific sizes. A few instances of this
include the following (but this is not an exhaustive list):
- AFSVolListOneVolume should only return a single volintInfo.
- PR_NameToID should return the same number of IDs as names given.
- VL_GetAddrsU should return the same number of addresses as the
'nentries' OUT argument.
Some callers of these RPCs just assume that the server has not
violated these rules. If the server responds with a nonsensical array
size, this could cause us to read beyond the end of the array, or
cause a NULL dereference or other errors.
For example, some callers of VL_GetAddrsU will iterate over 'nentries'
addresses, even if the 'blkaddrs' OUT argument contains fewer entries.
Or with AFSVolListOneVolume, some callers assume that at least 1
volintInfo has been returned; if 0 have been returned, we can try to
access a NULL array.
To avoid all of this, add various sanity checks on the relevant
returned lengths of these RPCs. For most cases, if the lengths are not
sane, return an internal error from the appropriate subsystem (or
RXGEN_CC_UNMARSHAL if there isn't one). For VL_GetAddrsU, if
'nentries' is too long, just set it to the length of the returned
array.
FIXES 135043
Change-Id: Ibdc7837ab09b4765436fc4c0d780e695bba07128
Reviewed-on: https://gerrit.openafs.org/15921
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
When making an RPC call from a client, output arguments that use
arrays (or array-like objects like strings and opaques) can be
allocated by XDR, like so:
{
struct idlist ids;
ids.idlist_val = NULL;
ids.idlist_len = 0;
code = PR_NameToID(rxconn, names, &ids);
/* data inside ids.idlist_val[...] */
xdr_free((xdrproc_t) xdr_idlist, &ids);
}
With this approach, during XDR_DECODE, xdr_array() reads in the number
of array elements from the peer, then allocates enough memory to hold
that many elements, and then reads in the array elements.
Alternatively, the caller can provide preallocated memory, like so:
{
struct idlist ids;
afs_int32 ids_buf[30];
ids.idlist_val = ids_buf;
ids.idlist_len = 30;
code = PR_NameToID(rxconn, names, &ids);
/* data inside ids.idlist_val[...] */
}
With this approach, during XDR_DECODE, xdr_array() reads in the number
of array elements from the peer, and then reads in the array elements
into the supplied buffer. However, in this case, xdr_array() never
checks that the number of array elements will actually fit into the
supplied buffer; the _len field provided by the caller is just ignored.
In this example, if the ptserver responds with 50 elements for the 'ids'
output argument, xdr_array() will write 50 afs_int32's into
'ids.idlist_val', going beyond the end of the 30 elements that are
actually allocated.
It's also possible, and in fact very easy, to use xdr-allocated
buffers and then reuse them as a preallocated buffer, possibly
accidentally. For example:
{
struct idlist ids;
ids.idlist_val = NULL;
ids.idlist_len = 0;
while (some_condition) {
code = PR_NameToID(rxconn, names, &ids);
}
}
In this case, the first call to PR_NameToID can cause the buffer for
'ids' to be allocated by XDR, which will then be reused by the
subsequent calls to PR_NameToId. Note that this can happen even if the
first PR_NameToID call fails; the call can be aborted after the output
array is allocated.
Retrying an RPC in this way is effectively what all ubik_Call*
codepaths do (including all ubik_* wrappers, e.g. ubik_PR_NameToID).
Or some callers retry effectively the same RPC when falling back to
earlier versions (e.g. VL_ListAttributesN2 -> VL_ListAttributesN).
To prevent this for arrays and opaques, change xdr_array (and
xdr_bytes) to check if the _len field for preallocated buffers is
large enough, and return failure if it's not.
Also perform the same check for the ka_CBS and ka_BBS structures. These
are mostly the same as opaques, but they have custom serialization
functions in src/kauth/kaaux.c. ka_BBS also has two lengths: the actual
length of bytes, and a 'max' length. ka_CBS isn't used for any RPC
output arguments, but fix it for consistency.
For strings, the situation is complicated by the fact that callers
cannot pass in how much space was allocated for the string, since
callers only provide a char**. So for strings, just refuse to use a
preallocated buffer at all, and return failure if one is provided.
Note that for some callers using preallocated arrays or strings, the
described buffer overruns are not possible, since the preallocated
buffers are larger than the max length specified in the relevant
RPC-L. For example, afs_DoBulkStat() allocates AFSCBMAX entries for
the output args for RXAFS_InlineBulkStatus, which is the max length
specified in the RPC-L, so a buffer overrun is impossible. But since
it is so easy to allow a buffer overrun, enforce the length checks for
everyone.
FIXES 135043
Change-Id: Iaf913e2156af2081c72125ec066d9636086c7d14
Reviewed-on: https://gerrit.openafs.org/15920
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
Currently, a few RPCs with arrays or opaque OUT arguments are called
with preallocated memory for the arg, but also provide a _len of 0 (or
an uninitialized _len). This makes it impossible for the xdr routine to
tell whether we have allocated enough space to actually hold the
response from the server.
To help this situation, either specify an appropriate _len for the
preallocated value (cm_IoctlGetACL, fsprobe_LWP), or don't provide a
preallocated buffer at all and let xdr allocate a buffer for us
(PGetAcl).
Note that this commit doesn't change xdr to actually check the value of
the given _len; but now a future commit can do so without breaking
callers.
FIXES 135043
Change-Id: Ieb50aaa5ae9a1bde027999ce1c668e0c99b4d82b
Reviewed-on: https://gerrit.openafs.org/15919
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
Currently, several callers call RPCs with string OUT arguments, and
provide preallocated memory for those arguments. This can easily allow a
response from the server to overrun the allocated buffer, stomping over
stack or heap memory.
We could simply make our preallocated buffers larger than the maximum
size that the RPC allows, but relying on that is error prone, and
there's no way for XDR to check if a string buffer is large enough.
Instead, to make sure we don't overrun a given preallocated buffer,
avoid giving a preallocated buffer to such RPCs, and let XDR allocate
the memory for us.
Specifically, this commit changes several callers to
RXAFS_GetVolumeStatus(), and one caller of BOZO_GetInstanceParm(), to
avoid passing in a preallocated string buffer.
All other callers of RPCs with string OUT args already let XDR allocate
the buffers for them.
FIXES 135043
Change-Id: If42e2cc983903cff9766e1bab487142d4d493a17
Reviewed-on: https://gerrit.openafs.org/15918
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10396
Several places in the tree parse ACLs using sscanf() calls that look
similar to this:
sscanf(str, "%d dfs:%d %s", &nplus, &dfs, cell);
sscanf(str, "%100s %d", tname, &trights);
Some callers check whether the scanf() returns negative or 0, but some
callers do not check the return code at all. If only some of the fields
are present in the sscanf()'d string (because, for instance, the ACL is
malformed), some of the arguments are left alone, and may be set to
garbage if the relevant variable was never initialized.
If the parsed ACL is copied to another ACL, this can result in the
copied ACL containing uninitialized memory.
To avoid this, make sure all of the variables passed to sscanf() and
similar calls are initialized before parsing. This commit does not
guarantee that the results make sense, but at least the results do not
contain uninitialized memory.
Change-Id: Ib0becffbce5a6e15f91ac4390bb9c422d478bea6
Reviewed-on: https://gerrit.openafs.org/15917
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10396
After the preceding commits, the data returned by the VIOCGETAL
pioctl (a RXAFS_FetchAcl wrapper) will safely be NUL-terminated.
However, the callers that attempt to parse the ACL string make
assumptions that the returned data will be properly formatted,
and implement a "skip to next line" functionality (under various
names) that blindly increments a char* until it finds a newline
character, which can read past the end of even a properly
NUL-terminated string if there is not a newline where one is
expected.
Adjust the various "skip to next line" functionality to keep
the current string pointer at the trailing NUL if the end of the
string is reached while searching for a newline.
Change-Id: I7fb7f23d7d6f68608f3e656a1530a7fc40b4a567
Reviewed-on: https://gerrit.openafs.org/15916
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10396
Supplement the previous commit by additionally verifying that
the returned ACL string occupies the entire XDR opaque, rejecting
any values returned that have an internal NUL prior to the end
of the opaque.
Change-Id: Iefa3d00a9a0e25ef66b7166fe952aae0603ee3d7
Reviewed-on: https://gerrit.openafs.org/15915
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10396
Analogously to how a call to RXAFS_StoreACL() with a malformed
ACL string can cause a fileserver to perform invalid memory operations,
a malformed ACL string returned in response to a call to RXAFS_FetchACL()
can cause a client to perform invalid memory operations.
Modify all the in-tree callers of the RPC to verify that the ACL
data, which is conveyed as an XDR 'opaque' but whose contents
are actually expected to be a string, is a valid C string. If
a zero-length opaque or one without a trailing NUL is received,
treat that as an error response from the fileserver rather than
returning success.
The Unix cache manager's pioctl handler already has logic to cope with a
zero-length reply by emitting a single NUL byte to userspace. This
special-casing seems to have been in place from the original IBM import,
though it does so by confusingly "skipping over" a NUL byte already put
in place. For historical compatibility, preserve that behavior rather
than treating the zero-length reply as an error as we do for the other
callers. It seems likely that this location should treat a zero-length
reply as an error just as the other call sites do, but that can be done
as a later change.
Change-Id: Ibf685e54e7e3fca6a4caac63c961cfcfb2f4732a
Reviewed-on: https://gerrit.openafs.org/15914
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10396
Currently in SRXAFS_StoreACL, if CallPreamble() or check_acl() fail, we
will jump to Bad_StoreACL, which will pass the ACL string from the
client to osi_auditU. Since check_acl() hasn't yet checked if the given
ACL contains a NUL byte, the ACL may be an unterminated string. If
auditing is enabled, this can cause garbage to be logged to the audit
log, or cause the fileserver to crash.
To avoid this, set 'rawACL' to NULL at first, only setting it to the
actual ACL string after check_acl() has succeeded. This ensures that all
code accessing 'rawACL' is guaranteed to be using a terminated string.
This may mean that we pass a NULL AUD_ACL to osi_auditU. Our auditing
code explicitly checks for and handles handles NULL strings, so this is
fine.
FIXES 135445
Change-Id: Iecde5677805a28d55c833b135732a14fd86cc985
Reviewed-on: https://gerrit.openafs.org/15913
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10396
Change our StoreACL implementation to refer to the 'AccessList' argument
via a new local variable called 'rawACL'. This makes it clearer to
users that the data is a string, and makes it easier for future commits
to make sure we don't access the 'AccessList' argument in certain
situations.
Update almost all users in StoreACL to refer to 'rawACL' instead of
'AccessList'. Change the name of 'AccessList' to 'uncheckedACL' to make
sure we don't miss any users. Update our check_acl() call to use
'uncheckedACL' (and not 'rawACL'), because it must use an AFSOpaque to
check the ACL.
Change RXStore_AccessList() and printableACL() to accept a plain char*
instead of a struct AFSOpaque.
This commit should not incur any noticeable behavior change. Technically
printableACL() is changed to run strlen() on the given string, but this
should not cause any noticeable change in behavior:
This change could cause printableACL() to process less of the string
than before, if the string contains a NUL byte before the end of the
AFSOpaque buffer. But this doesn't matter, since the all of our code
after this treats the ACL as a plain string, and so doesn't look at any
data beyond the first NUL. It's not possible for printableACL() to
process more data than before, because check_acl() has already checked
that the ACL string contains a NUL byte, so we must process
AFSOpaque_len bytes or fewer.
FIXES 135445
Change-Id: I26229a39528fec788d96c77012c042c278a214c9
Reviewed-on: https://gerrit.openafs.org/15912
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10396
In acl_Internalize_pr(), each line in an ACL granting rights (positive
or negative) is sscanf()'d with "%63s\t%d\n", and then we try to
advance 'nextc' beyond the next newline character.
However, sscanf()'ing "%63s\t%d\n" does not guarantee that there is a
newline in the given string. Whitespace characters in sscanf() are not
matched exactly, and may match any amount of whitespace (including
none at all). For example, a string like "foo 4" may be parsed by
sscanf(), but does not contain any newlines.
If this happens, strchr(nextc, '\n') will return NULL, and we'll
advance 'nextc' to 0x1, causing a segfault when we next try to
dereference 'nextc'.
To avoid this, check if 'nextc' is NULL after the strchr() call, and
return an error if so.
FIXES 135445
Change-Id: I6bcbbaf88a16202fb84c0932578dd8d5712726dd
Reviewed-on: https://gerrit.openafs.org/15911
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10396
The early parsing code in acl_Internalize_pr() tries to advance
'nextc' to go beyond the first two newlines in the given ACL string.
But if the given ACL string has no newlines, or only 1 newline, then
'nextc' will point beyond the end of the ACL string, potentially
pointing to garbage.
Intuitively, it may look like the ACL string must contain at least 2
newlines because we have sscanf()'d the string with "%d\n%\d".
However, whitespace characters in sscanf() are not matched exactly
like non-whitespace characters are; a sequence of whitespace
characters matches any amount of whitespace (including none). So, a
string like "1 2" will be parsed by "%d\n%d\n", but will not contain
any newline characters.
Usually this should result in a parse error from acl_Internalize_pr(),
but if the garbage happens to parse successfully, this could result in
unrelated memory getting stored to the ACL.
To fix this, don't advance 'nextc' if we're already at the end of the
ACL string.
FIXES 135445
Change-Id: Ie009b59bec9a75afc81fee201c2fca6955f484e4
Reviewed-on: https://gerrit.openafs.org/15910
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10394
Currently, we only throttle PAG creation in afs_setpag(). But there
are several callers that call setpag() directly, not via afs_setpag;
notably _settok_setParentPag in afs_pioctl.c. When setpag() is called
with a PAG value of -1, it generates a new PAG internally without any
throttling. So, those callers effectively bypass the PAG throttling
mechanism, which allows a calling user to create PAGs without any
delay.
To avoid this, move our afs_pag_wait call from afs_setpag() to
afs_genpag(), which all code uses to generate a new PAG value. This
ensures that PAG creation is always throttled for unprivileged users.
FIXES 135062
Change-Id: Ic4cb352edaa693984995fbdb6dc35b89686e8470
Reviewed-on: https://gerrit.openafs.org/15907
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10394
Currently, several areas in the code call genpag() to generate a new
PAG id, but the signature of genpag() is very limited. To allow for
the code in genpag() to return errors and to examine the calling
user's credentials, introduce a new function, afs_genpag(), that does
the same thing as genpag(), but accepts creds and allows errors to be
returned.
Convert all existing callers to use afs_genpag() and to handle any
errors, though no errors are ever returned in this commit on its own.
To ensure there are no old callers of genpag() left around, change the
existing genpag() to be called genpagval(), and declare it static.
FIXES 135062
Change-Id: I5c96c0134db901f21ca30c8b3f57aeec1eb67aa5
Reviewed-on: https://gerrit.openafs.org/14090
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>