You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/05/19 20:23:08 UTC
[GitHub] [incubator-nuttx] davids5 opened a new pull request #3747: mm:initialize ensure alignment.
davids5 opened a new pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747
## Summary
Broken by 635cfa.
On an stm32 the heap is formed from
const uintptr_t g_idle_topstack = HEAP_BASE;
where HEAP_BASE is &_bss + CONFIG_IDLE_STACKSIZE.
Both these values are not deterministic. One
comes from the compiler, the other the system
configurator.
a 3 byte bss and 250 byte stack would lead to
and unaligned address used as the heap to be.
The compiler used clever `strd r1,r3,[r5,#8]` to
store 2 values in one memory cycle into the
heap_impl struct. Resulting in a hardfault.
Change the amount of bss or the CONFIG_IDLE_STACKSIZE
could lead to a non-functional NuttX system.
## Impact
Critical to fix in 10.1 release
## Testing
cuav_x7pro_bootloader [PX4]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-844456703
I don't believe this is a problem. The alignment is handled at link time by the linker script.
NOTE:
> where HEAP_BASE is &_bss + CONFIG_IDLE_STACKSIZE.
Actually _ebss, not _bss. _ebss should be the end+1 address of .bss
For stm32f4discovery:
.bss : ALIGN(4) {
_sbss = ABSOLUTE(.);
*(.bss .bss.*)
*(.gnu.linkonce.b.*)
*(COMMON)
. = ALIGN(4);
_ebss = ABSOLUTE(.);
} > sram
So _ebss will always be aligned. A three byte misalignment is impossible. But there could still be an alignment issue since the stack requires 8-byte alignment.
I would think that the correct fix might be to change this to ALIGN(8) in the linker script?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-844473960
@patacongo - ALIGN(8) did not work, the there wild card is the CONFIG_IDLE_STACKSIZE. The fix is restoring how it worked before the 635cfad. The change broke the alignment. Before the heap was adjusted, the changed code intercedes and uses the unaligned value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 edited a comment on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-844977465
> I think @davids5's concern is addressed before merging: both background info and ASAN heap manager is provided in PR.
--------------------------------------------------------
## Testing
Pass the build without error.
-------------------------------
No. Build testing an architectural change of this nature was NOT addressing my concerns.
Yet you merged it.
No one diversity tested it or had a chance to commented on the potential security issue this change could be used to exploit.
The diversity of configuration used to test is what finds this type of thing. Testing on Real HW finds this sort of thing.
I think this raises a bigger issue. The `authored by`, `reviewed by` and `committed by` should NOT be that same organization. That would be akin to merging your own PR. There needs to be real testing and approval on changes greater that a emergency build fix that effect the system.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 edited a comment on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-844473960
@patacongo - ALIGN(8) did not work, and there is the wild card value CONFIG_IDLE_STACKSIZE. The fix is restoring how it worked before the 635cfad. The change broke the alignment. Before the heap was adjusted, the changed code intercedes and uses the unaligned value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-845097148
> > I think @davids5's concern is addressed before merging: both background info and ASAN heap manager is provided in PR.
>
> ## Testing
> ## Pass the build without error.
> No. Build testing an architectural change of this nature was NOT addressing my concerns.
>
The change pass our internal test. The real problem is that your CONFIG_IDLE_STACKSIZE is too odd, but we don't have the similar config to reproduce this issue.
> Yet you merged it.
>
If you look at the timeline: https://github.com/apache/incubator-nuttx/pull/3086
1. PR is created on March 17
2. You give the comment for more info, we update PR immediately
3. There are more dissusion and clarification on March 18
4. I ask people for more comment on March 19
5. Nobody give the nagtive feedback or request more time to test
6. I finally merge PR on March 20
> No one diversity tested it or had a chance to commented on the potential security issue this change could be used to exploit.
>
> The diversity of configuration used to test is what finds this type of thing. Testing on Real HW finds this sort of thing.
>
> I think this raises a bigger issue. The `authored by`, `reviewed by` and `committed by` should NOT be that same organization. That would be akin to merging your own PR. There needs to be real testing and approval on changes greater that a emergency build fix that effect the system.
Yes, we need more test to avoid the regression. But, please give some test plan or progress in PR, so we can coordinate with each other.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] antmerlino commented on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-845134457
> Yes, we need more test to avoid the regression. But, please give some test plan or progress in PR, so we can coordinate with each other.
I think we need to give people more time. Everyone is dealing with their own sets of problems on a day to day basis. 3 days is an eternity when it's your work you're trying to push through - but it's not so long when you've got your own deadlines and then additional PRs come in that need thorough testing/review.
To @davids5 point, this is why it is probably better practice to not allow individuals from the same organization to approve PRs. Both will be incentivized to get it through. Much sophisticated engineering is done and cross-checked using red/blue teams. This mentality helps be critical of the others work. The same philosophy should be being applied here.
I really don't want to create processes that are burdensome and slow things down. However, I do think it's important that we adjust our process to avoid regressions like this and improve the security/reliability of the OS.
What I would propose is this:
1. Change 'Impact' in the PR template to be 'What boards will be affected by this change?'
To this, common answers will be:
- All stm32h7 boards
- All boards
- Any board using XX driver
The reason for this question is to be able to quickly find PRs that will effect everyone. If the PR states it effects all boards, as this change would, we can add a tag to the PR. Those PRs should get a more thorough review.
2. Any PR that gets the tag, or is otherwise labeled a critical change should be reviewed by more than 1 person. Perhaps we say 2 is enough, or maybe it should be 3. In either case we should setup Github to require 2-3 accepted reviews before allowing a merge.
3. In general, I think we should set a policy that individuals from the same organization cannot approve their own PRs.
If this seems like a reasonable proposal, we can move the conversation to the dev list.
Let me know what you guys think.
-Anthony
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-844633152
The change looks good.
> I walked through this with @davids5. I agree with the fix.
>
> This change broke things:
> [635cfad](https://github.com/apache/incubator-nuttx/commit/635cfadc2598f14d7cac4482acd9ed85d5dbe5ac)
>
> What I find interesting about this change is that @davids5 raised a valid criticism about the change needing a better explanation and yet it was approved without even a response to his comment.
>
I think @davids5's concern is addressed before merging: both background info and ASAN heap manager is provided in PR.
> Changes effecting such a critical piece of memory should get a more thorough review than a single reviewer.
Four people join the review in 3 days and get approved before merging. The issue is hard to find in our environment since most link scripts(include ours) add algin(4)/align(8) after .bss.
Anyway, thank you and @davids5 spending the time to find the root cause.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-844977465
> I think @davids5's concern is addressed before merging: both background info and ASAN heap manager is provided in PR.
--------------------------------------------------------
## Testing
Pass the build without error.
-------------------------------
No. Build testing an architectural change of this nature was NOT addressing my concerns.
Yet you merged it.
No one diversity tested it or had a chance to commented on the potential security issue this change could be used to exploit.
The diversity of configuration used to test is what finds this type of thing. Testing on Real HW finds this sort of thing.
I think this raises a bigger issue. The `authored by`, 'reviewed by` and `committed by` should NOT be that same organization. That would be akin to merging your own PR. There needs to be real testing and approval on changes greater that a emergency build fix that effect the system.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-844456703
I don't believe this is a problem. The alignment is handled at link time by the linker script.
NOTE:
> where HEAP_BASE is &_bss + CONFIG_IDLE_STACKSIZE.
Actually _ebss, not _bss. _ebss should be the end+1 address of .bss
For stm32f4discovery:
.bss : ALIGN(4) {
_sbss = ABSOLUTE(.);
*(.bss .bss.*)
*(.gnu.linkonce.b.*)
*(COMMON)
. = ALIGN(4);
_ebss = ABSOLUTE(.);
} > sram
So _ebss will always be aligned. A three byte misalignment is impossible. But there could still be an alignment issue since the stack requires 16-byte alignment.
I would think that the correct fix might be to change this to ALIGN(16) in the linker script?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-845823694
We discussed before the idea of using Code Owners and disabling the merge until XX approvals. It seems that Code Owners is frowned on in Apache but still there are some projects using this feature and I believe we can do so as well (unless there is a better way).
Regarding the "no merge from the same organisation", I think this should be subject to some exceptions. Some areas are only of interest to the people of one organisation. Waiting for approvals from others won't achieve much.
As for changing "Impact" to "What boards will be affected by this change?", I find "Impact" a broader term that includes everything, boards, fs, networking, build system, etc. We just have to clarify in that section.
> If this seems like a reasonable proposal, we can move the conversation to the dev list.
Yes, please. Let's move this discussion to the mailing list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-844445071
FYI @mage1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-845166533
I agree to improve the process to get more review from more people.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 edited a comment on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-844482619
The run below is with the align 8
24001530 A _ebss
The next step is common ISR on the way to hard fault
heap_impl is pointed to by R5 - it is not aligned. We should not rely on liker scripts and Config variables values to enforce the final alignment of an internal struct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-844482619
The run below is with the align 8
24001530 A _ebss
The next step is common ISR on the way to hard fault
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3747: mm:initialize ensure alignment.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-844456703
I don't believe this is a problem. The alignment is handled at link time by the linker script.
NOTE:
> where HEAP_BASE is &_bss + CONFIG_IDLE_STACKSIZE.
Actually _ebss, not _bss. _ebss should be the end+1 address of .bss
For stm32f4discovery:
.bss : ALIGN(4) {
_sbss = ABSOLUTE(.);
*(.bss .bss.*)
*(.gnu.linkonce.b.*)
*(COMMON)
. = ALIGN(4);
_ebss = ABSOLUTE(.);
} > sram
So _ebss will always be aligned. A three byte misalignment is impossible. But there could still be an alignment issue since the stack requires 8-byte alignment.
An alternative fix might be to change this to ALIGN(8) in the linker script?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org