Skip to content

Conversation

@jerpelea
Copy link
Contributor

@jerpelea jerpelea commented Oct 2, 2024

Summary
There was an error in the fork() routine when system calls are in use: the child context is saved on the child's user stack, which is incorrect, the context must be saved on the kernel stack instead.

The result is a full system crash if (when) the child executes on a different CPU which does not have the same MMU mappings active.

Impact
RELEASE

Testing
CI

pussuw added 2 commits October 2, 2024 12:27
There was an error in the fork() routine when system calls are in use:
the child context is saved on the child's user stack, which is incorrect,
the context must be saved on the kernel stack instead.

The result is a full system crash if (when) the child executes on a
different CPU which does not have the same MMU mappings active.
We need to record the parent's integer register context upon exception
entry to a separate non-volatile area. Why?

Because xcp.regs can move due to a context switch within the fork() system
call, be it either via interrupt or a synchronization point.

Fix this by adding a "sregs" area where the saved user context is placed.
The critical section within fork() is also unnecessary.
@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small labels Oct 2, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 2, 2024

[Experimental Bot, please feedback here]

NuttX PR Requirements Review

Missing Information:

  • Summary:
    • Functional part of code being changed: Specify which part of the fork() routine and related data structures are modified.
    • How the change works: Detail the implementation fix: How is the child context saved on the kernel stack instead?
  • Impact:
    • Existing feature changed: YES - fork() system call behavior corrected.
    • Impact on user: Potentially YES. While a bug fix, applications relying on the incorrect behavior (however unlikely) may break. Describe this potential impact and if any mitigation is needed.
    • Impact on hardware: Clarify if specific architectures are affected or if the fix is architecture-agnostic.
    • Impact on documentation: YES - If documentation exists for fork(), it should be reviewed and potentially updated to reflect the correct behavior.
    • Impact on security: Potentially YES. Exploiting the incorrect context saving could have security implications. Describe the potential vulnerability and how the fix addresses it.
  • Testing:
    • Build Host(s): Provide details on the development environment used.
    • Target(s): Specify the architectures and boards where the fix was tested.
    • Testing logs: Include relevant logs demonstrating the issue before the change and the successful fix afterward.

Concise Assessment:

The PR addresses a critical issue in fork(), but lacks essential details in the Summary, Impact assessment, and Testing sections to meet NuttX's requirements fully. Provide the missing information to ensure thorough review and acceptance.

@xiaoxiang781216 xiaoxiang781216 merged commit 10e44f8 into apache:releases/12.7 Oct 3, 2024
@lupyuen
Copy link
Member

lupyuen commented Oct 3, 2024

Oops arm-03 fails with the error below. examples/can calls ch_tcf, which comes from yesterday's PR: #13758

Hi @xiaoxiang781216: Wonder if we are building with the releases/12.7 of NuttX Apps? This error seems to be caused by the master branch?

https://github.com/apache/nuttx/actions/runs/11154095098/job/31002846380

Configuration/Tool: lx_cpu/nsh,CONFIG_ARM_TOOLCHAIN_GNU_EABI
Error: can_main.c:361:19: error: 'struct can_hdr_s' has no member named 'ch_tcf'
  361 |       txmsg.cm_hdr.ch_tcf    = 0;
      |   

@lupyuen
Copy link
Member

lupyuen commented Oct 3, 2024

@xiaoxiang781216 I think we have a bug in build.yml: https://github.com/apache/nuttx/pull/9289/files

echo "app_ref=$APPS_REF" >> $GITHUB_OUTPUT

app_ref should be be apps_ref? That's because build.yml refers to apps_ref later: https://github.com/apache/nuttx/blob/master/.github/workflows/build.yml#L100-L108

      - name: Checkout apps repo
        uses: actions/checkout@v4
        with:
          repository: apache/nuttx-apps
          ref: ${{ steps.gittargets.outputs.apps_ref }}

This is causing the incorrect version of NuttX Apps to be checked out.

Update: I have submitted the PR: #13806

@jerpelea jerpelea deleted the bp-13633 branch October 3, 2024 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture 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