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

irq: Add 0 address access panic configuration #15562

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

W-M-R
Copy link
Contributor

@W-M-R W-M-R commented Jan 15, 2025

Note: Please adhere to Contributing Guidelines.

Summary

Implemented using up_debugpoint_add

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

Testing

Update this section with details on how did you verify the change,
what Host was used for build (OS, CPU, compiler, ..), what Target was
used for verification (arch, board:config, ..), etc. Providing build
and runtime logs from before and after change is highly appreciated.

Implemented using up_debugpoint_add

Signed-off-by: wangmingrong1 <[email protected]>
@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Jan 15, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 15, 2025

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements. The provided information is far too sparse.

Here's why and what's missing:

  • Summary: "Implemented using up_debugpoint_add" tells us practically nothing. What problem does this solve? What part of the codebase is affected? How does up_debugpoint_add address the issue? A related issue number is essential for tracking and context.

  • Impact: The impact section is completely empty. All the bullet points within the Impact section require explicit "NO" or "YES" answers, and if "YES," a description of the impact. Leaving it blank implies the author hasn't considered the potential consequences of their change.

  • Testing: The testing section is also empty. There are no details on the host system used for building, the target hardware/simulator used for testing, or any logs demonstrating the change's behavior before and after the modification. Without this information, it's impossible to assess the validity of the change.

In short, this PR provides almost no context or evidence for the change. It needs significant revision to be considered.

@@ -59,6 +60,14 @@ struct irq_info_s g_irqvector[NR_IRQS];
* Public Functions
****************************************************************************/

#ifdef CONFIG_ARCH_PROTECT_ZERO_ADDRESS
Copy link
Contributor

Choose a reason for hiding this comment

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

could we move to sched/misc

@@ -538,6 +538,14 @@ config ARCH_PERF_EVENTS
Enable hardware performance counter support for perf events. If
disabled, perf events will use software events only.

config ARCH_PROTECT_ZERO_ADDRESS
bool "Protect address zero"
depends on ARCH_HAVE_DEBUG
Copy link
Contributor

Choose a reason for hiding this comment

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

move to nuttx/Kconfig or sched/Kconfig?

@@ -87,5 +96,11 @@ void irq_initialize(void)
#endif

up_irqinitialize();

#ifdef CONFIG_ARCH_PROTECT_ZERO_ADDRESS
up_debugpoint_add(DEBUGPOINT_WATCHPOINT_RW, 0, 0,
Copy link
Contributor

@anchao anchao Jan 15, 2025

Choose a reason for hiding this comment

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

Why not use the mpu? What happens if access the address 0 + offset?

Copy link
Contributor

Choose a reason for hiding this comment

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

it may legal to fetch the code at address 0, mpu can't distinguish fetch and read.

Copy link
Contributor

Choose a reason for hiding this comment

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

Most null pointer access is read and write, and MPU is clearly more suitable for monitoring null pointer access

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not approve of using debug monitor to only protect address 0, If the offset is based on address 0, it will not be able to protect

Copy link
Contributor

Choose a reason for hiding this comment

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

The automated configuration of MPU will be a rather substantial topic. It involves modifications to the unified linker script. A great deal of include logic needs to be added to the script for all linker segments and constants. We can draw inspiration from the implementations in Zephyr and Linux.

Copy link
Contributor Author

@W-M-R W-M-R Jan 16, 2025

Choose a reason for hiding this comment

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

Using 0+offset to access does not really catch the problem. I will think about the new approach carefully and update this patch.

default n
---help---
When the program accesses address 0, it will assert automatically.
Hardware support is required, such as ARM V8M DWT
Copy link
Contributor

Choose a reason for hiding this comment

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

@W-M-R since it depends on HW support, shouldn't it be dependent on ARCH_HAVE_PROTECT_ZERO_ADDRESS ?

Copy link
Contributor

Choose a reason for hiding this comment

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

already depends on ARCH_HAVE_HWDEBUG

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean ARCH_HAVE_DEBUG not ARCH_HAVE_HWDEBUG, right? But ARMV7M also has ARCH_HAVE_DEBUG and probably doesn't have this feature, correct? RISC-V also have ARCH_HAVE_DEBUG

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the current 0 address protection method is not unified for each ARCH. I will think about it carefully and modify this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants