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

Avoid signed overflow in DBG_FlushInstructionCache #105918

Merged

Conversation

AndreyLalaev
Copy link
Contributor

On ARM32 Linux we can have an infinite loop because of integer overflow.
For example, if DBG_FlushInstructionCache is called with the following parameters & locals:

  dwSize = 28
  pageSize = 4096
  begin = lpBaseAddress = 0x7ffff000
  end = begin + dwSize = 0x7ffff01c

ALIGN_UP(0x7ffff000, 4096) returns 0x80000000 which is actually a negative number because INT_PTR is just int32_t (on ARM32). And here we are getting an infinite loop because begin will never be greater or equal than end.

Fix the issue by using UINT_PTR instead of INT_PTR.

On ARM32 Linux we can have an infinite loop because of integer overflow.
For example, if DBG_FlushInstructionCache is called with
the following parameters & locals:
  dwSize = 28
  pageSize = 4096
  begin = lpBaseAddress = 0x7ffff000
  end = begin + dwSize = 0x7ffff01c

ALIGN_UP(0x7ffff000, 4096) returns 0x80000000 which is actually a
negative number because INT_PTR is just int32_t (on ARM32). And here we
are getting an infinite loop because "begin" will never be greater or
equal than "end".

So, this issue is related to all addresses between INT32_MAX - PAGE_SIZE and
INT32_MAX because ALIGN_UP returns the address of the next page which
will be greater or equal to INT32_MAX

Signed-off-by: Andrei Lalaev <andrei.lalaev@anton-paar.com>
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you

@m-korber
Copy link

Are there any plans to release this for .NET 6 on ARM32 anytime soon?

@jkotas
Copy link
Member

jkotas commented Aug 21, 2024

.NET 6 is near end of life. You will need to upgrade soon to stay on a supported version.

We can consider backporting this fix to .NET 8 & 9 if the bug impacts real world applications. Do you hit this bug in your applications?

@jkotas jkotas merged commit cfa8da5 into dotnet:main Aug 21, 2024
90 checks passed
@m-korber
Copy link

Yes we're affected in a way that our embedded applications hangs during startup every 10-20th time due to this bug (patch was actually provided by us).

We know that we should switch to .NET 8 or later, however, we can't due to another bug: #102396

So it would be really helpful if you could backport this fix to .NET 6, too.

@jkotas
Copy link
Member

jkotas commented Aug 22, 2024

/backport to release/9.0

Copy link
Contributor

Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10512533851

@jkotas
Copy link
Member

jkotas commented Aug 22, 2024

backport this fix to .NET 6

I am sorry. This is not a security fix. Our policy is to only backport security fixed during the last 6 months of the release lifetime: https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core .

we can't due to another bug: #102396

Could you please try .NET 9 Preview 7 to see whether the crash still repros?

If it still repros on .NET 9, it would be useful to get more details about the crash - unmanaged stacktraces, disassembly of the code around the place where it crashed, etc.

@AndreyLalaev AndreyLalaev deleted the coreclr-flush-icache-arm32-signed-overflow branch August 23, 2024 20:22
@m-korber
Copy link

m-korber commented Sep 6, 2024

Could you please try .NET 9 Preview 7 to see whether the crash still repros?

Prepared a setup with Toradex Apalis imx6q with Yocto scarthgap and .NET9 Preview 7: Issue still exists and application crashes.

If it still repros on .NET 9, it would be useful to get more details about the crash - unmanaged stacktraces, disassembly of the code around the place where it crashed, etc.

We'll do so in #102396

@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants