OpenAFS Master Repository branch, openafs-stable-1_8_x, updated. openafs-stable-1_8_16_1-11-g9a8b359

[Gerrit Code Review]

The following commit has been merged in the openafs-stable-1_8_x branch:
commit 9a8b359c3ee4dd813279650f6018a38e01acec89
Author: Andrew Deason <adeason@sinenomine.net>
Date:   Thu Jun 2 12:22:06 2022 -0500

    rx: Check rxi_AddRpcStat currentFunc bounds
    
    Our rxgen-generated server and client RPC stubs contain some code for
    recording call statistics that looks like this (for BOZO_GetStatus,
    for example; the 4th function in the BOZO_ package):
    
        if (rx_enable_stats) {
    	rx_RecordCallStatistics(z_call, BOZO_STATINDEX, 3,
    				BOZO_NO_OF_STAT_FUNCS, 0);
        }
    
    This eventually calls rxi_AddRpcStat with similar arguments. If a stat
    struct doesn't exist for the given package (BOZO_STATINDEX),
    rxi_AddRpcStat allocates a new struct rx_interface_stat, which can
    accomodate up to 'totalFunc' different functions
    (BOZO_NO_OF_STAT_FUNCS in the above example). If a struct already
    exists for the given package, we'll use the existing struct.
    
    When we update the stats for the current call, we update
    'rpc_stat->stats[currentFunc]' (were 'currentFunc' is 3 in the above
    example). We assume rpc_stats->stats has enough entries allocated for
    this to be okay, but we never actually check. We just assume that
    'currentFunc' is always less than 'totalFunc', and we assume that the
    caller always provides the same 'totalFunc' for that package.
    
    Normally this is true, but this is not checked or verified anywhere.
    If two packages accidentally use the same statindex, this may not
    always be true. For example, say someone defines a new service under
    package FOO_, with 2 functions, and specifies 'statindex 1' in the .xg
    file. This means that FOO_STATINDEX will be 1, and
    FOO_NO_OF_STAT_FUNCS will be 2. If someone calls one of the FOO_ RPCs,
    rxi_AddRpcStat will allocate a struct that can accommodate stats for 2
    functions.
    
    Later on, say that someone calls a BOZO_ RPC. rxi_AddRpcStat will look
    for a struct for BOZO_STATINDEX (which is also 1), and it finds one,
    since we just created one for the FOO_ call before (since
    FOO_STATINDEX is also 1). If the BOZO_ RPC is BOZO_GetStatus,
    'currentFunc' will be 3, which means 'rpc_stat->stats[currentFunc]'
    will go beyond the end of the array allocated for rpc_stat->stats,
    since space has only been allocated for 2 functions. This will
    silently corrupt whatever memory follows the original allocation for
    'rpc_stats', every time someone calls BOZO_GetStatus (or any other
    BOZO_ RPC beyond the first 2 RPCs).
    
    If someone calls BOZO_CreateBnode instead, this won't corrupt memory,
    since the 'currentFunc' given is 0, which will fit into the allocated
    'rpc_stat->stats' array. But the stats will be meaningless, since it
    will contain data for a FOO_ function and some other unrelated BOZO_
    function.
    
    This never happens in the current tree, and should never happen
    normally since different services shouldn't use the same statindex.
    But it can happen by accident, even if no 'statindex' is defined
    (without a defined statindex, the default value is derived from the
    service ID and port; someone could define different services on the
    same port/service). And if it does happen, it can silently corrupt
    memory.
    
    So, to make sure this doesn't corrupt memory by mistake, check that
    'currentFunc' actually fits in the 'rpc_stats->stats' array before
    updating the stats. If it doesn't, just don't update the stats. We
    could assert instead, since this situation should never happen, but
    failing to update our stats is a rather minor side effect, and so
    asserting doesn't seem worth it (especially if we're running in the
    kernel).
    
    Also ignore the stats if the given 'totalFunc' doesn't match what's in
    an existing stats struct. This avoids the case with the
    BOZO_CreateBnode example mentioned above, where there is no memory
    corruption, but the resulting stats are meaningless.
    
    Reviewed-on: https://gerrit.openafs.org/15026
    Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
    Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
    Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
    Tested-by: BuildBot <buildbot@rampaginggeek.com>
    Reviewed-by: Andrew Deason <adeason@sinenomine.net>
    (cherry picked from commit 34a46993434d046a8831c513c84cf1024ef1de16)
    
    Change-Id: I65879b47b70d2d29f1f84fa9a322e6eb06f04468
    Reviewed-on: https://gerrit.openafs.org/16229
    Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
    Reviewed-by: Andrew Deason <adeason@sinenomine.net>
    Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
    Reviewed-by: Kailas Zadbuke <kailashsz@in.ibm.com>
    Tested-by: BuildBot <buildbot@rampaginggeek.com>
    Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>

 src/rx/rx.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

-- 
OpenAFS Master Repository