icp: Use explicit_memset() exclusively in gcm_clear_ctx()

[ryao]

Motivation and Context

d634d20 had been intended to fix a potential information leak issue where the compiler's optimization passes appeared to remove memset() operations that sanitize sensitive data before memory is freed for use by the rest of the kernel.

When I wrote it, I had assumed that the compiler would not remove the other memset() operations, but upon reflection, I have realized that this was a bad assumption to make. I would rather have a very slight amount of additional overhead when calling gcm_clear_ctx() than risk a future compiler remove memset() calls. This is likely to happen if someone decides to try doing link time optimization and the person will not think to audit the assembly output for issues like this, so it is best to preempt the possibility before it happens.

Description

memset() calls were replaced with explicit_memset() calls

How Has This Been Tested?

No testing was done since this is trivial. The buildbot can test.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[ryao]

The importance of the original patch was overstated in hindsight. I just did a more in-depth analysis of the disassembly of the binary generated by GCC 12.2.1:

#14528 (comment)

Decompiling it via dogbolt.com (after reassembling the disassembly into a .o file) produced this:

typedef struct struct_0 {
    char padding_0[32];
    void* field_20;
    void* field_28;
    char padding_30[48];
    unsigned long long field_60;
    void* field_68;
    void* field_70;
    char padding_78[16];
    void* field_88;
    void* field_90;
    unsigned long long field_98;
    unsigned long long field_a0;
    void* field_a8;
    void* field_b0;
    char padding_b8[16];
    unsigned long long field_c8;
    unsigned int field_d0;
} struct_0;

void gcm_clear_ctx(struct_0 *a0)
{
    unsigned long long v1;  // 4128

    v1 = a0->field_d0;
    a0->field_20 = 0;
    a0->field_28 = 0;
    a0->field_88 = 0;
    a0->field_90 = 0;
    if ((unsigned int)v1 == 1)
    {
        if (!a0->field_98)
            external_func5(0, 0, 265, 0, 0, 0);
        external_func3();
        external_func4(a0->field_98, a0->field_a0);
    }
    if (a0->field_c8)
    {
        external_func1();
        external_func2(a0->field_c8, a0->field_60);
    }
    a0->field_a8 = 0;
    a0->field_b0 = 0;
    a0->field_68 = 0;
    a0->field_70 = 0;
    return;
}

The bit width of the zero assignments were not made obvious by the decompiler, but from the disassembly, we can see that the assignments are being done to 64-bit fields. Comparing to the original source code, we can see that our missing memset() calls had been inlined. Note that ctx->gcm_tmp was uint32_t[4], but was zeroed as if it were uint64_t[2], which is fine.

From a security standpoint, we never had an information leak (at least not when using GCC 12.2.1) and I had been wrong that the compiler had optimized away memset(). However, the compiler's dead store elimination removing memset() prior to a free is a real concern when we need to sanitize memory for security purposes prior to freeing it, so we should move forward with using explicit_memset() for the other memset() operations rather than revert the previous patch. It adds a negligible amount of overhead, but gives peace of mind against overly smart compilers. The concern is documented here (among other places):

https://pvs-studio.com/en/docs/warnings/v597/

[lundman]

Are you to provide explicit_memset() as well? Although I see Windows has a SecureZeroMemory() and I can probably borrow from FreeBSD for macOS

[ryao]

@lundman You should already have it. d634d20 introduced it to the code:

https://github.com/openzfs/zfs/blob/master/module/icp/algs/modes/modes.c#L150

[lundman]

Even better!

[tonyhutter]

As a side note (outside the scope of this PR) we should consider aliasing ZFS's explicit_memset() to the built-in explicit_bzero() or memset_explicit() functions, if they're detected at ./configure time`.

[ryao]

As a side note (outside the scope of this PR) we should consider aliasing ZFS's explicit_memset() to the built-in explicit_bzero() or memset_explicit() functions, if they're detected at ./configure time`.

I have no issue with someone doing that. However, the only theoretical benefit would be making the icp binary a few bytes smaller when there is a built-in function at the expense of more code in the repository and slightly longer build times, so I think it should be a low priority unless some theoretical security benefit is found. If one were found, we would also want to update our own explicit_memset() function for places where there is no built-in.

Note that an alternative way of implementing explicit_memset() would be to use a linker script to alias explicit_memset() to memset(). Then we would not need a builtin explicit_bzero() or memset_explicit(). The reason why an alias through a linker script would work is that the compiler optimization passes will see a call to an unknown function that might or might not modify global state based on values accessed through the pointers, so dead store elimination passes cannot prune it. However, if someone built our code with LTO, then the theory why a linker script alias works would no longer be true unless linker scripts support a way to keep LTO from optimizing away memset().

Implementing the alternative would require supporting at least ld.bfd, ld.gold, lld and maybe also mold. I had considered using a linker script alias when I implemented explicit_memset(), but decided against it because it was not worth the time needed to do it. I had not considered LTO at the time, but doing a linker script alias would require a fallback when LTO is used, unless there is a way to keep LTO from optimizing away memset() through the linker script. I should add that I am not even sure if all of the linkers that people use to build ZFS even support linker scripts. The benefit of the linker script alias is that we would be able to avoid the tiny overhead of an extra stack frame.

[ryao]

I just pushed a typo fix for the commit message. The code changes are exactly the same as they were in prior pushes.