Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use dummy allocator to make accesses defined as per standard #25688

Merged
merged 1 commit into from
Dec 14, 2022
Merged

Use dummy allocator to make accesses defined as per standard #25688

merged 1 commit into from
Dec 14, 2022

Conversation

siddhesh
Copy link
Contributor

@siddhesh siddhesh commented Dec 9, 2022

systemd uses malloc_usable_size() everywhere to use memory blocks obtained through malloc, but that is abuse since the malloc_usable_size() interface isn't meant for this kind of use, it is for diagnostics only. This is also why systemd behaviour is flaky when built with _FORTIFY_SOURCE.

One way to make this more standard (and hence safer) is to, at every malloc_usable_size() call, also 'reallocate' the block so that the compiler can see the larger size. This is done through a dummy reallocator whose only purpose is to tell the compiler about the larger usable size, it doesn't do any actual reallocation.

Florian Weimer pointed out that this doesn't solve the problem of an allocator potentially growing usable size at will, which will break the implicit assumption in systemd use that the value returned remains constant as long as the object is valid. The safest way to fix that is for systemd to step away from using malloc_usable_size() like this.

Resolves #22801.

@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Dec 9, 2022
@siddhesh
Copy link
Contributor Author

siddhesh commented Dec 9, 2022

In file included from /usr/include/string.h:535,
                 from /usr/include/x86_64-linux-gnu/sys/un.h:37,
                 from ../../../../src/shared/ask-password-api.c:17:
In function ‘explicit_bzero’,
    inlined from ‘explicit_bzero_safe’ at ../../../../src/basic/memory-util.h:97:17,
    inlined from ‘explicit_bzero_safe’ at ../../../../src/basic/memory-util.h:95:21,
    inlined from ‘erase_and_free’ at ../../../../src/basic/memory-util.h:112:9,
    inlined from ‘erase_and_freep’ at ../../../../src/basic/memory-util.h:117:9,
    inlined from ‘ask_password_credential’ at ../../../../src/shared/ask-password-api.c:966:42,
    inlined from ‘ask_password_auto’ at ../../../../src/shared/ask-password-api.c:1001:21:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:72:3: error: argument 1 is null but the corresponding size argument 2 value is [1, 18446744073709551615] [-Werror=nonnull]
   72 |   __explicit_bzero_chk (__dest, __len, __glibc_objsize0 (__dest));
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/x86_64-linux-gnu/bits/string_fortified.h: In function ‘ask_password_auto’:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:66:6: note: in a call to function ‘__explicit_bzero_chk’ declared with attribute ‘access (write_only, 1, 2)’
   66 | void __explicit_bzero_chk (void *__dest, size_t __len, size_t __destlen)
      |      ^~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

This looks like a spurious gcc warning, I'll take a closer look and see how to fix up.

src/basic/alloc-util.c Outdated Show resolved Hide resolved
src/basic/alloc-util.h Outdated Show resolved Hide resolved
src/basic/alloc-util.h Outdated Show resolved Hide resolved
src/basic/alloc-util.h Outdated Show resolved Hide resolved
src/basic/alloc-util.h Outdated Show resolved Hide resolved
@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Dec 12, 2022
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Dec 12, 2022
@siddhesh
Copy link
Contributor Author

Changes from v1:

  • Broke out most of malloc_sizeof_safe into an inline function
  • Updated comment
  • Added _returns_nonnull_ to fix the failure with ubsan
  • Fixed coding style issues

Tested with clang, gcc and the sanitizer fuzzer test that had failed, so hopefully this one should run fully clean.

src/basic/alloc-util.h Outdated Show resolved Hide resolved
src/basic/alloc-util.h Outdated Show resolved Hide resolved
src/basic/alloc-util.h Outdated Show resolved Hide resolved
src/basic/alloc-util.h Outdated Show resolved Hide resolved
src/basic/alloc-util.h Outdated Show resolved Hide resolved
@yuwata yuwata added util-lib reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Dec 13, 2022
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Dec 13, 2022
@siddhesh siddhesh requested review from yuwata and poettering and removed request for yuwata December 13, 2022 12:36
src/basic/alloc-util.h Outdated Show resolved Hide resolved
src/basic/alloc-util.c Show resolved Hide resolved
src/basic/alloc-util.h Outdated Show resolved Hide resolved
src/basic/alloc-util.h Outdated Show resolved Hide resolved
src/basic/alloc-util.h Outdated Show resolved Hide resolved
systemd uses malloc_usable_size() everywhere to use memory blocks
obtained through malloc, but that is abuse since the
malloc_usable_size() interface isn't meant for this kind of use, it is
for diagnostics only.  This is also why systemd behaviour is flaky when
built with _FORTIFY_SOURCE.

One way to make this more standard (and hence safer) is to, at every
malloc_usable_size() call, also 'reallocate' the block so that the
compiler can see the larger size.  This is done through a dummy
reallocator whose only purpose is to tell the compiler about the larger
usable size, it doesn't do any actual reallocation.

Florian Weimer pointed out that this doesn't solve the problem of an
allocator potentially growing usable size at will, which will break the
implicit assumption in systemd use that the value returned remains
constant as long as the object is valid.  The safest way to fix that is
for systemd to step away from using malloc_usable_size() like this.

Resolves #22801.
@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Dec 14, 2022
@poettering
Copy link
Member

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed util-lib
Development

Successfully merging this pull request may close these issues.

*** buffer overflow detected *** with GCC 12 and -D_FORTIFY_SOURCE=3
4 participants