Handle `munlock` errors consistently

[SaswatPadhi]

Security issue notifications

As per offline (off-GitHub) discussions with @lrstewart, this doesn't seem to be a security issue.

Problem:

If a call to munlock fails, s2n currently raises S2N_ERR_MUNLOCK after free(ptr):

s2n-tls/utils/s2n_mem.c

Lines 74 to 81 in 5a791ac

	static int s2n_mem_free_mlock_impl(void *ptr, uint32_t size)
	{
	int munlock_rc = munlock(ptr, size);
	free(ptr);
	POSIX_ENSURE(munlock_rc == 0, S2N_ERR_MUNLOCK);
	return S2N_SUCCESS;
	}

So munlock failure within s2n_mem_free_mlock_implis treated as a "soft error." However, all of its callers, e.g. s2n_free currently do not treat this soft error differently. In particular, s2n_free would free b->data but return S2N_FAILURE before clearing out b, if munlock fails:

s2n-tls/utils/s2n_mem.c

Lines 286 to 288 in 5a791ac

	POSIX_GUARD(s2n_mem_free_cb(b->data, b->allocated));
	*b = (struct s2n_blob) {0};

This would result in b being in an inconsistent state, and would lead to a double-free error if a client calls s2n_free again on the same s2n_blob on which S2N_ERR_MUNLOCK was raised.

This issue was caught by a recent CBMC harness designed to formally verify deallocator functions for the absence of memory leaks.

Solution:

@lrstewart suggested thatmunlock failures should not happen in the wild, so one solution would be to ignore munlock failures entirely. Concretely, the updated s2n_mem_free_mlock_impl would look like:

static int s2n_mem_free_mlock_impl(void *ptr, uint32_t size)
{
    munlock(ptr, size);
    free(ptr);
    return S2N_SUCCESS;
}

If there are other ideas on how to correctly handle munlock failures, let's discuss here.

• Does this change what S2N sends over the wire? No
• Does this change any public APIs? No
• Which versions of TLS will this impact? N/A

Requirements / Acceptance Criteria:

1. Failures to munlock must be consistently propagated on the call chain: Either be ignored (and continue free-ing) in all functions, or no functions should free anything on munlock failures
2. The object being deallocated should not be left in an inconsistent state: This is to make sure that we can always retry a *_free operation if it returned S2N_FAILURE earlier.

• RFC links: N/A
• Related Issues: N/A
• Will the Usage Guide or other documentation need to be updated? No
• Testing: CBMC harnesses can "simulate" an munlock failure.
  • Will this change trigger SAW changes? Probably not.
  • Should this change be fuzz tested? Probably.

Out of scope:

N/A