Skip to content
New issue

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

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

Already on GitHub? # to your account

Add inlines for overflow detection #5643

Merged
merged 4 commits into from
May 10, 2022
Merged

Conversation

smcv
Copy link
Contributor

@smcv smcv commented May 9, 2022

Description

It's common to do arithmetic to work out how much space to allocate for something. This can be a problem if the inputs to the arithmetic are attacker-supplied and could overflow (as in libsdl-org/SDL_ttf#187 if using a malicious attacker-supplied font). Signed integer overflow is undefined behaviour, which could make anything happen, including a crash or a security vulnerability. Unsigned overflow is defined to wrap around, but that's exactly what we don't want when dealing with memory allocations, because it would be Very Bad to allocate less memory than we thought we had.

We can defend against this sort of thing by doing explicit bounds checks, but because signed overflow is UB, it's necessary to avoid actually doing the computation that would overflow. Recent versions of gcc (and I think clang?) have some builtins that are useful for this, and it's reasonably straightforward to emulate them for other compilers by shuffling around the expression to ensure no single step overflows.

Existing Issue(s)

No specific issue; defending against issues like libsdl-org/SDL_ttf#187

Commits

  • stdinc: Add overflow-checking add and multiply for size_t

    This can be used to check whether untrusted sizes would cause overflow
    when used to calculate how much memory is needed.

  • test: Add a unit test for overflow detection

  • cpuinfo: Set padding to 0 if none is needed

    It'll be simpler to use overflow detection after this refactor.

  • cpuinfo: Check for overflow in SIMD allocation

    If the size to be allocated is very large and untrusted, then adding
    the padding etc. might be enough to cause unsigned overflow, after
    which a very small amount of memory will be allocated.

smcv added 4 commits May 9, 2022 18:46
This can be used to check whether untrusted sizes would cause overflow
when used to calculate how much memory is needed.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
It'll be simpler to use overflow detection after this refactor.

Signed-off-by: Simon McVittie <smcv@collabora.com>
If the size to be allocated is very large and untrusted, then adding
the padding etc. might be enough to cause unsigned overflow, after
which a very small amount of memory will be allocated.

Signed-off-by: Simon McVittie <smcv@collabora.com>
@slouken slouken requested a review from icculus May 9, 2022 18:39
@slouken slouken added this to the 2.0.24 milestone May 9, 2022
}

#if _SDL_HAS_BUILTIN(__builtin_mul_overflow)
SDL_FORCE_INLINE int _SDL_size_mul_overflow_builtin (size_t a,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should add a comment in the code, but for now, a comment on the PR instead:

The reason the builtin is wrapped in an inline is that __builtin_mul_overflow() is generic across types (!) but I'm fairly sure we want this to be consistently interpreting its arguments as size_t for consistency with the non-builtin.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up for this is on #5644

@slouken slouken merged commit 2a79480 into libsdl-org:main May 10, 2022
smcv added a commit to smcv/SDL_ttf that referenced this pull request May 10, 2022
On practical computers, doing memory-size calculations in the signed
integer domain should work fine; but it could be technically possible
for a crafted TTF file to make the height or width negative, resulting
in signed integer overflow, which is formally undefined behaviour
(although gcc -fwrapv defines it to wrap around as twos-complement).

To make it more obvious that this is right, do all size calculations
in AllocateAlignedPixels in the unsigned size_t domain, with
range-checks that do not rely on the behaviour at overflow.

SDL_size_add_overflow() and SDL_size_mul_overflow were added by
libsdl-org/SDL#5643, but a fallback implementation is included here to
avoid a dependency on the new SDL (which hasn't been released yet anyway).

Signed-off-by: Simon McVittie <smcv@collabora.com>
slouken pushed a commit to libsdl-org/SDL_ttf that referenced this pull request May 10, 2022
On practical computers, doing memory-size calculations in the signed
integer domain should work fine; but it could be technically possible
for a crafted TTF file to make the height or width negative, resulting
in signed integer overflow, which is formally undefined behaviour
(although gcc -fwrapv defines it to wrap around as twos-complement).

To make it more obvious that this is right, do all size calculations
in AllocateAlignedPixels in the unsigned size_t domain, with
range-checks that do not rely on the behaviour at overflow.

SDL_size_add_overflow() and SDL_size_mul_overflow were added by
libsdl-org/SDL#5643, but a fallback implementation is included here to
avoid a dependency on the new SDL (which hasn't been released yet anyway).

Signed-off-by: Simon McVittie <smcv@collabora.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants