12 Mar 2026 21:08:03 Josh Law <[email protected]>: > 12 Mar 2026 21:06:31 Steven Rostedt <[email protected]>: > >> On Thu, 12 Mar 2026 19:11:42 +0000 >> Josh Law <[email protected]> wrote: >> >>> From: Josh Law <[email protected]> >>> >>> The bounds check for brace_index happens after the array write. >>> While the current call pattern prevents an actual out-of-bounds >>> access (the previous call would have returned an error), the >>> write-before-check pattern is fragile and would become a real >>> out-of-bounds write if the error return were ever not propagated. >>> >>> Move the bounds check before the array write so the function is >>> self-contained and safe regardless of caller behavior. >> >> This is the only place that increments the index, and the check is >=, >> which means even if there was just one space left, it would fail. >> >> As there's no other place that updates brace_index, I don't believe this >> patch is needed. It could even replace the >= with ==. >> >> -- Steve >> >> >>> >>> Signed-off-by: Josh Law <[email protected]> >>> --- >>> lib/bootconfig.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/lib/bootconfig.c b/lib/bootconfig.c >>> index a1e6a2e14b01..62b4ed7a0ba6 100644 >>> --- a/lib/bootconfig.c >>> +++ b/lib/bootconfig.c >>> @@ -532,9 +532,9 @@ static char *skip_spaces_until_newline(char *p) >>> static int __init __xbc_open_brace(char *p) >>> { >>> /* Push the last key as open brace */ >>> - open_brace[brace_index++] = xbc_node_index(last_parent); >>> if (brace_index >= XBC_DEPTH_MAX) >>> return xbc_parse_error("Exceed max depth of braces", p); >>> + open_brace[brace_index++] = xbc_node_index(last_parent); >>> >>> return 0; >>> } > > That's a fair point, Steve. Given that brace_index isn't touched elsewhere > and the current check effectively prevents the overflow, I agree this isn't > strictly necessary. I'll drop this patch and stick with the fix for the > off-by-one reporting error instead. Thanks for the feedback!
Wait Steve, Thanks for the look. I see your point that it's currently redundant given the call patterns. It looks like Andrew has already merged this into the -mm tree, likely as a 'belt-and-suspenders' safety measure. I'll keep your feedback in mind for future cleanup, but I'm glad we got the other off-by-one fix in as well! And in my opinion, merging it is a decent idea.
