Re: [PATCH v2 2/3] lib/bootconfig: check bounds before writing in __xbc_open_brace()

[Josh Law <hlcj1234567@gmail.com>]

12 Mar 2026 21:08:03 Josh Law <hlcj1234567@gmail.com>:

> 12 Mar 2026 21:06:31 Steven Rostedt <rostedt@goodmis.org>:
>
>> On Thu, 12 Mar 2026 19:11:42 +0000
>> Josh Law <hlcj1234567@gmail.com> wrote:
>>
>>> From: Josh Law <objecting@objecting.org>
>>>
>>> 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 <objecting@objecting.org>
>>> ---
>>> 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.