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

STACK_POINTER_RANDOM is not working on ARM for the main thread #23702

Closed
ceolin opened this issue Mar 24, 2020 · 9 comments
Closed

STACK_POINTER_RANDOM is not working on ARM for the main thread #23702

ceolin opened this issue Mar 24, 2020 · 9 comments
Assignees
Labels
area: ARM ARM (32-bit) Architecture area: Kernel bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@ceolin
Copy link
Member

ceolin commented Mar 24, 2020

On arm (and possible others arch with custom swap to main) the main stack is not randomized, because arch_switch_to_main_thread() is called with K_THREAD_STACK_SIZEOF(z_main_stack) in switch_to_main_thread(), completely ignoring the value that was previous randomized in z_setup_new_thread(). The problem is that this value is not available anymore, except if it is build with CONFIG_THREAD_STACK_INFO

@ceolin ceolin added bug The issue is a bug, or the PR is fixing a bug area: Kernel area: ARM ARM (32-bit) Architecture labels Mar 24, 2020
@ceolin ceolin self-assigned this Mar 24, 2020
@ceolin
Copy link
Member Author

ceolin commented Mar 24, 2020

Possible solutions:

  1. Make STACK_POINTER_RANDOM depends on CONFIG_THREAD_STACK_INFO for this platform.
  2. change z_setup_new_thread to return this info
  3. randomize stack before call zetup_new_thread -> the worst probably
  4. Add stack size to k_thread independently of CONFIG_THREAD_STACK_INFO

Other possibilities ? Thoughts ?

@ceolin
Copy link
Member Author

ceolin commented Mar 24, 2020

@nashif nashif added the priority: medium Medium impact/importance bug label Mar 24, 2020
@ioannisg
Copy link
Member

(1) and (2) seem OK to me, (4) as well, if we don't care about the extra bytes...
Didn't take a detailed look yet, though

@stephanosio
Copy link
Member

I haven't followed this up closely; but at first glance, this seems to be being addressed in #24714, is that correct? @ceolin @andrewboie

@andrewboie
Copy link
Contributor

@stephanosio yes correct, I incorporated @ceolin's idea into my series.

My large stack overhaul PR is not landing until 2.4, so if we strongly desire a fix for 2.3 just use @ceolin's PR that he closed and I'll rebase on it later

@ioannisg
Copy link
Member

@stephanosio yes correct, I incorporated @ceolin's idea into my series.

My large stack overhaul PR is not landing until 2.4, so if we strongly desire a fix for 2.3 just use @ceolin's PR that he closed and I'll rebase on it later

That's fine with me.

@github-actions
Copy link

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Jul 26, 2020
@ioannisg ioannisg removed the Stale label Jul 27, 2020
@ioannisg
Copy link
Member

@andrewboie pls, clarify if this issue can be consider closed after we merged #24714

@andrewboie
Copy link
Contributor

yeah this shouldn't be a problem anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ARM ARM (32-bit) Architecture area: Kernel bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants