Add an environment IPMI service reporting kstuff's ShellCore capabilities #113
No reviewers
Labels
No labels
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-drakmor/ShadowMountPlus!113
Loading…
Reference in a new issue
No description provided.
Delete branch "sparky3387/envsvc-kstuff-capabilities"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds an environment IPMI service so a sandboxed title can ask the console what
it is running on, and answers the one question a backported title actually needs:
whether the loaded kstuff carries the ShellCore patches its NP registrations
depend on.
Why a capability probe rather than a version check
kstuff's loader exits after patching. A micro-ELF stays resident, but it carries
no version string, its kekcall interface has no call that would return one, and
the ShellCore patches are applied by the loader, so the resident half never
learns they happened.
A version would not be trustworthy even if one existed. Measured on 4.03,
probing each payload present for both patches:
The
-drfork numbers above 1.10 and carries neither patch, so aminimum-version check waves through the exact build that does not work. What is
knowable is the patch itself, read out of SceShellCore's live text, matched
against the whole patch signature so a testkit or devkit layout cannot produce a
false positive.
What is in here
sm_kstuff_caps.c— per-firmware offset table, 2.50 through 12.70.sm_env_ipmi.c— the service, registered asSceShadowMnt, oneargument-free query returning capabilities plus this build's own version.
Never fatal: a console with no backported title does not need it, so a failure
is logged and startup continues.
ipmi_*.c/h— the IPMI server plumbing: runtime symbol resolution, anEventHandler assembled by copying the base vtable and replacing only the slots
that can be identified by address, and a client used to check the service name
is free before claiming it.
Only a positive capability reading is cached. ShellCore is never un-patched, so
a positive cannot go stale, but a zero may only mean kstuff has not run yet,
which depends on autoload ordering. Confirmed with kstuff-lite autoloading
after this payload: the startup probe reads
caps=0x0and the next probeeleven seconds later reads
caps=0x3.Testing
Built clean and run on 4.03 retail.
SceShadowMntregisters and serves, alllibSceIpmi symbols resolve, the EventHandler vtable measures nine slots, and a
real sandboxed title (MW2, PPSA23012) connects and reads
caps=0x3, after whichits UDS and Trophy2
RegisterContextcalls both return 0.Several constants that had been taken on trust were confirmed against a live
registration while preparing this: the server object is constructed into the
storage passed to
create()rather than into scratch,Config+0x32gates thedispatcher, and the EventHandler vtable ends after nine virtuals. The comments
record what was measured.
Summary by CodeRabbit
New Features
Build Improvements
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Advanced
Run ID:
76329245-7a9e-4972-ac1d-f945631f8fb4📥 Commits
Reviewing files that changed from the base of the PR and between
cda661ea53and1f34f77751.📒 Files selected for processing (1)
src/ipmi_handler.c🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
📝 Walkthrough
Walkthrough
The change adds an IPMI environment service. It resolves firmware symbols, builds an event-handler shim, serves a version and capability query, integrates startup and shutdown lifecycle calls, and probes SceShellCore patches for kstuff capabilities.
Changes
IPMI environment service
.gitignore,Makefile,include/ipmi.h,include/ipmi_client.h,include/ipmi_handler.h,include/ipmi_log.h,include/ipmi_symbols.h,include/sm_env_ipmi.h,include/sm_env_ipmi_dispatch.h,include/sm_kstuff_caps.hlibSceIpmiand includes the new IPMI sources. Headers define firmware layouts, symbol tables, handler APIs, service wire data, dispatch results, logging functions, and capability flags.src/ipmi_symbols.c,src/ipmi_client.clibSceIpmi, resolves exported addresses, inspects vtables, and wraps client creation, connection, and destruction.src/ipmi_handler.csrc/sm_env_ipmi.c,src/main.csrc/sm_kstuff_caps.cPriority: ➖ Normal
Estimated code review effort: 5 (Critical) | ~120 minutes
Change: Feature
Sequence Diagram(s)
Merge Risk: 🔵 Low · up to
1f34fA partially resolved IPMI client can leak resources on retries. This is bounded but should be addressed before repeated service starts are relied on.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
❤️ Share
Comment
@coderabbitai helpto get the list of available commands.Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Team
Run ID:
9a960497-f53a-484f-a6d7-dcbc5951ba7c📥 Commits
Reviewing files that changed from the base of the PR and between
3530d8d88eand 3728c52b158ff5b04aa07fe1c8459f55eb7bb828.📒 Files selected for processing (16)
.gitignoreMakefileinclude/ipmi.hinclude/ipmi_client.hinclude/ipmi_handler.hinclude/ipmi_log.hinclude/ipmi_symbols.hinclude/sm_env_ipmi.hinclude/sm_env_ipmi_dispatch.hinclude/sm_kstuff_caps.hsrc/ipmi_client.csrc/ipmi_handler.csrc/ipmi_symbols.csrc/main.csrc/sm_env_ipmi.csrc/sm_kstuff_caps.cIncluded review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
@ -0,0 +157,4 @@/* Ours to free once destroy() has released the kid: storage is our malloc,* not the library's. NULL when it had to be leaked above. */free(c->storage);🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not free client storage when
destroySlot < 0.ipmi_client_opencan succeed without a destruction slot. The probe then callsipmi_client_close, which skipsdestroybut frees the storage that backs theClientobject. This leaves the live client with a dangling backing buffer. Require a valid destruction path before accepting the client, and freec->storageonly after destruction completes.🤖 Prompt for AI Agents
@ -0,0 +1,657 @@// SPDX-License-Identifier: GPL-3.0-or-later🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Guard
cfgbefore writingmemorySize.Line 106 treats
cfgas possibly zero. Line 121 does not. If the framework passescfg == 0, line 128 writes 8 bytes at address0x148, and line 142 passes a nullcfgtocreateSession. Both happen inside the connection window, where a fault kills the process.Add
cfgto the condition.🐛 Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents
✅ Addressed in commits
4dafddeto0eb9fdd🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Allocate per-session storage for
createSessioncapture_connectpasses the same staticg_sessionMemto everyServerImpl::createSessioncall. The handler has no single-session guard, andonSessionKilleddoes not release or replace this storage. If a second client connects while the firstSession*is active, the new session can overwrite the first session's backing memory, corrupting dispatch state and potentially terminating the process. Allocate aligned storage per session, associate it with the returnedSession*, and release it only after the session is killed or destroyed.🤖 Prompt for AI Agents
✅ Addressed in commits
4dafddeto0eb9fdd@ -0,0 +160,4 @@addr ? "" : " <-- MISSING");}logf_("resolved %d/%zu libSceIpmi symbols", found, kSymCount);return true;🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 36661
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 23423
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 3639
Reject startup when
g_syms.srvTryDispatchis unresolved.sm_env_ipmi_servecreates the server and returnstruebefore the detacheddispatcher_threadchecks this symbol. The thread then exits without dispatching requests, whileg_srvremains registered. Checkg_syms.srvTryDispatchbeforeServer::create, or tear down the server and returnfalsewhen it is missing.🤖 Prompt for AI Agents
✅ Addressed in commits
4dafddeto0eb9fdd@ -0,0 +1,421 @@// SPDX-License-Identifier: GPL-3.0-or-later🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Clear
g_srvafter the dispatcher is out, not before.dispatcher_threadre-readsg_srvon every iteration (line 104).server_teardownsetsg_srv = NULLat line 183 and only then setsg_disp_stopat line 188. A dispatcher that has already passed its!g_disp_stopcheck callstryDispatch(NULL, ...), which passes a nullselfinto a firmware virtual call.The wedged path at line 199 makes this worse: it returns with
g_srvstill NULL while the dispatcher keeps polling, so every following iteration repeats the null call.Signal the stop first, wait for
g_disp_aliveto clear, and clearg_srvafter that. The localsrvcopy already keeps the destroy path correct.🐛 Proposed reordering
🤖 Prompt for AI Agents
✅ Addressed in commits
4dafddeto0eb9fdd@ -0,0 +1,179 @@#include "sm_platform.h"🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 32433
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 23044
🌐 Web query:
kstuff-lite SceShellCore sysdirpath trophy patch source💡 Result:
The reference to "kstuff-lite," "SceShellCore," "sysdirpath," and "trophy patch" pertains to open-source kernel-mode patches for the PlayStation 5 (and sometimes PlayStation 4) console, specifically within the kstuff-lite project maintained by developers like EchoStretch [1][2]. In this context, the patches are used to modify the behavior of the SceShellCore process, which manages the system shell and user interface [3]. Regarding the specific terms: Trophy Patch: This refers to modifications (e.g., in
shellui_patch.c) that override the SceShellUI trophy system's online availability checks [4][5]. By forcing functions like IsOnlineModeAvailable to return false, these patches allow users to access and interact with the trophies menu even when the console is not connected to PSN or has account activation issues, though they generally do not work with fully activated accounts [4][5][6]. Sysdirpath: The reference togetSceSysDirPathin the kstuff-lite source code indicates efforts to patch or redirect system directory paths, often used to ensure the console loads the correct system files (like SceShellCore.elf) from specific locations [4][7]. These path redirections are fundamental for custom firmware and homebrew environments to maintain stability and prevent the system from falling back to default or locked configurations [2][7]. These patches are compiled for various firmware versions and are applied by locating the relevant function offsets within the SceShellCore binary [1][8][9]. The kstuff-lite project uses a modular approach where specific patches (defined in header files likeshellcore_patches/10_00.h) are applied to the process memory at runtime to bypass security checks or enable additional functionality [1][10][11].Citations:
github.com/EchoStretch/kstuff-lite@6550243210git.etawen.dev/EchoStretch/kstuff-lite@6550243210git.etawen.dev/EchoStretch/kstuff-lite@6550243210.patchgit.etawen.dev/EchoStretch/kstuff-lite@91273eaf2fgit.etawen.dev/EchoStretch/kstuff-lite@e3d0099ba8.patch🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 2447
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 3959
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 19906
Retry partial capability results.
patch_shellcore()writes the sysdir and trophy entries sequentially.run_probe()can read between those writes and return one capability bit. The current condition then caches that partial result. Require both capability bits before caching a complete result.Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents
✅ Addressed in commit
d645d9aActionable comments posted: 4
🧹 Nitpick comments (2)
🤖 Prompt for all review comments with AI agents
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Team
Run ID:
eba034c7-4511-43b1-94b8-c24f63e24386📥 Commits
Reviewing files that changed from the base of the PR and between 3728c52b158ff5b04aa07fe1c8459f55eb7bb828 and
0eb9fddc73.📒 Files selected for processing (3)
src/ipmi_client.csrc/ipmi_handler.csrc/sm_env_ipmi.cIncluded review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
@ -0,0 +1,163 @@// SPDX-License-Identifier: GPL-3.0-or-later🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Check the
destroy()return code before you freec->storage.Line 133 discards the result of
destroy(). Theelsebranch at lines 135-145 documents why storage must not be freed while libSceIpmi still holds a client kid that points into it. A faileddestroy()leaves the same state, but line 152 then frees the buffer. Treat a negative return the same way as a missing slot: log it and leak the storage.🛡️ Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents
✅ Addressed in commit
d645d9a@ -0,0 +1,657 @@// SPDX-License-Identifier: GPL-3.0-or-later🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
cfgHead + 0x40reads past the end ofcfgHead.cfgHeadis declared asunsigned char cfgHead[0x40]at line 84, so valid offsets end at0x3f. Line 402 copies 4 bytes from offset0x40. That read leaves the array and lands on the followingmemorySizemember, sonumMsgQueuelogs the low half ofmemorySizeon every drained connect, not theSessionImpl::Configfield.capture_connectalso only copiessizeof(g_connect.cfgHead)bytes from the firmwarecfg, so the field at+0x40is never captured. Enlarge the capture buffer to include it.🐛 Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents
✅ Addressed in commit
d645d9a@ -0,0 +373,4 @@respondSlot, (unsigned)respondRc,respondSlot < 0 ? " <-- NOT FOUND: the request is unanswered ""and the kernel will kill us" : "");return SM_ENV_IPMI_ENOTSUP;🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🌐 Web query:
PS4 libSceIpmi EventHandler onSyncMethodDispatch raw arguments respondToSyncMethodRequest💡 Result:
The terms you are referencing appear to be related to the internal mechanisms of the PlayStation 4 system library libSceIpmi. libSceIpmi is a system library (often associated with module handle 0x8000001D) that provides Intelligent Platform Management Interface (IPMI) or similar inter-process/system communication capabilities [1][2][3]. In the context of low-level development for the PlayStation 4 platform, handlers such as onSyncMethodDispatch are typically used for intercepting or responding to synchronous remote procedure calls or inter-process communication (IPC) requests within the system [4][5]. Based on common patterns in Sony's SCE (Sony Computer Entertainment) libraries: 1. onSyncMethodDispatch: This is likely a callback or event handler responsible for processing incoming synchronous requests (method dispatches). 2. Raw Arguments: When a request is dispatched, it often carries a pointer to a data structure containing the request's arguments. Accessing these raw arguments typically involves casting this pointer to the expected structure defined in the internal SCE headers. 3. respondToSyncMethodRequest: This is the corresponding function used to send the result of the processed request back to the caller, effectively closing the request-response cycle. Please note that libSceIpmi is a proprietary, internal system library [1]. There is no official public documentation for these specific functions. The information available in the development community (such as in PS4 reverse engineering repositories like ps4libdoc) is derived from community-maintained, reverse-engineered headers and is not guaranteed to reflect official Sony naming conventions or parameter definitions [6][7][8]. If you are working on homebrew or security research, you may find the reverse-engineered headers in repositories such as libps4-sce-include or PS4 Developer Wiki to be the most relevant resources for examining function signatures [1][7].
Citations:
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 17367
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 16779
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 16621
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 28708
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 18168
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 14535
Reply to raw synchronous requests through
sessRespondSyncRaw.The raw ABI is
Session*, method, input, inputSize, output, outputSize.KIND_SYNC_RAWreturnsSM_ENV_IPMI_ENOTSUPwithout replying. Locateg_syms->sessRespondSyncRawin the session vtable and call it withSM_ENV_IPMI_ENOTSUP,NULL, and0; the raw API has no out-entry count. Otherwise, the synchronous request can remain unanswered and terminate the server.🤖 Prompt for AI Agents
✅ Addressed in commit
d645d9a@ -0,0 +1,421 @@// SPDX-License-Identifier: GPL-3.0-or-later🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
The dispatcher liveness handshake has a startup race.
g_disp_alivestarts false, and only the dispatcher thread itself sets it true at line 101.server_teardownwaits on that flag at lines 195-196 and treats false as "the dispatcher is provably out".Between
pthread_createat line 393 and the thread reaching line 101, the flag is still false. If teardown runs in that window, the wait loop exits on the first check. Teardown then clearsg_srv, callsdestroyat line 222, and freesg_work_bufat line 227. The thread is scheduled afterwards and enters the poll loop with a destroyed server and a freed working buffer, sotryDispatchtakes a danglingselfand a dangling buffer into a firmware virtual call.The thread is detached at line 399, so teardown cannot join it. The flag is the only synchronisation. Publish liveness from the creator instead, so the flag is true before any teardown can observe it.
🐛 Proposed fix
Then drop the assignment at line 101 and keep the two
g_disp_alive = falseexits inside the thread.🤖 Prompt for AI Agents
✅ Addressed in commit
d645d9a@ -0,0 +157,4 @@/* Ours to free once destroy() has released the kid: storage is our malloc,* not the library's. NULL when it had to be leaked above. */free(c->storage);Already fixed in
0eb9fdd, before this review was posted. The quoted comment /* Ours to free either way ... */ was removed in that commit — git log -S "Ours to free either way" finds it only in190880eand0eb9fdd. The else branch now sets c->storage = NULL before the free(), so a missing destroy slot leaks the buffer deliberately rather than handing libSceIpmi a dangling pointer.The second half — "require a valid destruction path before accepting the client" — is deliberately not done, for the reason recorded at lines 138-141: rejecting in open() makes the probe skip its connect(), so name_is_free() would report the name free without having checked it, and create() on a held name terminates the process.
@ -0,0 +157,4 @@/* Ours to free once destroy() has released the kid: storage is our malloc,* not the library's. NULL when it had to be leaked above. */free(c->storage);🧩 Analysis chain
🏁 Script executed:
Length of output: 8964
@sparky3387, verified. The currentipmi_client_closesetsc->storage = NULLwhendestroySlotis absent ordestroy()fails. The laterfree(c->storage)is then safe.The client must remain usable without
destroySlotsoname_is_free()can executeconnect(). Rejecting it inipmi_client_open()would cause a false “free” result and can makecreate()terminate the process when the name is held.This finding does not apply to the current code.
✏️ Learnings added
✅ Review thread resolved.
You are interacting with an AI system.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Advanced
Run ID:
8aa96582-6262-4dea-8c80-e622399aca4a📥 Commits
Reviewing files that changed from the base of the PR and between
d645d9af5dand d08ef0e22fe0f35ef8c8e7869d7cd8883bfc2004.📒 Files selected for processing (3)
include/ipmi_symbols.hsrc/ipmi_handler.csrc/ipmi_symbols.cIncluded review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
@ -0,0 +1,657 @@// SPDX-License-Identifier: GPL-3.0-or-later🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 17000
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 15547
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 17723
Do not derive
g_connectSlotfrom the first unmatched slot.ipmi_syms_resolve()permits individualevh*symbols to remain null.match_mask()then classifies their actual vtable slots asKIND_UNKNOWN. This loop can select an ordinary callback as connect.When that callback runs,
capture_connect()interprets incompatible arguments assrvandcfg. It can then read from or write tocfg + 0x148, which can terminate the process.Require a positively verified connect slot for each firmware, or reject handler construction when the connect callback cannot be identified. Verify this case with partially unresolved EventHandler symbols on every supported firmware.
🤖 Prompt for AI Agents
✅ Confirmed as addressed by @sparky3387
@coderabbitai resume
✅ Action performed
Reviews resumed and review finished.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Advanced
Run ID:
2c21f0f1-260e-4fdb-ada1-ff69220d4ba5📥 Commits
Reviewing files that changed from the base of the PR and between
c88404b654andcda661ea53.📒 Files selected for processing (3)
.gitignoreMakefilesrc/ipmi_handler.c🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
@ -0,0 +1,657 @@// SPDX-License-Identifier: GPL-3.0-or-later🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 22625
🏁 Script executed:
Repository: drakmor/ShadowMountPlus
Length of output: 32386
Require an async reply path before exposing async dispatch.
ipmi_syms_resolve()leaves individual missing symbols asNULL, andhandler_build()does not reject a missingsessRespondAsyncData. If the async handler slots are identified, both async thunks callrespond_async_refusal(). That function returns when the symbol is missing or absent from the session vtable, while the thunk returnsSM_ENV_IPMI_ENOTSUP. The async request can remain unanswered.🤖 Prompt for AI Agents
✅ Addressed in commit
1f34f77@ -0,0 +1,657 @@// SPDX-License-Identifier: GPL-3.0-or-laterFixed in
c88404b—g_connectSlotis no longer derived from the first unmatched slot.It initialises to
-1(src/ipmi_handler.c:67) and is only ever assigned from apositional anchor: an unnamed pair sitting between
KIND_ASYNC_DATAINFOandKIND_SESSION_KILLED(src/ipmi_handler.c:588-594). The path you describe — anevh*symbol failing to resolve, leaving its own slotKIND_UNKNOWN, andfirst-unnamed then handing connect to a destructor — cannot select a slot under
this rule: the anchor requires both named neighbours, and an unresolved symbol
removes exactly the neighbour the pattern needs.
Capture is gated on the identification, not merely on reaching the default arm:
capture_connect()runs only whenslot == g_connectSlot(
src/ipmi_handler.c:415). Withg_connectSlot == -1no slot matches, sonothing interprets
srv/cfgand nothing writescfg + 0x148.When the anchor does not match we log it and degrade to the safe half rather than
rejecting handler construction outright (
src/ipmi_handler.c:597-601):connections are accepted but get no session, every client sees
0x8002000d, andnothing is captured. Sync dispatch is the service's actual job and is unaffected
by a connect slot we could not name, so refusing only to capture leaves the
failure loud, harmless and diagnosable.
On verifying with partially unresolved EventHandler symbols across firmware: the
vtable scan is driven entirely by dlsym results, so a partial resolve is the case
the anchor is written against rather than a property of any particular firmware.
For the record, across all 43 firmware dumps we have (1.00 through 12.70)
libSceIpmi exports every
EventHandlermethod the scan names, so no shippedfirmware produces a partial resolve here.
@ -0,0 +1,657 @@// SPDX-License-Identifier: GPL-3.0-or-later🧩 Analysis chain
🏁 Script executed:
Length of output: 16621
🏁 Script executed:
Length of output: 22982
@sparky3387, verified on the current branch.g_connectSlotis now selected only from the anchored sequence atsrc/ipmi_handler.cLines 588-594. The sequence requires both named neighbors and the two unnamed slots.capture_connect()remains gated byslot == g_connectSlotat Line 415. If the anchor is absent,g_connectSlotstays-1, so no unknown slot can read or write through the callback arguments.handler_build()is called from the service startup path atsrc/sm_env_ipmi.cLine 323. The reported change resolves the original unsafe slot-selection path.If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
✅ Review thread resolved.
You are interacting with an AI system.
@ -0,0 +1,657 @@// SPDX-License-Identifier: GPL-3.0-or-laterAddressed in
1f34f77— though not by gating async dispatch, because the measurement says that gate would be the wrong shape.The precondition does not exist on any shipped firmware. libSceIpmi exports
SessionImpl::respondToAsyncMethodRequest(DataInfo)on all 43 firmware dumps from 1.00 through 12.70, alongside bothEventHandler::onAsyncMethodDispatchoverloads. There is no firmware where the async dispatch slots resolve and the reply symbol does not, soipmi_syms_resolve()cannot produce the state this finding needs.The unanswered request is the firmware's own default, not something this handler introduces. Disassembling the base class at its exported addresses on each of those dumps, every
IPMI::Server::EventHandlervirtual is a singleret:ret/retret/retretEventHandleris a pure no-op interface: leaving a request unanswered is exactly what the base class does for any slot a server does not override. This handler already does strictly better, answeringENOTSUPwhenever the symbol resolves — which is always.That rules out both suggested remedies. Rejecting handler construction would make us stricter than the class we are standing in for, and would take down a sync dispatch path that the missing symbol does not affect. Disabling the async slots is not a fix either: an unnamed slot falls through to
default:and returns 0 without answering, which is the identical hang minus the diagnosis.What did survive is narrower, and worth fixing on its own merits. The guard folded three conditions into one line, so a null
sessRespondAsyncDatawas the single silent exit in a function whose other failure branch logs, sitting next to a sync path that states outright when a request goes unanswered.1f34f77splits the guard so the missing symbol logs like everything else. Behaviour is otherwise unchanged, deliberately.@ -0,0 +1,657 @@// SPDX-License-Identifier: GPL-3.0-or-later🧩 Analysis chain
🏁 Script executed:
Length of output: 23553
@sparky3387, thanks for the detailed firmware evidence.I verified that commit
1f34f77separates thesessRespondAsyncDatacheck inrespond_async_refusal()and logs the missing reply path. The existing session-vtable failure path already logs.The supported firmware evidence shows that the missing-symbol state does not occur for firmware 1.00 through 12.70. Gating handler construction would also disable the independent synchronous dispatch path. I withdraw the original finding.
🐇🔍
✏️ Learnings added
✅ Review thread resolved.
You are interacting with an AI system.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.