sec: restore stack protection on the credential path - #130
Merged
Conversation
src/user.o, src/shell_user.o and src/shell.o were built with
-fno-stack-protector -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0 -O1, so the three
files that parse attacker-supplied passwords were the only part of the kernel
without -fstack-protector-strong. That was recorded nowhere -- neither
CLAUDE.md nor any doc/ mentioned it.
The stated reason ("PBKDF2 has deep call stacks that cause stack overflow with
stack protection") is obsolete: the kernel task stack was later raised from
64 KB to 128 KB (KERNEL_TASK_STACK_PAGES = 32). Measuring the two variables
separately shows the protector was never the cost anyway -- max frame per file,
O1/noSSP vs O1/SSP: user.c 40->40, shell.c 56->56, shell_user.c 296->312. The
larger frames under the restored recipe (shell.c parse_and_execute 3308 bytes)
come from -O1 -> -O2 widening stack slots in one big function, not from the
protector, and are negligible against a 128 KB stack.
The three recipes now collapse to exactly the generic %.o rule, so they are
deleted and replaced by a comment recording why the exception is gone.
Verified: warning-clean -Werror build; all three objects carry __stack_chk
references (i686-elf-nm), matching tcp.o as a control; verify-ring3-cred.sh
PASSes, with the typist trace showing a full `passwd` cycle reaching
"password updated successfully" -- i.e. PBKDF2 actually ran, on the protected
build, and the session survived.
Also corrects two stale comments that asserted the opposite of the shipped
design: scheduler_tick's header and the IRQ0 block in interrupts.c both claimed
context switches cannot happen from interrupts / that the interrupt frame is
rewritten. Neither is true -- interrupts.c:537 preempts, and the preempted task
is suspended mid-ISR via context_switch() and resumes through iret, as
scheduler_schedule_from_interrupt's own comment already documented.
And marks doc/ARCHITECTURAL_SECURITY_ISSUES.md historical: all four issues are
superseded by shipped code (copy_user.c + handle_copy_user_fault, preemption,
wait_queue.c, VFS file_operations_t), each verified present in the tree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CEkhAhgTxbE5TgifyYf8v4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The three files that parse attacker-supplied passwords (
src/user.c,src/shell_user.c,src/shell.c) were the only part of the kernel built without-fstack-protector-strong, via per-file Makefile recipes that also set-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0 -O1. The exception was documented nowhere — not inCLAUDE.md, not in anydoc/.Why it is safe to remove
The stated reason was "PBKDF2 has deep call stacks that cause stack overflow with stack protection." That is obsolete: the kernel task stack was later raised 64 KB → 128 KB (
KERNEL_TASK_STACK_PAGES = 32).Measuring the two variables separately shows the protector was never the cost — max frame per file:
src/user.csrc/shell.csrc/shell_user.cThe protector costs 0–16 bytes. The larger frames under the restored recipe come from
-O1→-O2(wider stack slots in one large function — the function count is unchanged, so nothing was inlined), and 3,308 bytes is negligible against 128 KB.The three recipes now collapse to exactly the generic
%.orule, so they are deleted and replaced by a comment recording why the exception is gone.Verification
-Werrorbuild__stack_chkrefs (i686-elf-nm), matchingtcp.oas controlverify/verify-ring3-cred.shPASS, with the typist trace showing a fullpasswdcycle reachingpassword updated successfully— PBKDF2 actually ran on the protected build and the session survivedAlso in this PR
scheduler_ticks header and the IRQ0 block ininterrupts.cboth claimed context switches cannot happen from interrupts / that the interrupt frame is rewritten. Neither is true —interrupts.c:537preempts, and the preempted task is suspended mid-ISR viacontext_switch(), resuming throughiret, exactly asscheduler_schedule_from_interrupts own comment already documented.doc/ARCHITECTURAL_SECURITY_ISSUES.mdmarked historical. All four issues are superseded by shipped code (copy_user.c+handle_copy_user_fault, preemption,wait_queue.c, VFSfile_operations_t), each verified present.🤖 Generated with Claude Code
https://claude.ai/code/session_01CEkhAhgTxbE5TgifyYf8v4